refactor(server): album controller (#2539)

* refactor: album controller/service

* chore: open-api

* fix: tests

server/apps/immich/src/api-v1/album/album.controller.ts

@@ -20,124 +20,112 @@ import {
 } from '../../constants/download.constant';
 import { DownloadDto } from '../asset/dto/download-library.dto';
 import { CreateAlbumShareLinkDto as CreateAlbumSharedLinkDto } from './dto/create-album-shared-link.dto';
-import { AlbumIdDto } from './dto/album-id.dto';
 import { UseValidation } from '../../decorators/use-validation.decorator';
+import { UUIDParamDto } from '../../controllers/dto/uuid-param.dto';
+import { DownloadArchive } from '../../modules/download/download.service';
+
+const handleDownload = (download: DownloadArchive, res: Res) => {
+  res.attachment(download.fileName);
+  res.setHeader(IMMICH_CONTENT_LENGTH_HINT, download.fileSize);
+  res.setHeader(IMMICH_ARCHIVE_FILE_COUNT, download.fileCount);
+  res.setHeader(IMMICH_ARCHIVE_COMPLETE, `${download.complete}`);
+  return download.stream;
+};
 
 @ApiTags('Album')
 @Controller('album')
 @UseValidation()
 export class AlbumController {
-  constructor(private readonly albumService: AlbumService) {}
+  constructor(private readonly service: AlbumService) {}
 
   @Authenticated()
   @Get('count-by-user-id')
-  async getAlbumCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise<AlbumCountResponseDto> {
-    return this.albumService.getAlbumCountByUserId(authUser);
+  getAlbumCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise<AlbumCountResponseDto> {
+    return this.service.getCountByUserId(authUser);
   }
 
   @Authenticated()
   @Post()
-  async createAlbum(@GetAuthUser() authUser: AuthUserDto, @Body() createAlbumDto: CreateAlbumDto) {
+  createAlbum(@GetAuthUser() authUser: AuthUserDto, @Body() dto: CreateAlbumDto) {
     // TODO: Handle nonexistent sharedWithUserIds and assetIds.
-    return this.albumService.create(authUser, createAlbumDto);
+    return this.service.create(authUser, dto);
   }
 
   @Authenticated()
-  @Put('/:albumId/users')
-  async addUsersToAlbum(
-    @GetAuthUser() authUser: AuthUserDto,
-    @Body() addUsersDto: AddUsersDto,
-    @Param() { albumId }: AlbumIdDto,
-  ) {
+  @Put(':id/users')
+  addUsersToAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body() dto: AddUsersDto) {
     // TODO: Handle nonexistent sharedUserIds.
-    return this.albumService.addUsersToAlbum(authUser, addUsersDto, albumId);
+    return this.service.addUsers(authUser, id, dto);
   }
 
   @Authenticated({ isShared: true })
-  @Put('/:albumId/assets')
-  async addAssetsToAlbum(
+  @Put(':id/assets')
+  addAssetsToAlbum(
     @GetAuthUser() authUser: AuthUserDto,
-    @Body() addAssetsDto: AddAssetsDto,
-    @Param() { albumId }: AlbumIdDto,
+    @Param() { id }: UUIDParamDto,
+    @Body() dto: AddAssetsDto,
   ): Promise<AddAssetsResponseDto> {
     // TODO: Handle nonexistent assetIds.
     // TODO: Disallow adding assets of another user to an album.
-    return this.albumService.addAssetsToAlbum(authUser, addAssetsDto, albumId);
+    return this.service.addAssets(authUser, id, dto);
   }
 
   @Authenticated({ isShared: true })
-  @Get('/:albumId')
-  async getAlbumInfo(@GetAuthUser() authUser: AuthUserDto, @Param() { albumId }: AlbumIdDto) {
-    return this.albumService.getAlbumInfo(authUser, albumId);
+  @Get(':id')
+  getAlbumInfo(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) {
+    return this.service.get(authUser, id);
   }
 
   @Authenticated()
-  @Delete('/:albumId/assets')
-  async removeAssetFromAlbum(
+  @Delete(':id/assets')
+  removeAssetFromAlbum(
     @GetAuthUser() authUser: AuthUserDto,
-    @Body() removeAssetsDto: RemoveAssetsDto,
-    @Param() { albumId }: AlbumIdDto,
+    @Body() dto: RemoveAssetsDto,
+    @Param() { id }: UUIDParamDto,
   ): Promise<AlbumResponseDto> {
-    return this.albumService.removeAssetsFromAlbum(authUser, removeAssetsDto, albumId);
+    return this.service.removeAssets(authUser, id, dto);
   }
 
   @Authenticated()
-  @Delete('/:albumId')
-  async deleteAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { albumId }: AlbumIdDto) {
-    return this.albumService.deleteAlbum(authUser, albumId);
+  @Delete(':id')
+  deleteAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) {
+    return this.service.delete(authUser, id);
   }
 
   @Authenticated()
-  @Delete('/:albumId/user/:userId')
-  async removeUserFromAlbum(
+  @Delete(':id/user/:userId')
+  removeUserFromAlbum(
     @GetAuthUser() authUser: AuthUserDto,
-    @Param() { albumId }: AlbumIdDto,
+    @Param() { id }: UUIDParamDto,
     @Param('userId', new ParseMeUUIDPipe({ version: '4' })) userId: string,
   ) {
-    return this.albumService.removeUserFromAlbum(authUser, albumId, userId);
+    return this.service.removeUser(authUser, id, userId);
   }
 
   @Authenticated()
-  @Patch('/:albumId')
-  async updateAlbumInfo(
-    @GetAuthUser() authUser: AuthUserDto,
-    @Body() updateAlbumInfoDto: UpdateAlbumDto,
-    @Param() { albumId }: AlbumIdDto,
-  ) {
+  @Patch(':id')
+  updateAlbumInfo(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body() dto: UpdateAlbumDto) {
     // TODO: Handle nonexistent albumThumbnailAssetId.
     // TODO: Disallow setting asset from other user as albumThumbnailAssetId.
-    return this.albumService.updateAlbumInfo(authUser, updateAlbumInfoDto, albumId);
+    return this.service.update(authUser, id, dto);
   }
 
   @Authenticated({ isShared: true })
-  @Get('/:albumId/download')
+  @Get(':id/download')
   @ApiOkResponse({ content: { 'application/zip': { schema: { type: 'string', format: 'binary' } } } })
-  async downloadArchive(
+  downloadArchive(
     @GetAuthUser() authUser: AuthUserDto,
-    @Param() { albumId }: AlbumIdDto,
+    @Param() { id }: UUIDParamDto,
     @Query() dto: DownloadDto,
     @Response({ passthrough: true }) res: Res,
   ) {
-    this.albumService.checkDownloadAccess(authUser);
-
-    const { stream, fileName, fileSize, fileCount, complete } = await this.albumService.downloadArchive(
-      authUser,
-      albumId,
-      dto,
-    );
-    res.attachment(fileName);
-    res.setHeader(IMMICH_CONTENT_LENGTH_HINT, fileSize);
-    res.setHeader(IMMICH_ARCHIVE_FILE_COUNT, fileCount);
-    res.setHeader(IMMICH_ARCHIVE_COMPLETE, `${complete}`);
-    return stream;
+    this.service.checkDownloadAccess(authUser);
+    return this.service.downloadArchive(authUser, id, dto).then((download) => handleDownload(download, res));
   }
 
   @Authenticated()
-  @Post('/create-shared-link')
-  async createAlbumSharedLink(
-    @GetAuthUser() authUser: AuthUserDto,
-    @Body() createAlbumShareLinkDto: CreateAlbumSharedLinkDto,
-  ) {
-    return this.albumService.createAlbumSharedLink(authUser, createAlbumShareLinkDto);
+  @Post('create-shared-link')
+  createAlbumSharedLink(@GetAuthUser() authUser: AuthUserDto, @Body() dto: CreateAlbumSharedLinkDto) {
+    return this.service.createSharedLink(authUser, dto);
   }
 }

server/apps/immich/src/api-v1/album/album.service.spec.ts

@@ -182,14 +182,14 @@ describe('Album service', () => {
       shared: false,
       assetCount: 0,
     };
-    await expect(sut.getAlbumInfo(authUser, albumId)).resolves.toEqual(expectedResult);
+    await expect(sut.get(authUser, albumId)).resolves.toEqual(expectedResult);
   });
 
   it('gets a shared album', async () => {
     const albumEntity = _getSharedWithAuthUserAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
 
-    const result = await sut.getAlbumInfo(authUser, albumId);
+    const result = await sut.get(authUser, albumId);
     expect(result.id).toEqual(albumId);
     expect(result.ownerId).toEqual(sharedAlbumOwnerId);
     expect(result.shared).toEqual(true);
@@ -203,19 +203,19 @@ describe('Album service', () => {
     const albumId = albumEntity.id;
 
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
-    await expect(sut.getAlbumInfo(authUser, albumId)).rejects.toBeInstanceOf(ForbiddenException);
+    await expect(sut.get(authUser, albumId)).rejects.toBeInstanceOf(ForbiddenException);
   });
 
   it('throws a not found exception if the album is not found', async () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve(null));
-    await expect(sut.getAlbumInfo(authUser, '0002')).rejects.toBeInstanceOf(NotFoundException);
+    await expect(sut.get(authUser, '0002')).rejects.toBeInstanceOf(NotFoundException);
   });
 
   it('deletes an owned album', async () => {
     const albumEntity = _getOwnedAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.delete.mockImplementation(() => Promise.resolve());
-    await sut.deleteAlbum(authUser, albumId);
+    await sut.delete(authUser, albumId);
     expect(albumRepositoryMock.delete).toHaveBeenCalledTimes(1);
     expect(albumRepositoryMock.delete).toHaveBeenCalledWith(albumEntity);
   });
@@ -223,14 +223,14 @@ describe('Album service', () => {
   it('prevents deleting a shared album (shared with auth user)', async () => {
     const albumEntity = _getSharedWithAuthUserAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
-    await expect(sut.deleteAlbum(authUser, albumId)).rejects.toBeInstanceOf(ForbiddenException);
+    await expect(sut.delete(authUser, albumId)).rejects.toBeInstanceOf(ForbiddenException);
   });
 
   it('removes a shared user from an owned album', async () => {
     const albumEntity = _getOwnedSharedAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve());
-    await expect(sut.removeUserFromAlbum(authUser, albumEntity.id, ownedAlbumSharedWithId)).resolves.toBeUndefined();
+    await expect(sut.removeUser(authUser, albumEntity.id, ownedAlbumSharedWithId)).resolves.toBeUndefined();
     expect(albumRepositoryMock.removeUser).toHaveBeenCalledTimes(1);
     expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, ownedAlbumSharedWithId);
   });
@@ -242,7 +242,7 @@ describe('Album service', () => {
 
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
 
-    await expect(sut.removeUserFromAlbum(authUser, albumId, userIdToRemove)).rejects.toBeInstanceOf(ForbiddenException);
+    await expect(sut.removeUser(authUser, albumId, userIdToRemove)).rejects.toBeInstanceOf(ForbiddenException);
     expect(albumRepositoryMock.removeUser).not.toHaveBeenCalled();
   });
 
@@ -251,7 +251,7 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve());
 
-    await sut.removeUserFromAlbum(authUser, albumEntity.id, authUser.id);
+    await sut.removeUser(authUser, albumEntity.id, authUser.id);
     expect(albumRepositoryMock.removeUser).toHaveReturnedTimes(1);
     expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, authUser.id);
   });
@@ -261,7 +261,7 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve());
 
-    await sut.removeUserFromAlbum(authUser, albumEntity.id, 'me');
+    await sut.removeUser(authUser, albumEntity.id, 'me');
     expect(albumRepositoryMock.removeUser).toHaveReturnedTimes(1);
     expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, authUser.id);
   });
