From bdf35b668810459c424829574fea75419af5a6bd Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Sat, 4 Mar 2023 15:16:48 +0100 Subject: [PATCH] feat(server): improve thumbnail relation and updating (#1897) * feat(server): improve thumbnail relation and updating * improve query + update tests and migration * make sure uuids are valid in migration * fix unit test --- .../src/api-v1/album/album-repository.ts | 48 ++++++++++++++- .../immich/src/api-v1/album/album.module.ts | 4 +- .../src/api-v1/album/album.service.spec.ts | 49 ++++++--------- .../immich/src/api-v1/album/album.service.ts | 30 ++-------- server/libs/domain/test/fixtures.ts | 2 + .../infra/src/db/entities/album.entity.ts | 5 +- .../1677613712565-AlbumThumbnailRelation.ts | 60 +++++++++++++++++++ 7 files changed, 138 insertions(+), 60 deletions(-) create mode 100644 server/libs/infra/src/db/migrations/1677613712565-AlbumThumbnailRelation.ts diff --git a/server/apps/immich/src/api-v1/album/album-repository.ts b/server/apps/immich/src/api-v1/album/album-repository.ts index c76691b6b..1fbf9279b 100644 --- a/server/apps/immich/src/api-v1/album/album-repository.ts +++ b/server/apps/immich/src/api-v1/album/album-repository.ts @@ -1,4 +1,4 @@ -import { AlbumEntity, AssetEntity, UserEntity } from '@app/infra'; +import { AlbumEntity, AssetEntity, dataSource, UserEntity } from '@app/infra'; import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository, Not, IsNull, FindManyOptions } from 'typeorm'; @@ -22,6 +22,7 @@ export interface IAlbumRepository { removeAssets(album: AlbumEntity, removeAssets: RemoveAssetsDto): Promise; addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise; updateAlbum(album: AlbumEntity, updateAlbumDto: UpdateAlbumDto): Promise; + updateThumbnails(): Promise; getListByAssetId(userId: string, assetId: string): Promise; getCountByUserId(userId: string): Promise; getSharedWithUserAlbumCount(userId: string, assetId: string): Promise; @@ -34,6 +35,9 @@ export class AlbumRepository implements IAlbumRepository { constructor( @InjectRepository(AlbumEntity) private albumRepository: Repository, + + @InjectRepository(AssetEntity) + private assetRepository: Repository, ) {} async getPublicSharingList(ownerId: string): Promise { @@ -208,6 +212,48 @@ export class AlbumRepository implements IAlbumRepository { return this.albumRepository.save(album); } + /** + * Makes sure all thumbnails for albums are updated by: + * - Removing thumbnails from albums without assets + * - Removing references of thumbnails to assets outside the album + * - Setting a thumbnail when none is set and the album contains assets + * + * @returns Amount of updated album thumbnails or undefined when unknown + */ + async updateThumbnails(): Promise { + // Subquery for getting a new thumbnail. + const newThumbnail = this.assetRepository + .createQueryBuilder('assets') + .select('albums_assets2.assetsId') + .addFrom('albums_assets_assets', 'albums_assets2') + .where('albums_assets2.assetsId = assets.id') + .andWhere('albums_assets2.albumsId = "albums"."id"') // Reference to albums.id outside this query + .orderBy('assets.fileCreatedAt', 'DESC') + .limit(1); + + // Using dataSource, because there is no direct access to albums_assets_assets. + const albumHasAssets = dataSource + .createQueryBuilder() + .select('1') + .from('albums_assets_assets', 'albums_assets') + .where('"albums"."id" = "albums_assets"."albumsId"'); + + const albumContainsThumbnail = albumHasAssets + .clone() + .andWhere('"albums"."albumThumbnailAssetId" = "albums_assets"."assetsId"'); + + const updateAlbums = this.albumRepository + .createQueryBuilder('albums') + .update(AlbumEntity) + .set({ albumThumbnailAssetId: () => `(${newThumbnail.getQuery()})` }) + .where(`"albums"."albumThumbnailAssetId" IS NULL AND EXISTS (${albumHasAssets.getQuery()})`) + .orWhere(`"albums"."albumThumbnailAssetId" IS NOT NULL AND NOT EXISTS (${albumContainsThumbnail.getQuery()})`); + + const result = await updateAlbums.execute(); + + return result.affected; + } + async getSharedWithUserAlbumCount(userId: string, assetId: string): Promise { return this.albumRepository.count({ where: [ diff --git a/server/apps/immich/src/api-v1/album/album.module.ts b/server/apps/immich/src/api-v1/album/album.module.ts index 5b152d38d..1e66fd2b2 100644 --- a/server/apps/immich/src/api-v1/album/album.module.ts +++ b/server/apps/immich/src/api-v1/album/album.module.ts @@ -2,7 +2,7 @@ import { Module } from '@nestjs/common'; import { AlbumService } from './album.service'; import { AlbumController } from './album.controller'; import { TypeOrmModule } from '@nestjs/typeorm'; -import { AlbumEntity } from '@app/infra'; +import { AlbumEntity, AssetEntity } from '@app/infra'; import { AlbumRepository, IAlbumRepository } from './album-repository'; import { DownloadModule } from '../../modules/download/download.module'; @@ -12,7 +12,7 @@ const ALBUM_REPOSITORY_PROVIDER = { }; @Module({ - imports: [TypeOrmModule.forFeature([AlbumEntity]), DownloadModule], + imports: [TypeOrmModule.forFeature([AlbumEntity, AssetEntity]), DownloadModule], controllers: [AlbumController], providers: [AlbumService, ALBUM_REPOSITORY_PROVIDER], exports: [ALBUM_REPOSITORY_PROVIDER], diff --git a/server/apps/immich/src/api-v1/album/album.service.spec.ts b/server/apps/immich/src/api-v1/album/album.service.spec.ts index a469940bf..b347d8569 100644 --- a/server/apps/immich/src/api-v1/album/album.service.spec.ts +++ b/server/apps/immich/src/api-v1/album/album.service.spec.ts @@ -129,6 +129,7 @@ describe('Album service', () => { removeAssets: jest.fn(), removeUser: jest.fn(), updateAlbum: jest.fn(), + updateThumbnails: jest.fn(), getListByAssetId: jest.fn(), getCountByUserId: jest.fn(), getSharedWithUserAlbumCount: jest.fn(), @@ -502,59 +503,47 @@ describe('Album service', () => { it('updates the album thumbnail by listing all albums', async () => { const albumEntity = _getOwnedAlbum(); const assetEntity = new AssetEntity(); - const newThumbnailAssetId = 'e5e65c02-b889-4f3c-afe1-a39a96d578ed'; + const newThumbnailAsset = new AssetEntity(); + newThumbnailAsset.id = 'e5e65c02-b889-4f3c-afe1-a39a96d578ed'; albumEntity.albumThumbnailAssetId = 'nonexistent'; - assetEntity.id = newThumbnailAssetId; + assetEntity.id = newThumbnailAsset.id; albumEntity.assets = [assetEntity]; albumRepositoryMock.getList.mockImplementation(async () => [albumEntity]); - albumRepositoryMock.updateAlbum.mockImplementation(async () => ({ - ...albumEntity, - albumThumbnailAssetId: newThumbnailAssetId, - })); + albumRepositoryMock.updateThumbnails.mockImplementation(async () => { + albumEntity.albumThumbnailAsset = newThumbnailAsset; + albumEntity.albumThumbnailAssetId = newThumbnailAsset.id; + + return 1; + }); const result = await sut.getAllAlbums(authUser, {}); expect(result).toHaveLength(1); - expect(result[0].albumThumbnailAssetId).toEqual(newThumbnailAssetId); + expect(result[0].albumThumbnailAssetId).toEqual(newThumbnailAsset.id); expect(albumRepositoryMock.getList).toHaveBeenCalledTimes(1); - expect(albumRepositoryMock.updateAlbum).toHaveBeenCalledTimes(1); + expect(albumRepositoryMock.updateThumbnails).toHaveBeenCalledTimes(1); expect(albumRepositoryMock.getList).toHaveBeenCalledWith(albumEntity.ownerId, {}); - expect(albumRepositoryMock.updateAlbum).toHaveBeenCalledWith(albumEntity, { - albumThumbnailAssetId: newThumbnailAssetId, - }); }); it('removes the thumbnail for an empty album', async () => { const albumEntity = _getOwnedAlbum(); - const newAlbumEntity = { ...albumEntity, albumThumbnailAssetId: null }; albumEntity.albumThumbnailAssetId = 'e5e65c02-b889-4f3c-afe1-a39a96d578ed'; albumRepositoryMock.getList.mockImplementation(async () => [albumEntity]); - albumRepositoryMock.updateAlbum.mockImplementation(async () => newAlbumEntity); + albumRepositoryMock.updateThumbnails.mockImplementation(async () => { + albumEntity.albumThumbnailAsset = null; + albumEntity.albumThumbnailAssetId = null; + + return 1; + }); const result = await sut.getAllAlbums(authUser, {}); expect(result).toHaveLength(1); expect(result[0].albumThumbnailAssetId).toBeNull(); expect(albumRepositoryMock.getList).toHaveBeenCalledTimes(1); - expect(albumRepositoryMock.updateAlbum).toHaveBeenCalledTimes(1); - expect(albumRepositoryMock.getList).toHaveBeenCalledWith(albumEntity.ownerId, {}); - expect(albumRepositoryMock.updateAlbum).toHaveBeenCalledWith(newAlbumEntity, { - albumThumbnailAssetId: undefined, - }); - }); - - it('listing empty albums does not unnecessarily update the album', async () => { - const albumEntity = _getOwnedAlbum(); - albumRepositoryMock.getList.mockImplementation(async () => [albumEntity]); - albumRepositoryMock.updateAlbum.mockImplementation(async () => albumEntity); - - const result = await sut.getAllAlbums(authUser, {}); - - expect(result).toHaveLength(1); - expect(albumRepositoryMock.getList).toHaveBeenCalledTimes(1); - expect(albumRepositoryMock.updateAlbum).toHaveBeenCalledTimes(0); + expect(albumRepositoryMock.updateThumbnails).toHaveBeenCalledTimes(1); expect(albumRepositoryMock.getList).toHaveBeenCalledWith(albumEntity.ownerId, {}); }); }); diff --git a/server/apps/immich/src/api-v1/album/album.service.ts b/server/apps/immich/src/api-v1/album/album.service.ts index 3fa51b9a4..999b19898 100644 --- a/server/apps/immich/src/api-v1/album/album.service.ts +++ b/server/apps/immich/src/api-v1/album/album.service.ts @@ -41,6 +41,8 @@ export class AlbumService { albumId: string; validateIsOwner?: boolean; }): Promise { + await this.albumRepository.updateThumbnails(); + const album = await this.albumRepository.get(albumId); if (!album) { throw new NotFoundException('Album Not Found'); @@ -67,6 +69,8 @@ export class AlbumService { * @returns All Shared Album And Its Members */ async getAllAlbums(authUser: AuthUserDto, getAlbumsDto: GetAlbumsDto): Promise { + await this.albumRepository.updateThumbnails(); + let albums: AlbumEntity[]; if (typeof getAlbumsDto.assetId === 'string') { albums = await this.albumRepository.getListByAssetId(authUser.id, getAlbumsDto.assetId); @@ -81,10 +85,6 @@ export class AlbumService { albums = _.uniqBy(albums, (album) => album.id); - for (const album of albums) { - await this._checkValidThumbnail(album); - } - return albums.map((album) => mapAlbumExcludeAssetInfo(album)); } @@ -131,10 +131,6 @@ export class AlbumService { const deletedCount = await this.albumRepository.removeAssets(album, removeAssetsDto); const newAlbum = await this._getAlbum({ authUser, albumId }); - if (newAlbum) { - await this._checkValidThumbnail(newAlbum); - } - if (deletedCount !== removeAssetsDto.assetIds.length) { throw new BadRequestException('Some assets were not found in the album'); } @@ -191,24 +187,6 @@ export class AlbumService { return this.downloadService.downloadArchive(album.albumName, assets); } - async _checkValidThumbnail(album: AlbumEntity) { - const assets = album.assets || []; - - // Check if the album's thumbnail is invalid by referencing - // an asset outside the album. - const invalid = assets.length > 0 && !assets.some((asset) => asset.id === album.albumThumbnailAssetId); - - // Check if an empty album still has a thumbnail. - const isEmptyWithThumbnail = assets.length === 0 && album.albumThumbnailAssetId !== null; - - if (invalid || isEmptyWithThumbnail) { - const albumThumbnailAssetId = assets[0]?.id; - - album.albumThumbnailAssetId = albumThumbnailAssetId || null; - await this.albumRepository.updateAlbum(album, { albumThumbnailAssetId }); - } - } - async createAlbumSharedLink(authUser: AuthUserDto, dto: CreateAlbumShareLinkDto): Promise { const album = await this._getAlbum({ authUser, albumId: dto.albumId }); diff --git a/server/libs/domain/test/fixtures.ts b/server/libs/domain/test/fixtures.ts index da2aa42a5..78f0ee03f 100644 --- a/server/libs/domain/test/fixtures.ts +++ b/server/libs/domain/test/fixtures.ts @@ -163,6 +163,7 @@ export const albumStub = { ownerId: authStub.admin.id, owner: userEntityStub.admin, assets: [], + albumThumbnailAsset: null, albumThumbnailAssetId: null, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), @@ -422,6 +423,7 @@ export const sharedLinkStub = { albumName: 'Test Album', createdAt: today.toISOString(), updatedAt: today.toISOString(), + albumThumbnailAsset: null, albumThumbnailAssetId: null, sharedUsers: [], sharedLinks: [], diff --git a/server/libs/infra/src/db/entities/album.entity.ts b/server/libs/infra/src/db/entities/album.entity.ts index 565fdf217..a785a939d 100644 --- a/server/libs/infra/src/db/entities/album.entity.ts +++ b/server/libs/infra/src/db/entities/album.entity.ts @@ -33,7 +33,10 @@ export class AlbumEntity { @UpdateDateColumn({ type: 'timestamptz' }) updatedAt!: string; - @Column({ comment: 'Asset ID to be used as thumbnail', type: 'varchar', nullable: true }) + @ManyToOne(() => AssetEntity, { nullable: true, onDelete: 'SET NULL', onUpdate: 'CASCADE' }) + albumThumbnailAsset!: AssetEntity | null; + + @Column({ comment: 'Asset ID to be used as thumbnail', nullable: true }) albumThumbnailAssetId!: string | null; @ManyToMany(() => UserEntity) diff --git a/server/libs/infra/src/db/migrations/1677613712565-AlbumThumbnailRelation.ts b/server/libs/infra/src/db/migrations/1677613712565-AlbumThumbnailRelation.ts new file mode 100644 index 000000000..71f022dcf --- /dev/null +++ b/server/libs/infra/src/db/migrations/1677613712565-AlbumThumbnailRelation.ts @@ -0,0 +1,60 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AlbumThumbnailRelation1677613712565 implements MigrationInterface { + name = 'AlbumThumbnailRelation1677613712565'; + + public async up(queryRunner: QueryRunner): Promise { + // Make sure all albums have a valid albumThumbnailAssetId UUID or NULL. + await queryRunner.query(` + UPDATE "albums" + SET + "albumThumbnailAssetId" = ( + SELECT + "albums_assets2"."assetsId" + FROM + "assets" "assets", + "albums_assets_assets" "albums_assets2" + WHERE + "albums_assets2"."assetsId" = "assets"."id" + AND "albums_assets2"."albumsId" = "albums"."id" + ORDER BY + "assets"."fileCreatedAt" DESC + LIMIT 1 + ), + "updatedAt" = CURRENT_TIMESTAMP + WHERE + "albums"."albumThumbnailAssetId" IS NULL + AND EXISTS ( + SELECT 1 + FROM "albums_assets_assets" "albums_assets" + WHERE "albums"."id" = "albums_assets"."albumsId" + ) + OR "albums"."albumThumbnailAssetId" IS NOT NULL + AND NOT EXISTS ( + SELECT 1 + FROM "albums_assets_assets" "albums_assets" + WHERE + "albums"."id" = "albums_assets"."albumsId" + AND "albums"."albumThumbnailAssetId" = "albums_assets"."assetsId"::varchar + ) + `); + + await queryRunner.query(` + ALTER TABLE "albums" + ALTER COLUMN "albumThumbnailAssetId" + TYPE uuid USING "albumThumbnailAssetId"::uuid + `); + + await queryRunner.query(` + ALTER TABLE "albums" ADD CONSTRAINT "FK_05895aa505a670300d4816debce" FOREIGN KEY ("albumThumbnailAssetId") REFERENCES "assets"("id") ON DELETE SET NULL ON UPDATE CASCADE + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "albums" DROP CONSTRAINT "FK_05895aa505a670300d4816debce"`); + + await queryRunner.query(` + ALTER TABLE "albums" ALTER COLUMN "albumThumbnailAssetId" TYPE varchar USING "albumThumbnailAssetId"::varchar + `); + } +}