From 3f1e11afcc0719661bfa0fa947f47b48f686a1a7 Mon Sep 17 00:00:00 2001 From: xCJPECKOVERx Date: Sun, 24 Aug 2025 22:33:10 -0400 Subject: [PATCH] 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 --- .../model/albums_add_assets_response_dto.dart | 18 +-- open-api/immich-openapi-specs.json | 8 -- open-api/typescript-sdk/src/fetch-client.ts | 2 - server/src/dtos/album.dto.ts | 4 - server/src/repositories/album.repository.ts | 8 ++ server/src/services/album.service.spec.ts | 116 ++++++++++-------- server/src/services/album.service.ts | 71 +++++++---- 7 files changed, 117 insertions(+), 110 deletions(-) diff --git a/mobile/openapi/lib/model/albums_add_assets_response_dto.dart b/mobile/openapi/lib/model/albums_add_assets_response_dto.dart index 168b3f2c45..4ad2c5e150 100644 --- 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 toJson() { final json = {}; - 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(); return AlbumsAddAssetsResponseDto( - albumSuccessCount: mapValueOfType(json, r'albumSuccessCount')!, - assetSuccessCount: mapValueOfType(json, r'assetSuccessCount')!, error: BulkIdErrorReason.fromJson(json[r'error']), success: mapValueOfType(json, r'success')!, ); @@ -124,8 +110,6 @@ class AlbumsAddAssetsResponseDto { /// The list of required keys that must be present in a JSON. static const requiredKeys = { - 'albumSuccessCount', - 'assetSuccessCount', 'success', }; } diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index f00f9082e2..913590d59b 100644 --- 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" diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 5cd59c9d3c..2e3c0dcbfb 100644 --- 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; }; diff --git a/server/src/dtos/album.dto.ts b/server/src/dtos/album.dto.ts index 73630b63cb..00f5759aac 100644 --- 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; } diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index f077c36c41..b023068f16 100644 --- 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 { + 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 diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index f3ba57d744..e22d486bba 100644 --- 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(); diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 32832f0dd3..d7b857d666 100644 --- 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 { const results: AlbumsAddAssetsResponseDto = { success: false, - albumSuccessCount: 0, - assetSuccessCount: 0, error: BulkIdErrorReason.DUPLICATE, }; - const successfulAssetIds: Set = 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; - results.error = undefined; - successfulAssetIds.add(res.id); - } else if (results.error && res.error !== BulkIdErrorReason.DUPLICATE) { - results.error = BulkIdErrorReason.UNKNOWN; - } - } - if (success) { - results.albumSuccessCount++; - } - } catch { - if (results.error) { - results.error = BulkIdErrorReason.UNKNOWN; - } + 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; + 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 }); } } - results.assetSuccessCount = successfulAssetIds.size; return results; }