@@ -270,9 +270,7 @@ describe('Album service', () => {
     const albumEntity = _getOwnedAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
 
-    await expect(sut.removeUserFromAlbum(authUser, albumEntity.id, authUser.id)).rejects.toBeInstanceOf(
-      BadRequestException,
-    );
+    await expect(sut.removeUser(authUser, albumEntity.id, authUser.id)).rejects.toBeInstanceOf(BadRequestException);
   });
 
   it('updates a owned album', async () => {
@@ -284,14 +282,10 @@ describe('Album service', () => {
     const updatedAlbum = { ...albumEntity, albumName: updatedAlbumName };
     albumRepositoryMock.updateAlbum.mockResolvedValue(updatedAlbum);
 
-    const result = await sut.updateAlbumInfo(
-      authUser,
-      {
-        albumName: updatedAlbumName,
-        albumThumbnailAssetId: updatedAlbumThumbnailAssetId,
-      },
-      albumId,
-    );
+    const result = await sut.update(authUser, albumId, {
+      albumName: updatedAlbumName,
+      albumThumbnailAssetId: updatedAlbumThumbnailAssetId,
+    });
 
     expect(result.id).toEqual(albumId);
     expect(result.albumName).toEqual(updatedAlbumName);
@@ -310,14 +304,10 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
 
     await expect(
-      sut.updateAlbumInfo(
-        authUser,
-        {
-          albumName: 'new album name',
-          albumThumbnailAssetId: '69d2f917-0b31-48d8-9d7d-673b523f1aac',
-        },
-        albumId,
-      ),
+      sut.update(authUser, albumId, {
+        albumName: 'new album name',
+        albumThumbnailAssetId: '69d2f917-0b31-48d8-9d7d-673b523f1aac',
+      }),
     ).rejects.toBeInstanceOf(ForbiddenException);
   });
 
@@ -334,13 +324,7 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.addAssets.mockImplementation(() => Promise.resolve<AddAssetsResponseDto>(albumResponse));
 
-    const result = (await sut.addAssetsToAlbum(
-      authUser,
-      {
-        assetIds: ['1'],
-      },
-      albumId,
-    )) as AddAssetsResponseDto;
+    const result = (await sut.addAssets(authUser, albumId, { assetIds: ['1'] })) as AddAssetsResponseDto;
 
     // TODO: stub and expect album rendered
     expect(result.album?.id).toEqual(albumId);
@@ -359,13 +343,7 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.addAssets.mockImplementation(() => Promise.resolve<AddAssetsResponseDto>(albumResponse));
 
-    const result = (await sut.addAssetsToAlbum(
-      authUser,
-      {
-        assetIds: ['1'],
-      },
-      albumId,
-    )) as AddAssetsResponseDto;
+    const result = (await sut.addAssets(authUser, albumId, { assetIds: ['1'] })) as AddAssetsResponseDto;
 
     // TODO: stub and expect album rendered
     expect(result.album?.id).toEqual(albumId);
@@ -384,15 +362,7 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.addAssets.mockImplementation(() => Promise.resolve<AddAssetsResponseDto>(albumResponse));
 
-    await expect(
-      sut.addAssetsToAlbum(
-        authUser,
-        {
-          assetIds: ['1'],
-        },
-        albumId,
-      ),
-    ).rejects.toBeInstanceOf(ForbiddenException);
+    await expect(sut.addAssets(authUser, albumId, { assetIds: ['1'] })).rejects.toBeInstanceOf(ForbiddenException);
   });
 
   // it('removes assets from owned album', async () => {
