refactor(server): extract add/remove assets logic to utility function (#8329)

extract add/remove assets logic to utility function

fix tests

chore: generate sql

foo

server/src/cores/access.core.ts

@@ -84,7 +84,7 @@ export class AccessCore {
    *
    * @returns Set<string>
    */
-  async checkAccess(auth: AuthDto, permission: Permission, ids: Set<string> | string[]) {
+  async checkAccess(auth: AuthDto, permission: Permission, ids: Set<string> | string[]): Promise<Set<string>> {
     const idSet = Array.isArray(ids) ? new Set(ids) : ids;
     if (idSet.size === 0) {
       return new Set();
@@ -97,7 +97,11 @@ export class AccessCore {
     return this.checkAccessOther(auth, permission, idSet);
   }
 
-  private async checkAccessSharedLink(sharedLink: SharedLinkEntity, permission: Permission, ids: Set<string>) {
+  private async checkAccessSharedLink(
+    sharedLink: SharedLinkEntity,
+    permission: Permission,
+    ids: Set<string>,
+  ): Promise<Set<string>> {
     const sharedLinkId = sharedLink.id;
 
     switch (permission) {
@@ -140,7 +144,7 @@ export class AccessCore {
     }
   }
 
-  private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set<string>) {
+  private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set<string>): Promise<Set<string>> {
     switch (permission) {
       // uses album id
       case Permission.ACTIVITY_CREATE: {

server/src/interfaces/album.interface.ts

@@ -1,4 +1,5 @@
 import { AlbumEntity } from 'src/entities/album.entity';
+import { IBulkAsset } from 'src/utils/asset.util';
 
 export const IAlbumRepository = 'IAlbumRepository';
 
@@ -23,15 +24,14 @@ export interface AlbumAssets {
   assetIds: string[];
 }
 
-export interface IAlbumRepository {
+export interface IAlbumRepository extends IBulkAsset {
   getById(id: string, options: AlbumInfoOptions): Promise<AlbumEntity | null>;
   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(albumId: string, assetIds: string[]): Promise<void>;
+  removeAssetIds(albumId: string, assetIds: string[]): Promise<void>;
   getMetadataForIds(ids: string[]): Promise<AlbumAssetCount[]>;
   getInvalidThumbnail(): Promise<string[]>;
   getOwned(ownerId: string): Promise<AlbumEntity[]>;

server/src/queries/album.repository.sql

@@ -590,7 +590,7 @@ DELETE FROM "albums_assets_assets"
 WHERE
   "albums_assets_assets"."assetsId" = $1
 
--- AlbumRepository.removeAssets
+-- AlbumRepository.removeAssetIds
 DELETE FROM "albums_assets_assets"
 WHERE
   (
@@ -646,7 +646,7 @@ WHERE
 LIMIT
   1
 
--- AlbumRepository.addAssets
+-- AlbumRepository.addAssetIds
 INSERT INTO
   "albums_assets_assets" ("albumsId", "assetsId")
 VALUES

server/src/repositories/album.repository.ts

@@ -5,13 +5,7 @@ import { dataSource } from 'src/database.config';
 import { Chunked, ChunkedArray, DATABASE_PARAMETER_CHUNK_SIZE, DummyValue, GenerateSql } from 'src/decorators';
 import { AlbumEntity } from 'src/entities/album.entity';
 import { AssetEntity } from 'src/entities/asset.entity';
-import {
-  AlbumAsset,
-  AlbumAssetCount,
-  AlbumAssets,
-  AlbumInfoOptions,
-  IAlbumRepository,
-} from 'src/interfaces/album.interface';
+import { AlbumAsset, AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from 'src/interfaces/album.interface';
 import { Instrumentation } from 'src/utils/instrumentation';
 import { setUnion } from 'src/utils/set';
 import { DataSource, FindOptionsOrder, FindOptionsRelations, In, IsNull, Not, Repository } from 'typeorm';
@@ -203,7 +197,7 @@ export class AlbumRepository implements IAlbumRepository {
 
   @GenerateSql({ params: [DummyValue.UUID, [DummyValue.UUID]] })
   @Chunked({ paramIndex: 1 })
-  async removeAssets(albumId: string, assetIds: string[]): Promise<void> {
+  async removeAssetIds(albumId: string, assetIds: string[]): Promise<void> {
     await this.dataSource
       .createQueryBuilder()
       .delete()
@@ -260,8 +254,8 @@ export class AlbumRepository implements IAlbumRepository {
     });
   }
 
-  @GenerateSql({ params: [{ albumId: DummyValue.UUID, assetIds: [DummyValue.UUID] }] })
-  async addAssets({ albumId, assetIds }: AlbumAssets): Promise<void> {
+  @GenerateSql({ params: [DummyValue.UUID, [DummyValue.UUID]] })
+  async addAssetIds(albumId: string, assetIds: string[]): Promise<void> {
     await this.dataSource
       .createQueryBuilder()
       .insert()

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

@@ -518,10 +518,7 @@ describe(AlbumService.name, () => {
         updatedAt: expect.any(Date),
         albumThumbnailAssetId: 'asset-1',
       });
-      expect(albumMock.addAssets).toHaveBeenCalledWith({
-        albumId: 'album-123',
-        assetIds: ['asset-1', 'asset-2', 'asset-3'],
-      });
+      expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
     });
 
     it('should not set the thumbnail if the album has one already', async () => {
@@ -539,7 +536,7 @@ describe(AlbumService.name, () => {
         updatedAt: expect.any(Date),
         albumThumbnailAssetId: 'asset-id',
       });
-      expect(albumMock.addAssets).toHaveBeenCalled();
+      expect(albumMock.addAssetIds).toHaveBeenCalled();
     });
 
     it('should allow a shared user to add assets', async () => {
@@ -561,10 +558,7 @@ describe(AlbumService.name, () => {
         updatedAt: expect.any(Date),
         albumThumbnailAssetId: 'asset-1',
       });
-      expect(albumMock.addAssets).toHaveBeenCalledWith({
-        albumId: 'album-123',
-        assetIds: ['asset-1', 'asset-2', 'asset-3'],
-      });
+      expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
     });
 
     it('should allow a shared link user to add assets', async () => {
@@ -586,10 +580,7 @@ describe(AlbumService.name, () => {
         updatedAt: expect.any(Date),
         albumThumbnailAssetId: 'asset-1',
       });
-      expect(albumMock.addAssets).toHaveBeenCalledWith({
-        albumId: 'album-123',
-        assetIds: ['asset-1', 'asset-2', 'asset-3'],
-      });
+      expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
 
       expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalledWith(
         authStub.adminSharedLink.sharedLink?.id,
@@ -665,23 +656,23 @@ describe(AlbumService.name, () => {
 
   describe('removeAssets', () => {
     it('should allow the owner to remove assets', async () => {
-      accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123']));
-      accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id']));
+      accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
+      accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id']));
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
-      albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id']));
+      albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id']));
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: true, id: 'asset-id' },
       ]);
 
       expect(albumMock.update).toHaveBeenCalledWith({ id: 'album-123', updatedAt: expect.any(Date) });
-      expect(albumMock.removeAssets).toHaveBeenCalledWith('album-123', ['asset-id']);
+      expect(albumMock.removeAssetIds).toHaveBeenCalledWith('album-123', ['asset-id']);
     });
 
     it('should skip assets not in the album', async () => {
       accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.empty));
-      albumMock.getAssetIds.mockResolvedValueOnce(new Set());
+      albumMock.getAssetIds.mockResolvedValue(new Set());
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: false, id: 'asset-id', error: BulkIdErrorReason.NOT_FOUND },
@@ -693,7 +684,7 @@ describe(AlbumService.name, () => {
     it('should skip assets without user permission to remove', async () => {
       accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123']));
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
-      albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id']));
+      albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id']));
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         {
@@ -707,10 +698,10 @@ describe(AlbumService.name, () => {
     });
 
     it('should reset the thumbnail if it is removed', async () => {
-      accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123']));
-      accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id']));
+      accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
+      accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id']));
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.twoAssets));
-      albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id']));
+      albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id']));
 
       await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([
         { success: true, id: 'asset-id' },

server/src/services/album.service.ts

@@ -12,7 +12,7 @@ import {
   mapAlbumWithAssets,
   mapAlbumWithoutAssets,
 } from 'src/dtos/album.dto';
-import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto';
+import { BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto';
 import { AuthDto } from 'src/dtos/auth.dto';
 import { AlbumEntity } from 'src/entities/album.entity';
 import { AssetEntity } from 'src/entities/asset.entity';
@@ -21,13 +21,13 @@ import { IAccessRepository } from 'src/interfaces/access.interface';
 import { AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from 'src/interfaces/album.interface';
 import { IAssetRepository } from 'src/interfaces/asset.interface';
 import { IUserRepository } from 'src/interfaces/user.interface';
-import { setUnion } from 'src/utils/set';
+import { addAssets, removeAssets } from 'src/utils/asset.util';
 
 @Injectable()
 export class AlbumService {
   private access: AccessCore;
   constructor(
-    @Inject(IAccessRepository) accessRepository: IAccessRepository,
+    @Inject(IAccessRepository) private accessRepository: IAccessRepository,
     @Inject(IAlbumRepository) private albumRepository: IAlbumRepository,
     @Inject(IAssetRepository) private assetRepository: IAssetRepository,
     @Inject(IUserRepository) private userRepository: IUserRepository,
@@ -164,37 +164,20 @@ export class AlbumService {
 
   async addAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise<BulkIdResponseDto[]> {
     const album = await this.findOrFail(id, { withAssets: false });
-
     await this.access.requirePermission(auth, Permission.ALBUM_READ, id);
 
-    const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids);
-    const notPresentAssetIds = dto.ids.filter((id) => !existingAssetIds.has(id));
-    const allowedAssetIds = await this.access.checkAccess(auth, Permission.ASSET_SHARE, notPresentAssetIds);
+    const results = await addAssets(
+      auth,
+      { accessRepository: this.accessRepository, repository: this.albumRepository },
+      { id, assetIds: dto.ids },
+    );
 
-    const results: BulkIdResponseDto[] = [];
-    for (const assetId of dto.ids) {
-      const hasAsset = existingAssetIds.has(assetId);
-      if (hasAsset) {
-        results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE });
-        continue;
-      }
-
-      const hasAccess = allowedAssetIds.has(assetId);
-      if (!hasAccess) {
-        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
-        continue;
-      }
-
-      results.push({ id: assetId, success: true });
-    }
-
-    const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id);
-    if (newAssetIds.length > 0) {
-      await this.albumRepository.addAssets({ albumId: id, assetIds: newAssetIds });
+    const { id: firstNewAssetId } = results.find(({ success }) => success) || {};
+    if (firstNewAssetId) {
       await this.albumRepository.update({
         id,
         updatedAt: new Date(),
-        albumThumbnailAssetId: album.albumThumbnailAssetId ?? newAssetIds[0],
+        albumThumbnailAssetId: album.albumThumbnailAssetId ?? firstNewAssetId,
       });
     }
 
@@ -206,31 +189,14 @@ export class AlbumService {
 
     await this.access.requirePermission(auth, Permission.ALBUM_READ, id);
 
-    const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids);
-    const canRemove = await this.access.checkAccess(auth, Permission.ALBUM_REMOVE_ASSET, existingAssetIds);
-    const canShare = await this.access.checkAccess(auth, Permission.ASSET_SHARE, existingAssetIds);
-    const allowedAssetIds = setUnion(canRemove, canShare);
-
-    const results: BulkIdResponseDto[] = [];
-    for (const assetId of dto.ids) {
-      const hasAsset = existingAssetIds.has(assetId);
-      if (!hasAsset) {
-        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND });
-        continue;
-      }
-
-      const hasAccess = allowedAssetIds.has(assetId);
-      if (!hasAccess) {
-        results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
-        continue;
-      }
-
-      results.push({ id: assetId, success: true });
-    }
+    const results = await removeAssets(
+      auth,
+      { accessRepository: this.accessRepository, repository: this.albumRepository },
+      { id, assetIds: dto.ids, permissions: [Permission.ASSET_SHARE, Permission.ALBUM_REMOVE_ASSET] },
+    );
 
     const removedIds = results.filter(({ success }) => success).map(({ id }) => id);
     if (removedIds.length > 0) {
-      await this.albumRepository.removeAssets(id, removedIds);
       await this.albumRepository.update({ id, updatedAt: new Date() });
       if (album.albumThumbnailAssetId && removedIds.includes(album.albumThumbnailAssetId)) {
         await this.albumRepository.updateThumbnails();

server/src/utils/asset.util.ts

@@ -0,0 +1,91 @@
+import { AccessCore, Permission } from 'src/cores/access.core';
+import { BulkIdErrorReason, BulkIdResponseDto } from 'src/dtos/asset-ids.response.dto';
+import { AuthDto } from 'src/dtos/auth.dto';
+import { IAccessRepository } from 'src/interfaces/access.interface';
+import { setDifference, setUnion } from 'src/utils/set';
+
+export interface IBulkAsset {
+  getAssetIds: (id: string, assetIds: string[]) => Promise<Set<string>>;
+  addAssetIds: (id: string, assetIds: string[]) => Promise<void>;
+  removeAssetIds: (id: string, assetIds: string[]) => Promise<void>;
+}
+
+export const addAssets = async (
+  auth: AuthDto,
+  repositories: { accessRepository: IAccessRepository; repository: IBulkAsset },
+  dto: { id: string; assetIds: string[] },
+) => {
+  const { accessRepository, repository } = repositories;
+  const access = AccessCore.create(accessRepository);
+
+  const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds);
+  const notPresentAssetIds = dto.assetIds.filter((id) => !existingAssetIds.has(id));
+  const allowedAssetIds = await access.checkAccess(auth, Permission.ASSET_SHARE, notPresentAssetIds);
+
+  const results: BulkIdResponseDto[] = [];
+  for (const assetId of dto.assetIds) {
+    const hasAsset = existingAssetIds.has(assetId);
+    if (hasAsset) {
+      results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE });
+      continue;
+    }
+
+    const hasAccess = allowedAssetIds.has(assetId);
+    if (!hasAccess) {
+      results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
+      continue;
+    }
+
+    results.push({ id: assetId, success: true });
+  }
+
+  const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id);
+  if (newAssetIds.length > 0) {
+    await repository.addAssetIds(dto.id, newAssetIds);
+  }
+
+  return results;
+};
+
+export const removeAssets = async (
+  auth: AuthDto,
+  repositories: { accessRepository: IAccessRepository; repository: IBulkAsset },
+  dto: { id: string; assetIds: string[]; permissions: Permission[] },
+) => {
+  const { accessRepository, repository } = repositories;
+  const access = AccessCore.create(accessRepository);
+
+  const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds);
+  let allowedAssetIds = new Set<string>();
+  let remainingAssetIds = existingAssetIds;
+
+  for (const permission of dto.permissions) {
+    const newAssetIds = await access.checkAccess(auth, permission, setDifference(remainingAssetIds, allowedAssetIds));
+    remainingAssetIds = setDifference(remainingAssetIds, newAssetIds);
+    allowedAssetIds = setUnion(allowedAssetIds, newAssetIds);
+  }
+
+  const results: BulkIdResponseDto[] = [];
+  for (const assetId of dto.assetIds) {
+    const hasAsset = existingAssetIds.has(assetId);
+    if (!hasAsset) {
+      results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND });
+      continue;
+    }
+
+    const hasAccess = allowedAssetIds.has(assetId);
+    if (!hasAccess) {
+      results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION });
+      continue;
+    }
+
+    results.push({ id: assetId, success: true });
+  }
+
+  const removedIds = results.filter(({ success }) => success).map(({ id }) => id);
+  if (removedIds.length > 0) {
+    await repository.removeAssetIds(dto.id, removedIds);
+  }
+
+  return results;
+};

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

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