fix(server): album add/remove asset performance (#4516)

server/src/domain/album/album.service.spec.ts

@@ -1,7 +1,6 @@
 import { BadRequestException } from '@nestjs/common';
 import {
   albumStub,
-  assetStub,
   authStub,
   IAccessRepositoryMock,
   newAccessRepositoryMock,
@@ -225,7 +224,7 @@ describe(AlbumService.name, () => {
         }),
       ).rejects.toBeInstanceOf(BadRequestException);
 
-      expect(albumMock.hasAsset).toHaveBeenCalledWith(albumStub.oneAsset.id, 'not-in-album');
+      expect(albumMock.hasAsset).toHaveBeenCalledWith({ albumId: 'album-4', assetId: 'not-in-album' });
       expect(albumMock.update).not.toHaveBeenCalled();
     });
 
@@ -461,6 +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);
 
       await expect(
         sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }),
@@ -473,9 +473,12 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [assetStub.image, { id: 'asset-1' }, { id: 'asset-2' }, { id: 'asset-3' }],
         albumThumbnailAssetId: 'asset-1',
       });
+      expect(albumMock.addAssets).toHaveBeenCalledWith({
+        albumId: 'album-123',
+        assetIds: ['asset-1', 'asset-2', 'asset-3'],
+      });
     });
 
     it('should not set the thumbnail if the album has one already', async () => {
@@ -490,9 +493,9 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [{ id: 'asset-1' }],
         albumThumbnailAssetId: 'asset-id',
       });
+      expect(albumMock.addAssets).toHaveBeenCalled();
     });
 
     it('should allow a shared user to add assets', async () => {
@@ -512,9 +515,12 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [{ id: 'asset-1' }, { id: 'asset-2' }, { id: 'asset-3' }],
         albumThumbnailAssetId: 'asset-1',
       });
+      expect(albumMock.addAssets).toHaveBeenCalledWith({
+        albumId: 'album-123',
+        assetIds: ['asset-1', 'asset-2', 'asset-3'],
+      });
     });
 
     it('should allow a shared link user to add assets', async () => {
@@ -523,6 +529,7 @@ describe(AlbumService.name, () => {
       accessMock.album.hasSharedLinkAccess.mockResolvedValue(true);
       accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
+      albumMock.hasAsset.mockResolvedValue(false);
 
       await expect(
         sut.addAssets(authStub.adminSharedLink, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }),
@@ -535,9 +542,12 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [assetStub.image, { id: 'asset-1' }, { id: 'asset-2' }, { id: 'asset-3' }],
         albumThumbnailAssetId: 'asset-1',
       });
+      expect(albumMock.addAssets).toHaveBeenCalledWith({
+        albumId: 'album-123',
+        assetIds: ['asset-1', 'asset-2', 'asset-3'],
+      });
 
       expect(accessMock.album.hasSharedLinkAccess).toHaveBeenCalledWith(
         authStub.adminSharedLink.sharedLinkId,
@@ -550,6 +560,7 @@ describe(AlbumService.name, () => {
       accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
       accessMock.asset.hasPartnerAccess.mockResolvedValue(true);
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
+      albumMock.hasAsset.mockResolvedValue(false);
 
       await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1'] })).resolves.toEqual([
         { success: true, id: 'asset-1' },
@@ -558,10 +569,8 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [assetStub.image, { id: 'asset-1' }],
         albumThumbnailAssetId: 'asset-1',
       });
-
       expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1');
     });
 
@@ -569,6 +578,7 @@ describe(AlbumService.name, () => {
       accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
+      albumMock.hasAsset.mockResolvedValue(true);
 
       await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: false, id: 'asset-id', error: BulkIdErrorReason.DUPLICATE },
@@ -620,17 +630,14 @@ 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);
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: true, id: 'asset-id' },
       ]);
 
-      expect(albumMock.update).toHaveBeenCalledWith({
+      expect(albumMock.update).toHaveBeenCalledWith({ id: 'album-123', updatedAt: expect.any(Date) });
-        id: 'album-123',
+      expect(albumMock.removeAssets).toHaveBeenCalledWith({ assetIds: ['asset-id'], albumId: 'album-123' });
-        updatedAt: expect.any(Date),
-        assets: [],
-        albumThumbnailAssetId: null,
-      });
     });
 
     it('should skip assets not in the album', async () => {
@@ -647,9 +654,14 @@ 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);
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
-        { success: false, id: 'asset-id', error: BulkIdErrorReason.NO_PERMISSION },
+        {
+          success: false,
+          id: 'asset-id',
+          error: BulkIdErrorReason.NO_PERMISSION,
+        },
       ]);
 
       expect(albumMock.update).not.toHaveBeenCalled();