@@ -448,14 +418,6 @@ describe('Album service', () => {
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
     albumRepositoryMock.addAssets.mockImplementation(() => Promise.resolve<AddAssetsResponseDto>(albumResponse));
 
-    await expect(
-      sut.removeAssetsFromAlbum(
-        authUser,
-        {
-          assetIds: ['1'],
-        },
-        albumId,
-      ),
-    ).rejects.toBeInstanceOf(ForbiddenException);
+    await expect(sut.removeAssets(authUser, albumId, { assetIds: ['1'] })).rejects.toBeInstanceOf(ForbiddenException);
   });
 });

server/apps/immich/src/api-v1/album/album.service.ts

@@ -61,18 +61,18 @@ export class AlbumService {
     return mapAlbum(albumEntity);
   }
 
-  async getAlbumInfo(authUser: AuthUserDto, albumId: string): Promise<AlbumResponseDto> {
+  async get(authUser: AuthUserDto, albumId: string): Promise<AlbumResponseDto> {
     const album = await this._getAlbum({ authUser, albumId, validateIsOwner: false });
     return mapAlbum(album);
   }
 
-  async addUsersToAlbum(authUser: AuthUserDto, addUsersDto: AddUsersDto, albumId: string): Promise<AlbumResponseDto> {
+  async addUsers(authUser: AuthUserDto, albumId: string, dto: AddUsersDto): Promise<AlbumResponseDto> {
     const album = await this._getAlbum({ authUser, albumId });
-    const updatedAlbum = await this.albumRepository.addSharedUsers(album, addUsersDto);
+    const updatedAlbum = await this.albumRepository.addSharedUsers(album, dto);
     return mapAlbum(updatedAlbum);
   }
 
-  async deleteAlbum(authUser: AuthUserDto, albumId: string): Promise<void> {
+  async delete(authUser: AuthUserDto, albumId: string): Promise<void> {
     const album = await this._getAlbum({ authUser, albumId });
 
     for (const sharedLink of album.sharedLinks) {
@@ -83,7 +83,7 @@ export class AlbumService {
     await this.jobRepository.queue({ name: JobName.SEARCH_REMOVE_ALBUM, data: { ids: [albumId] } });
   }
 
-  async removeUserFromAlbum(authUser: AuthUserDto, albumId: string, userId: string | 'me'): Promise<void> {
+  async removeUser(authUser: AuthUserDto, albumId: string, userId: string | 'me'): Promise<void> {
     const sharedUserId = userId == 'me' ? authUser.id : userId;
     const album = await this._getAlbum({ authUser, albumId, validateIsOwner: false });
     if (album.ownerId != authUser.id && authUser.id != sharedUserId) {
@@ -95,34 +95,26 @@ export class AlbumService {
     await this.albumRepository.removeUser(album, sharedUserId);
   }
 
-  async removeAssetsFromAlbum(
-    authUser: AuthUserDto,
-    removeAssetsDto: RemoveAssetsDto,
-    albumId: string,
-  ): Promise<AlbumResponseDto> {
+  async removeAssets(authUser: AuthUserDto, albumId: string, dto: RemoveAssetsDto): Promise<AlbumResponseDto> {
     const album = await this._getAlbum({ authUser, albumId });
-    const deletedCount = await this.albumRepository.removeAssets(album, removeAssetsDto);
+    const deletedCount = await this.albumRepository.removeAssets(album, dto);
     const newAlbum = await this._getAlbum({ authUser, albumId });
 
-    if (deletedCount !== removeAssetsDto.assetIds.length) {
+    if (deletedCount !== dto.assetIds.length) {
       throw new BadRequestException('Some assets were not found in the album');
     }
 
     return mapAlbum(newAlbum);
   }
 
-  async addAssetsToAlbum(
-    authUser: AuthUserDto,
-    addAssetsDto: AddAssetsDto,
-    albumId: string,
-  ): Promise<AddAssetsResponseDto> {
+  async addAssets(authUser: AuthUserDto, albumId: string, dto: AddAssetsDto): Promise<AddAssetsResponseDto> {
     if (authUser.isPublicUser && !authUser.isAllowUpload) {
       this.logger.warn('Deny public user attempt to add asset to album');
       throw new ForbiddenException('Public user is not allowed to upload');
     }
 
     const album = await this._getAlbum({ authUser, albumId, validateIsOwner: false });
-    const result = await this.albumRepository.addAssets(album, addAssetsDto);
+    const result = await this.albumRepository.addAssets(album, dto);
     const newAlbum = await this._getAlbum({ authUser, albumId, validateIsOwner: false });
 
     return {
@@ -131,25 +123,21 @@ export class AlbumService {
     };
   }
 
-  async updateAlbumInfo(
-    authUser: AuthUserDto,
-    updateAlbumDto: UpdateAlbumDto,
-    albumId: string,
-  ): Promise<AlbumResponseDto> {
+  async update(authUser: AuthUserDto, albumId: string, dto: UpdateAlbumDto): Promise<AlbumResponseDto> {
     const album = await this._getAlbum({ authUser, albumId });
 
     if (authUser.id != album.ownerId) {
       throw new BadRequestException('Unauthorized to change album info');
     }
 
-    const updatedAlbum = await this.albumRepository.updateAlbum(album, updateAlbumDto);
+    const updatedAlbum = await this.albumRepository.updateAlbum(album, dto);
 
     await this.jobRepository.queue({ name: JobName.SEARCH_INDEX_ALBUM, data: { ids: [updatedAlbum.id] } });
 
     return mapAlbum(updatedAlbum);
   }
 
-  async getAlbumCountByUserId(authUser: AuthUserDto): Promise<AlbumCountResponseDto> {
+  async getCountByUserId(authUser: AuthUserDto): Promise<AlbumCountResponseDto> {
     return this.albumRepository.getCountByUserId(authUser.id);
   }
 
@@ -160,7 +148,7 @@ export class AlbumService {
     return this.downloadService.downloadArchive(album.albumName, assets);
   }
 
-  async createAlbumSharedLink(authUser: AuthUserDto, dto: CreateAlbumShareLinkDto): Promise<SharedLinkResponseDto> {
+  async createSharedLink(authUser: AuthUserDto, dto: CreateAlbumShareLinkDto): Promise<SharedLinkResponseDto> {
     const album = await this._getAlbum({ authUser, albumId: dto.albumId });
 
     const sharedLink = await this.shareCore.create(authUser.id, {

server/apps/immich/src/api-v1/album/dto/album-id.dto.ts

@@ -1,6 +0,0 @@
-import { ValidateUUID } from 'apps/immich/src/decorators/validate-uuid.decorator';
-
-export class AlbumIdDto {
-  @ValidateUUID()
-  albumId!: string;
-}