From 2288b022bcba1b7b7ab1aa39980df718acca16dc Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Mon, 23 Oct 2023 09:02:27 -0400 Subject: [PATCH] fix(server): Check album asset membership in bulk (#4603) Add `AlbumRepository` method to retrieve an album's asset ids, with an optional parameter to only filter by the provided asset ids. With this, we can now check asset membership using a single query. When adding or removing assets to an album, checking whether each asset is already present in the album now requires a single query, instead of one query per asset. Related to #4539 performance improvements. Before: ``` // Asset membership and permissions check (2 queries per asset) immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "albums" "AlbumEntity" LEFT JOIN "albums_assets_assets" "AlbumEntity_AlbumEntity__AlbumEntity_assets" ON "AlbumEntity_AlbumEntity__AlbumEntity_assets"."albumsId"="AlbumEntity"."id" LEFT JOIN "assets" "AlbumEntity__AlbumEntity_assets" ON "AlbumEntity__AlbumEntity_assets"."id"="AlbumEntity_AlbumEntity__AlbumEntity_assets"."assetsId" AND ("AlbumEntity__AlbumEntity_assets"."deletedAt" IS NULL) WHERE ( ("AlbumEntity"."id" = $1 AND "AlbumEntity__AlbumEntity_assets"."id" = $2) ) AND ( "AlbumEntity"."deletedAt" IS NULL )) LIMIT 1 -- PARAMETERS: ["3fdf0e58-a1c7-4efe-8288-06e4c3f38df9","b666ae6c-afa8-4d6f-a1ad-7091a0659320"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["b666ae6c-afa8-4d6f-a1ad-7091a0659320","6bc60cf1-bd18-4501-a1c2-120b51276fda"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "albums" "AlbumEntity" LEFT JOIN "albums_assets_assets" "AlbumEntity_AlbumEntity__AlbumEntity_assets" ON "AlbumEntity_AlbumEntity__AlbumEntity_assets"."albumsId"="AlbumEntity"."id" LEFT JOIN "assets" "AlbumEntity__AlbumEntity_assets" ON "AlbumEntity__AlbumEntity_assets"."id"="AlbumEntity_AlbumEntity__AlbumEntity_assets"."assetsId" AND ("AlbumEntity__AlbumEntity_assets"."deletedAt" IS NULL) WHERE ( ("AlbumEntity"."id" = $1 AND "AlbumEntity__AlbumEntity_assets"."id" = $2) ) AND ( "AlbumEntity"."deletedAt" IS NULL )) LIMIT 1 -- PARAMETERS: ["3fdf0e58-a1c7-4efe-8288-06e4c3f38df9","c656ab1c-7775-4ff7-b56f-01308c072a76"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["c656ab1c-7775-4ff7-b56f-01308c072a76","6bc60cf1-bd18-4501-a1c2-120b51276fda"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "albums" "AlbumEntity" LEFT JOIN "albums_assets_assets" "AlbumEntity_AlbumEntity__AlbumEntity_assets" ON "AlbumEntity_AlbumEntity__AlbumEntity_assets"."albumsId"="AlbumEntity"."id" LEFT JOIN "assets" "AlbumEntity__AlbumEntity_assets" ON "AlbumEntity__AlbumEntity_assets"."id"="AlbumEntity_AlbumEntity__AlbumEntity_assets"."assetsId" AND ("AlbumEntity__AlbumEntity_assets"."deletedAt" IS NULL) WHERE ( ("AlbumEntity"."id" = $1 AND "AlbumEntity__AlbumEntity_assets"."id" = $2) ) AND ( "AlbumEntity"."deletedAt" IS NULL )) LIMIT 1 -- PARAMETERS: ["3fdf0e58-a1c7-4efe-8288-06e4c3f38df9","cf82adb2-1fcc-4f9e-9013-8fc03cc8d3a9"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["cf82adb2-1fcc-4f9e-9013-8fc03cc8d3a9","6bc60cf1-bd18-4501-a1c2-120b51276fda"] ``` After: ``` // Asset membership check (1 query for all assets) immich_server | query: SELECT "albums_assets"."assetsId" AS "assetId" FROM "albums_assets_assets" "albums_assets" WHERE "albums_assets"."albumsId" = $1 AND "albums_assets"."assetsId" IN ($2, $3, $4) -- PARAMETERS: ["ca870d76-6311-4e89-bf9a-f5b51ea2452c","b666ae6c-afa8-4d6f-a1ad-7091a0659320","c656ab1c-7775-4ff7-b56f-01308c072a76","cf82adb2-1fcc-4f9e-9013-8fc03cc8d3a9"] // Permissions check (1 query per asset) immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["b666ae6c-afa8-4d6f-a1ad-7091a0659320","6bc60cf1-bd18-4501-a1c2-120b51276fda"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["c656ab1c-7775-4ff7-b56f-01308c072a76","6bc60cf1-bd18-4501-a1c2-120b51276fda"] immich_server | query: SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (SELECT 1 FROM "assets" "AssetEntity" WHERE ("AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2)) LIMIT 1 -- PARAMETERS: ["cf82adb2-1fcc-4f9e-9013-8fc03cc8d3a9","6bc60cf1-bd18-4501-a1c2-120b51276fda"] ``` --- server/src/domain/album/album.service.spec.ts | 18 +++++++++------ server/src/domain/album/album.service.ts | 8 +++++-- .../domain/repositories/album.repository.ts | 1 + .../infra/repositories/album.repository.ts | 22 +++++++++++++++++++ .../repositories/album.repository.mock.ts | 1 + 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index 453539129d..25b616ad6d 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/server/src/domain/album/album.service.spec.ts @@ -460,7 +460,7 @@ describe(AlbumService.name, () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(false); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect( sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }), @@ -485,6 +485,7 @@ describe(AlbumService.name, () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep({ ...albumStub.empty, albumThumbnailAssetId: 'asset-id' })); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1'] })).resolves.toEqual([ { success: true, id: 'asset-1' }, @@ -503,6 +504,7 @@ describe(AlbumService.name, () => { accessMock.album.hasSharedAlbumAccess.mockResolvedValue(true); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithUser)); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect( sut.addAssets(authStub.user1, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }), @@ -529,7 +531,7 @@ describe(AlbumService.name, () => { accessMock.album.hasSharedLinkAccess.mockResolvedValue(true); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(false); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect( sut.addAssets(authStub.adminSharedLink, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }), @@ -560,7 +562,7 @@ describe(AlbumService.name, () => { accessMock.asset.hasOwnerAccess.mockResolvedValue(false); accessMock.asset.hasPartnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(false); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1'] })).resolves.toEqual([ { success: true, id: 'asset-1' }, @@ -578,7 +580,7 @@ describe(AlbumService.name, () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(true); + albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: false, id: 'asset-id', error: BulkIdErrorReason.DUPLICATE }, @@ -592,6 +594,7 @@ describe(AlbumService.name, () => { accessMock.asset.hasOwnerAccess.mockResolvedValue(false); accessMock.asset.hasPartnerAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.oneAsset); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1'] })).resolves.toEqual([ { success: false, id: 'asset-1', error: BulkIdErrorReason.NO_PERMISSION }, @@ -630,7 +633,7 @@ describe(AlbumService.name, () => { it('should allow the owner to remove assets', async () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(true); + albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: true, id: 'asset-id' }, @@ -643,6 +646,7 @@ describe(AlbumService.name, () => { it('should skip assets not in the album', async () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.empty)); + albumMock.getAssetIds.mockResolvedValueOnce(new Set()); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: false, id: 'asset-id', error: BulkIdErrorReason.NOT_FOUND }, @@ -654,7 +658,7 @@ describe(AlbumService.name, () => { it('should skip assets without user permission to remove', async () => { accessMock.album.hasSharedAlbumAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.hasAsset.mockResolvedValue(true); + albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { @@ -670,7 +674,7 @@ describe(AlbumService.name, () => { it('should reset the thumbnail if it is removed', async () => { accessMock.album.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.twoAssets)); - albumMock.hasAsset.mockResolvedValue(true); + albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: true, id: 'asset-id' }, diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index e6f1edfa77..ecff9f49d7 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -152,9 +152,11 @@ export class AlbumService { await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); + const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids); + const results: BulkIdResponseDto[] = []; for (const assetId of dto.ids) { - const hasAsset = await this.albumRepository.hasAsset({ albumId: id, assetId }); + const hasAsset = existingAssetIds.has(assetId); if (hasAsset) { results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE }); continue; @@ -187,9 +189,11 @@ export class AlbumService { await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); + const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids); + const results: BulkIdResponseDto[] = []; for (const assetId of dto.ids) { - const hasAsset = await this.albumRepository.hasAsset({ albumId: id, assetId }); + const hasAsset = existingAssetIds.has(assetId); if (!hasAsset) { results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND }); continue; diff --git a/server/src/domain/repositories/album.repository.ts b/server/src/domain/repositories/album.repository.ts index 276ab796bd..d3ca62da12 100644 --- a/server/src/domain/repositories/album.repository.ts +++ b/server/src/domain/repositories/album.repository.ts @@ -26,6 +26,7 @@ export interface IAlbumRepository { getByIds(ids: string[]): Promise; getByAssetId(ownerId: string, assetId: string): Promise; addAssets(assets: AlbumAssets): Promise; + getAssetIds(albumId: string, assetIds?: string[]): Promise>; hasAsset(asset: AlbumAsset): Promise; removeAsset(assetId: string): Promise; removeAssets(assets: AlbumAssets): Promise; diff --git a/server/src/infra/repositories/album.repository.ts b/server/src/infra/repositories/album.repository.ts index a8cd504146..69df226859 100644 --- a/server/src/infra/repositories/album.repository.ts +++ b/server/src/infra/repositories/album.repository.ts @@ -183,6 +183,28 @@ export class AlbumRepository implements IAlbumRepository { .execute(); } + /** + * Get asset IDs for the given album ID. + * + * @param albumId Album ID to get asset IDs for. + * @param assetIds Optional list of asset IDs to filter on. + * @returns Set of Asset IDs for the given album ID. + */ + async getAssetIds(albumId: string, assetIds?: string[]): Promise> { + const query = this.dataSource + .createQueryBuilder() + .select('albums_assets.assetsId', 'assetId') + .from('albums_assets_assets', 'albums_assets') + .where('"albums_assets"."albumsId" = :albumId', { albumId }); + + if (assetIds?.length) { + query.andWhere('"albums_assets"."assetsId" IN (:...assetIds)', { assetIds }); + } + + const result = await query.getRawMany(); + return new Set(result.map((row) => row['assetId'])); + } + hasAsset(asset: AlbumAsset): Promise { return this.repository.exist({ where: { diff --git a/server/test/repositories/album.repository.mock.ts b/server/test/repositories/album.repository.mock.ts index 20c3552699..7cd0a846b3 100644 --- a/server/test/repositories/album.repository.mock.ts +++ b/server/test/repositories/album.repository.mock.ts @@ -17,6 +17,7 @@ export const newAlbumRepositoryMock = (): jest.Mocked => { addAssets: jest.fn(), removeAsset: jest.fn(), removeAssets: jest.fn(), + getAssetIds: jest.fn(), hasAsset: jest.fn(), create: jest.fn(), update: jest.fn(),