Browse Source

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"]
```
Michael Manganiello 1 year ago
parent
commit
2288b022bc

+ 11 - 7
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' },

+ 6 - 2
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;

+ 1 - 0
server/src/domain/repositories/album.repository.ts

@@ -26,6 +26,7 @@ export interface IAlbumRepository {
   getByIds(ids: string[]): Promise<AlbumEntity[]>;
   getByAssetId(ownerId: string, assetId: string): Promise<AlbumEntity[]>;
   addAssets(assets: AlbumAssets): Promise<void>;
+  getAssetIds(albumId: string, assetIds?: string[]): Promise<Set<string>>;
   hasAsset(asset: AlbumAsset): Promise<boolean>;
   removeAsset(assetId: string): Promise<void>;
   removeAssets(assets: AlbumAssets): Promise<void>;

+ 22 - 0
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<Set<string>> {
+    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<boolean> {
     return this.repository.exist({
       where: {

+ 1 - 0
server/test/repositories/album.repository.mock.ts

@@ -17,6 +17,7 @@ export const newAlbumRepositoryMock = (): jest.Mocked<IAlbumRepository> => {
     addAssets: jest.fn(),
     removeAsset: jest.fn(),
     removeAssets: jest.fn(),
+    getAssetIds: jest.fn(),
     hasAsset: jest.fn(),
     create: jest.fn(),
     update: jest.fn(),