@@ -658,6 +670,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);
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: true, id: 'asset-id' },
@@ -666,9 +679,8 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).toHaveBeenCalledWith({
         id: 'album-123',
         updatedAt: expect.any(Date),
-        assets: [assetStub.withLocation],
-        albumThumbnailAssetId: assetStub.withLocation.id,
       });
+      expect(albumMock.updateThumbnails).toHaveBeenCalled();
     });
   });
 

server/src/domain/album/album.service.ts

@@ -120,7 +120,7 @@ export class AlbumService {
     const album = await this.findOrFail(id, { withAssets: true });
 
     if (dto.albumThumbnailAssetId) {
-      const valid = await this.albumRepository.hasAsset(id, dto.albumThumbnailAssetId);
+      const valid = await this.albumRepository.hasAsset({ albumId: id, assetId: dto.albumThumbnailAssetId });
       if (!valid) {
         throw new BadRequestException('Invalid album thumbnail');
       }
@@ -148,35 +148,34 @@ export class AlbumService {
   }
 
   async addAssets(authUser: AuthUserDto, id: string, dto: BulkIdsDto): Promise<BulkIdResponseDto[]> {
-    const album = await this.findOrFail(id, { withAssets: true });
+    const album = await this.findOrFail(id, { withAssets: false });
 
     await this.access.requirePermission(authUser, Permission.ALBUM_READ, id);
 
     const results: BulkIdResponseDto[] = [];
-    for (const id of dto.ids) {
+    for (const assetId of dto.ids) {
-      const hasAsset = album.assets.find((asset) => asset.id === id);
+      const hasAsset = await this.albumRepository.hasAsset({ albumId: id, assetId });
       if (hasAsset) {
-        results.push({ id, success: false, error: BulkIdErrorReason.DUPLICATE });
+        results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE });
         continue;
       }
 
-      const hasAccess = await this.access.hasPermission(authUser, Permission.ASSET_SHARE, id);
+      const hasAccess = await this.access.hasPermission(authUser, Permission.ASSET_SHARE, assetId);
       if (!hasAccess) {
-        results.push({ id, success: false, error: BulkIdErrorReason.NO_PERMISSION });
+        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
         continue;
       }
 
-      results.push({ id, success: true });
+      results.push({ id: assetId, success: true });
-      album.assets.push({ id } as AssetEntity);
     }
 
-    const newAsset = results.find(({ success }) => success);
+    const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id);
-    if (newAsset) {
+    if (newAssetIds.length > 0) {
+      await this.albumRepository.addAssets({ albumId: id, assetIds: newAssetIds });
       await this.albumRepository.update({
         id,
-        assets: album.assets,
         updatedAt: new Date(),
-        albumThumbnailAssetId: album.albumThumbnailAssetId ?? newAsset.id,
+        albumThumbnailAssetId: album.albumThumbnailAssetId ?? newAssetIds[0],
       });
     }
 
@@ -184,42 +183,37 @@ export class AlbumService {
   }
 
   async removeAssets(authUser: AuthUserDto, id: string, dto: BulkIdsDto): Promise<BulkIdResponseDto[]> {
-    const album = await this.findOrFail(id, { withAssets: true });
+    const album = await this.findOrFail(id, { withAssets: false });
 
     await this.access.requirePermission(authUser, Permission.ALBUM_READ, id);
 
     const results: BulkIdResponseDto[] = [];
-    for (const id of dto.ids) {
+    for (const assetId of dto.ids) {
-      const hasAsset = album.assets.find((asset) => asset.id === id);
+      const hasAsset = await this.albumRepository.hasAsset({ albumId: id, assetId });
       if (!hasAsset) {
-        results.push({ id, success: false, error: BulkIdErrorReason.NOT_FOUND });
+        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND });
         continue;
       }
 
       const hasAccess = await this.access.hasAny(authUser, [
-        { permission: Permission.ALBUM_REMOVE_ASSET, id },
+        { permission: Permission.ALBUM_REMOVE_ASSET, id: assetId },
-        { permission: Permission.ASSET_SHARE, id },
+        { permission: Permission.ASSET_SHARE, id: assetId },
       ]);
       if (!hasAccess) {
-        results.push({ id, success: false, error: BulkIdErrorReason.NO_PERMISSION });
+        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
         continue;
       }
 
-      results.push({ id, success: true });
+      results.push({ id: assetId, success: true });
-      album.assets = album.assets.filter((asset) => asset.id !== id);
-      if (album.albumThumbnailAssetId === id) {
-        album.albumThumbnailAssetId = null;
-      }
     }
 
-    const hasSuccess = results.find(({ success }) => success);
+    const removedIds = results.filter(({ success }) => success).map(({ id }) => id);
-    if (hasSuccess) {
+    if (removedIds.length > 0) {
-      await this.albumRepository.update({
+      await this.albumRepository.removeAssets({ albumId: id, assetIds: removedIds });
-        id,
+      await this.albumRepository.update({ id, updatedAt: new Date() });
-        assets: album.assets,
+      if (album.albumThumbnailAssetId && removedIds.includes(album.albumThumbnailAssetId)) {
-        updatedAt: new Date(),
+        await this.albumRepository.updateThumbnails();
-        albumThumbnailAssetId: album.albumThumbnailAssetId || album.assets[0]?.id || null,
+      }
-      });
     }
 
     return results;

server/src/domain/repositories/album.repository.ts

@@ -11,13 +11,24 @@ export interface AlbumInfoOptions {
   withAssets: boolean;
 }
 
+export interface AlbumAsset {
+  albumId: string;
+  assetId: string;
+}
+
+export interface AlbumAssets {
+  albumId: string;
+  assetIds: string[];
+}
+
 export interface IAlbumRepository {
   getById(id: string, options: AlbumInfoOptions): Promise<AlbumEntity | null>;
   getByIds(ids: string[]): Promise<AlbumEntity[]>;
   getByAssetId(ownerId: string, assetId: string): Promise<AlbumEntity[]>;
-  hasAsset(id: string, assetId: string): Promise<boolean>;
+  addAssets(assets: AlbumAssets): Promise<void>;
-  /** Remove an asset from _all_ albums */
+  hasAsset(asset: AlbumAsset): Promise<boolean>;
-  removeAsset(id: string): Promise<void>;
+  removeAsset(assetId: string): Promise<void>;
+  removeAssets(assets: AlbumAssets): Promise<void>;
   getAssetCountForIds(ids: string[]): Promise<AlbumAssetCount[]>;
   getInvalidThumbnail(): Promise<string[]>;
   getOwned(ownerId: string): Promise<AlbumEntity[]>;

server/src/infra/repositories/album.repository.ts

@@ -1,4 +1,4 @@
-import { AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from '@app/domain';
+import { AlbumAsset, AlbumAssetCount, AlbumAssets, AlbumInfoOptions, IAlbumRepository } from '@app/domain';
 import { Injectable } from '@nestjs/common';
 import { InjectDataSource, InjectRepository } from '@nestjs/typeorm';
 import { DataSource, FindOptionsOrder, FindOptionsRelations, In, IsNull, Not, Repository } from 'typeorm';
@@ -168,16 +168,27 @@ export class AlbumRepository implements IAlbumRepository {
       .createQueryBuilder()
       .delete()
       .from('albums_assets_assets')
-      .where('"albums_assets_assets"."assetsId" = :assetId', { assetId })
+      .where('"albums_assets_assets"."assetsId" = :assetId', { assetId });
+  }
+
+  async removeAssets(asset: AlbumAssets): Promise<void> {
+    await this.dataSource
+      .createQueryBuilder()
+      .delete()
+      .from('albums_assets_assets')
+      .where({
+        albumsId: asset.albumId,
+        assetsId: In(asset.assetIds),
+      })
       .execute();
   }
 
-  hasAsset(id: string, assetId: string): Promise<boolean> {
+  hasAsset(asset: AlbumAsset): Promise<boolean> {
     return this.repository.exist({
       where: {
-        id,
+        id: asset.albumId,
         assets: {
-          id: assetId,
+          id: asset.assetId,
         },
       },
       relations: {
@@ -186,6 +197,15 @@ export class AlbumRepository implements IAlbumRepository {
     });
   }
 
+  async addAssets({ albumId, assetIds }: AlbumAssets): Promise<void> {
+    await this.dataSource
+      .createQueryBuilder()
+      .insert()
+      .into('albums_assets_assets', ['albumsId', 'assetsId'])
+      .values(assetIds.map((assetId) => ({ albumsId: albumId, assetsId: assetId })))
+      .execute();
+  }
+
   async create(album: Partial<AlbumEntity>): Promise<AlbumEntity> {
     return this.save(album);
   }

server/test/repositories/album.repository.mock.ts

@@ -14,7 +14,9 @@ export const newAlbumRepositoryMock = (): jest.Mocked<IAlbumRepository> => {
     softDeleteAll: jest.fn(),
     deleteAll: jest.fn(),
     getAll: jest.fn(),
+    addAssets: jest.fn(),
     removeAsset: jest.fn(),
+    removeAssets: jest.fn(),
     hasAsset: jest.fn(),
     create: jest.fn(),
     update: jest.fn(),