chore(server): Improve add to multiple albums via bulk checks and inserts (#21052)

* - add addAssetIdsToAlbums to album repo - update albumService to determine all albums and assets with access and coalesce into one set of album_assets to insert * - remove hasAsset check (unnecessary) * - lint * - cleanup * - remove success counts from addAssetsToAlbums results - Fix tests * open-api * await album update
2025-11-01 02:17:43 +09:00 · 2025-08-24 22:33:10 -04:00
parent 28dce2d0df
commit 3f1e11afcc
7 changed files with 117 additions and 110 deletions
--- a/mobile/openapi/lib/model/albums_add_assets_response_dto.dart
+++ b/mobile/openapi/lib/model/albums_add_assets_response_dto.dart
@@ -13,16 +13,10 @@ part of openapi.api;
 class AlbumsAddAssetsResponseDto {
  /// Returns a new [AlbumsAddAssetsResponseDto] instance.
  AlbumsAddAssetsResponseDto({
-    required this.albumSuccessCount,
-    required this.assetSuccessCount,
    this.error,
    required this.success,
  });

-  int albumSuccessCount;
-
-  int assetSuccessCount;
-
  ///
  /// Please note: This property should have been non-nullable! Since the specification file
  /// does not include a default value (using the "default:" property), however, the generated
@@ -35,26 +29,20 @@ class AlbumsAddAssetsResponseDto {

  @override
  bool operator ==(Object other) => identical(this, other) || other is AlbumsAddAssetsResponseDto &&
-    other.albumSuccessCount == albumSuccessCount &&
-    other.assetSuccessCount == assetSuccessCount &&
    other.error == error &&
    other.success == success;

  @override
  int get hashCode =>
    // ignore: unnecessary_parenthesis
-    (albumSuccessCount.hashCode) +
-    (assetSuccessCount.hashCode) +
    (error == null ? 0 : error!.hashCode) +
    (success.hashCode);

  @override
-  String toString() => 'AlbumsAddAssetsResponseDto[albumSuccessCount=$albumSuccessCount, assetSuccessCount=$assetSuccessCount, error=$error, success=$success]';
+  String toString() => 'AlbumsAddAssetsResponseDto[error=$error, success=$success]';

  Map<String, dynamic> toJson() {
    final json = <String, dynamic>{};
-      json[r'albumSuccessCount'] = this.albumSuccessCount;
-      json[r'assetSuccessCount'] = this.assetSuccessCount;
    if (this.error != null) {
      json[r'error'] = this.error;
    } else {
@@ -73,8 +61,6 @@ class AlbumsAddAssetsResponseDto {
      final json = value.cast<String, dynamic>();

      return AlbumsAddAssetsResponseDto(
-        albumSuccessCount: mapValueOfType<int>(json, r'albumSuccessCount')!,
-        assetSuccessCount: mapValueOfType<int>(json, r'assetSuccessCount')!,
        error: BulkIdErrorReason.fromJson(json[r'error']),
        success: mapValueOfType<bool>(json, r'success')!,
      );
@@ -124,8 +110,6 @@ class AlbumsAddAssetsResponseDto {

  /// The list of required keys that must be present in a JSON.
  static const requiredKeys = <String>{
-    'albumSuccessCount',
-    'assetSuccessCount',
    'success',
  };
 }
--- a/open-api/immich-openapi-specs.json
+++ b/open-api/immich-openapi-specs.json
@@ -10007,12 +10007,6 @@
      },
      "AlbumsAddAssetsResponseDto": {
        "properties": {
-          "albumSuccessCount": {
-            "type": "integer"
-          },
-          "assetSuccessCount": {
-            "type": "integer"
-          },
          "error": {
            "allOf": [
              {
@@ -10025,8 +10019,6 @@
          }
        },
        "required": [
-          "albumSuccessCount",
-          "assetSuccessCount",
          "success"
        ],
        "type": "object"
--- a/open-api/typescript-sdk/src/fetch-client.ts
+++ b/open-api/typescript-sdk/src/fetch-client.ts
@@ -389,8 +389,6 @@ export type AlbumsAddAssetsDto = {
    assetIds: string[];
 };
 export type AlbumsAddAssetsResponseDto = {
-    albumSuccessCount: number;
-    assetSuccessCount: number;
    error?: BulkIdErrorReason;
    success: boolean;
 };
--- a/server/src/dtos/album.dto.ts
+++ b/server/src/dtos/album.dto.ts
@@ -65,10 +65,6 @@ export class AlbumsAddAssetsDto {

 export class AlbumsAddAssetsResponseDto {
  success!: boolean;
-  @ApiProperty({ type: 'integer' })
-  albumSuccessCount!: number;
-  @ApiProperty({ type: 'integer' })
-  assetSuccessCount!: number;
  @ValidateEnum({ enum: BulkIdErrorReason, name: 'BulkIdErrorReason', optional: true })
  error?: BulkIdErrorReason;
 }
--- a/server/src/repositories/album.repository.ts
+++ b/server/src/repositories/album.repository.ts
@@ -321,6 +321,14 @@ export class AlbumRepository {
      .execute();
  }

+  @Chunked({ chunkSize: 30_000 })
+  async addAssetIdsToAlbums(values: { albumsId: string; assetsId: string }[]): Promise<void> {
+    if (values.length === 0) {
+      return;
+    }
+    await this.db.insertInto('album_asset').values(values).execute();
+  }
+
  /**
   * Makes sure all thumbnails for albums are updated by:
   * - Removing thumbnails from albums without assets
--- a/server/src/services/album.service.spec.ts
+++ b/server/src/services/album.service.spec.ts
@@ -778,9 +778,7 @@ describe(AlbumService.name, () => {

  describe('addAssetsToAlbums', () => {
    it('should allow the owner to add assets', async () => {
-      mocks.access.album.checkOwnerAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set(['album-321']));
+      mocks.access.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123', 'album-321']));
      mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
      mocks.album.getById
        .mockResolvedValueOnce(_.cloneDeep(albumStub.empty))
@@ -792,7 +790,7 @@ describe(AlbumService.name, () => {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 2, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(2);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-123', {
@@ -805,14 +803,18 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-1',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-321', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-123', assetsId: 'asset-1' },
+        { albumsId: 'album-123', assetsId: 'asset-2' },
+        { albumsId: 'album-123', assetsId: 'asset-3' },
+        { albumsId: 'album-321', assetsId: 'asset-1' },
+        { albumsId: 'album-321', assetsId: 'asset-2' },
+        { albumsId: 'album-321', assetsId: 'asset-3' },
+      ]);
    });

    it('should not set the thumbnail if the album has one already', async () => {
-      mocks.access.album.checkOwnerAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set(['album-321']));
+      mocks.access.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123', 'album-321']));
      mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
      mocks.album.getById
        .mockResolvedValueOnce(_.cloneDeep({ ...albumStub.empty, albumThumbnailAssetId: 'asset-id' }))
@@ -824,7 +826,7 @@ describe(AlbumService.name, () => {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 2, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(2);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-123', {
@@ -837,14 +839,18 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-id',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-321', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-123', assetsId: 'asset-1' },
+        { albumsId: 'album-123', assetsId: 'asset-2' },
+        { albumsId: 'album-123', assetsId: 'asset-3' },
+        { albumsId: 'album-321', assetsId: 'asset-1' },
+        { albumsId: 'album-321', assetsId: 'asset-2' },
+        { albumsId: 'album-321', assetsId: 'asset-3' },
+      ]);
    });

    it('should allow a shared user to add assets', async () => {
-      mocks.access.album.checkSharedAlbumAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set(['album-321']));
+      mocks.access.album.checkSharedAlbumAccess.mockResolvedValueOnce(new Set(['album-123', 'album-321']));
      mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
      mocks.album.getById
        .mockResolvedValueOnce(_.cloneDeep(albumStub.sharedWithUser))
@@ -856,7 +862,7 @@ describe(AlbumService.name, () => {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 2, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(2);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-123', {
@@ -869,8 +875,14 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-1',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-321', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-123', assetsId: 'asset-1' },
+        { albumsId: 'album-123', assetsId: 'asset-2' },
+        { albumsId: 'album-123', assetsId: 'asset-3' },
+        { albumsId: 'album-321', assetsId: 'asset-1' },
+        { albumsId: 'album-321', assetsId: 'asset-2' },
+        { albumsId: 'album-321', assetsId: 'asset-3' },
+      ]);
      expect(mocks.event.emit).toHaveBeenCalledWith('AlbumUpdate', {
        id: 'album-123',
        recipientId: 'admin_id',
@@ -896,18 +908,14 @@ describe(AlbumService.name, () => {
        }),
      ).resolves.toEqual({
        success: false,
-        albumSuccessCount: 0,
-        assetSuccessCount: 0,
-        error: BulkIdErrorReason.UNKNOWN,
+        error: BulkIdErrorReason.NO_PERMISSION,
      });

      expect(mocks.album.update).not.toHaveBeenCalled();
    });

    it('should not allow a shared link user to add assets to multiple albums', async () => {
-      mocks.access.album.checkSharedLinkAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set());
+      mocks.access.album.checkSharedLinkAccess.mockResolvedValueOnce(new Set(['album-123']));
      mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
      mocks.album.getById
        .mockResolvedValueOnce(_.cloneDeep(albumStub.sharedWithUser))
@@ -919,7 +927,7 @@ describe(AlbumService.name, () => {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 1, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(1);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-123', {
@@ -927,22 +935,23 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-1',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledTimes(1);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-123', assetsId: 'asset-1' },
+        { albumsId: 'album-123', assetsId: 'asset-2' },
+        { albumsId: 'album-123', assetsId: 'asset-3' },
+      ]);
      expect(mocks.event.emit).toHaveBeenCalledWith('AlbumUpdate', {
        id: 'album-123',
        recipientId: 'user-id',
      });
      expect(mocks.access.album.checkSharedLinkAccess).toHaveBeenCalledWith(
        authStub.adminSharedLink.sharedLink?.id,
-        new Set(['album-123']),
+        new Set(['album-123', 'album-321']),
      );
    });

    it('should allow adding assets shared via partner sharing', async () => {
-      mocks.access.album.checkOwnerAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set(['album-321']));
+      mocks.access.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123', 'album-321']));
      mocks.access.asset.checkPartnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
      mocks.album.getById
        .mockResolvedValueOnce(_.cloneDeep(albumStub.empty))
@@ -954,7 +963,7 @@ describe(AlbumService.name, () => {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 2, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(2);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-123', {
@@ -967,8 +976,14 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-1',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-321', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-123', assetsId: 'asset-1' },
+        { albumsId: 'album-123', assetsId: 'asset-2' },
+        { albumsId: 'album-123', assetsId: 'asset-3' },
+        { albumsId: 'album-321', assetsId: 'asset-1' },
+        { albumsId: 'album-321', assetsId: 'asset-2' },
+        { albumsId: 'album-321', assetsId: 'asset-3' },
+      ]);
      expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(
        authStub.admin.user.id,
        new Set(['asset-1', 'asset-2', 'asset-3']),
@@ -976,23 +991,21 @@ describe(AlbumService.name, () => {
    });

    it('should skip some duplicate assets', async () => {
-      mocks.access.album.checkOwnerAccess
-        .mockResolvedValueOnce(new Set(['album-123']))
-        .mockResolvedValueOnce(new Set(['album-321']));
+      mocks.access.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123', 'album-321']));
      mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
-      mocks.album.getById
-        .mockResolvedValueOnce(_.cloneDeep(albumStub.empty))
-        .mockResolvedValueOnce(_.cloneDeep(albumStub.oneAsset));
      mocks.album.getAssetIds
        .mockResolvedValueOnce(new Set(['asset-1', 'asset-2', 'asset-3']))
        .mockResolvedValueOnce(new Set());
+      mocks.album.getById
+        .mockResolvedValueOnce(_.cloneDeep(albumStub.empty))
+        .mockResolvedValueOnce(_.cloneDeep(albumStub.oneAsset));

      await expect(
        sut.addAssetsToAlbums(authStub.admin, {
          albumIds: ['album-123', 'album-321'],
          assetIds: ['asset-1', 'asset-2', 'asset-3'],
        }),
-      ).resolves.toEqual({ success: true, albumSuccessCount: 1, assetSuccessCount: 3 });
+      ).resolves.toEqual({ success: true, error: undefined });

      expect(mocks.album.update).toHaveBeenCalledTimes(1);
      expect(mocks.album.update).toHaveBeenNthCalledWith(1, 'album-321', {
@@ -1000,8 +1013,11 @@ describe(AlbumService.name, () => {
        updatedAt: expect.any(Date),
        albumThumbnailAssetId: 'asset-1',
      });
-      expect(mocks.album.addAssetIds).toHaveBeenCalledTimes(1);
-      expect(mocks.album.addAssetIds).toHaveBeenCalledWith('album-321', ['asset-1', 'asset-2', 'asset-3']);
+      expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([
+        { albumsId: 'album-321', assetsId: 'asset-1' },
+        { albumsId: 'album-321', assetsId: 'asset-2' },
+        { albumsId: 'album-321', assetsId: 'asset-3' },
+      ]);
    });

    it('should skip all duplicate assets', async () => {
@@ -1021,8 +1037,6 @@ describe(AlbumService.name, () => {
        }),
      ).resolves.toEqual({
        success: false,
-        albumSuccessCount: 0,
-        assetSuccessCount: 0,
        error: BulkIdErrorReason.DUPLICATE,
      });

@@ -1046,9 +1060,7 @@ describe(AlbumService.name, () => {
        }),
      ).resolves.toEqual({
        success: false,
-        albumSuccessCount: 0,
-        assetSuccessCount: 0,
-        error: BulkIdErrorReason.UNKNOWN,
+        error: BulkIdErrorReason.NO_PERMISSION,
      });

      expect(mocks.album.update).not.toHaveBeenCalled();
@@ -1076,9 +1088,7 @@ describe(AlbumService.name, () => {
        }),
      ).resolves.toEqual({
        success: false,
-        albumSuccessCount: 0,
-        assetSuccessCount: 0,
-        error: BulkIdErrorReason.UNKNOWN,
+        error: BulkIdErrorReason.NO_PERMISSION,
      });

      expect(mocks.album.update).not.toHaveBeenCalled();
@@ -1099,9 +1109,7 @@ describe(AlbumService.name, () => {
        }),
      ).resolves.toEqual({
        success: false,
-        albumSuccessCount: 0,
-        assetSuccessCount: 0,
-        error: BulkIdErrorReason.UNKNOWN,
+        error: BulkIdErrorReason.NO_PERMISSION,
      });

      expect(mocks.access.album.checkSharedLinkAccess).toHaveBeenCalled();
--- a/server/src/services/album.service.ts
+++ b/server/src/services/album.service.ts
@@ -191,36 +191,57 @@ export class AlbumService extends BaseService {
  async addAssetsToAlbums(auth: AuthDto, dto: AlbumsAddAssetsDto): Promise<AlbumsAddAssetsResponseDto> {
    const results: AlbumsAddAssetsResponseDto = {
      success: false,
-      albumSuccessCount: 0,
-      assetSuccessCount: 0,
      error: BulkIdErrorReason.DUPLICATE,
    };
-    const successfulAssetIds: Set<string> = new Set();
-    for (const albumId of dto.albumIds) {
-      try {
-        const albumResults = await this.addAssets(auth, albumId, { ids: dto.assetIds });

-        let success = false;
-        for (const res of albumResults) {
-          if (res.success) {
-            success = true;
-            results.success = true;
+    const allowedAlbumIds = await this.checkAccess({
+      auth,
+      permission: Permission.AlbumAssetCreate,
+      ids: dto.albumIds,
+    });
+    if (allowedAlbumIds.size === 0) {
+      results.error = BulkIdErrorReason.NO_PERMISSION;
+      return results;
+    }
+
+    const allowedAssetIds = await this.checkAccess({ auth, permission: Permission.AssetShare, ids: dto.assetIds });
+    if (allowedAssetIds.size === 0) {
+      results.error = BulkIdErrorReason.NO_PERMISSION;
+      return results;
+    }
+
+    const albumAssetValues: { albumsId: string; assetsId: string }[] = [];
+    const events: { id: string; recipients: string[] }[] = [];
+    for (const albumId of allowedAlbumIds) {
+      const existingAssetIds = await this.albumRepository.getAssetIds(albumId, [...allowedAssetIds]);
+      const notPresentAssetIds = [...allowedAssetIds].filter((id) => !existingAssetIds.has(id));
+      if (notPresentAssetIds.length === 0) {
+        continue;
+      }
+      const album = await this.findOrFail(albumId, { withAssets: false });
      results.error = undefined;
-            successfulAssetIds.add(res.id);
-          } else if (results.error && res.error !== BulkIdErrorReason.DUPLICATE) {
-            results.error = BulkIdErrorReason.UNKNOWN;
+      results.success = true;
+
+      for (const assetId of notPresentAssetIds) {
+        albumAssetValues.push({ albumsId: albumId, assetsId: assetId });
+      }
+      await this.albumRepository.update(albumId, {
+        id: albumId,
+        updatedAt: new Date(),
+        albumThumbnailAssetId: album.albumThumbnailAssetId ?? notPresentAssetIds[0],
+      });
+      const allUsersExceptUs = [...album.albumUsers.map(({ user }) => user.id), album.owner.id].filter(
+        (userId) => userId !== auth.user.id,
+      );
+      events.push({ id: albumId, recipients: allUsersExceptUs });
+    }
+
+    await this.albumRepository.addAssetIdsToAlbums(albumAssetValues);
+    for (const event of events) {
+      for (const recipientId of event.recipients) {
+        await this.eventRepository.emit('AlbumUpdate', { id: event.id, recipientId });
      }
    }
-        if (success) {
-          results.albumSuccessCount++;
-        }
-      } catch {
-        if (results.error) {
-          results.error = BulkIdErrorReason.UNKNOWN;
-        }
-      }
-    }
-    results.assetSuccessCount = successfulAssetIds.size;

    return results;
  }