From 0994575bf3c240ab105108826831502e182f2bec Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 18 Oct 2023 11:56:00 -0400 Subject: [PATCH] fix(server): album add/remove asset performance (#4516) --- server/src/domain/album/album.service.spec.ts | 46 ++++++++------ server/src/domain/album/album.service.ts | 60 +++++++++---------- .../domain/repositories/album.repository.ts | 17 +++++- .../infra/repositories/album.repository.ts | 30 ++++++++-- .../repositories/album.repository.mock.ts | 2 + 5 files changed, 97 insertions(+), 58 deletions(-) diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index e326a27119..453539129d 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/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({ - id: 'album-123', - updatedAt: expect.any(Date), - assets: [], - albumThumbnailAssetId: null, - }); + expect(albumMock.update).toHaveBeenCalledWith({ id: 'album-123', updatedAt: expect.any(Date) }); + expect(albumMock.removeAssets).toHaveBeenCalledWith({ assetIds: ['asset-id'], albumId: 'album-123' }); }); 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(); }); }); diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 5da0b3440d..04b885040a 100644 --- a/server/src/domain/album/album.service.ts +++ b/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 { - 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) { - const hasAsset = album.assets.find((asset) => asset.id === id); + for (const assetId of dto.ids) { + 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 }); - album.assets.push({ id } as AssetEntity); + results.push({ id: assetId, success: true }); } - const newAsset = results.find(({ success }) => success); - if (newAsset) { + const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id); + 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 { - 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) { - const hasAsset = album.assets.find((asset) => asset.id === id); + for (const assetId of dto.ids) { + 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.ASSET_SHARE, id }, + { permission: Permission.ALBUM_REMOVE_ASSET, id: assetId }, + { 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 }); - album.assets = album.assets.filter((asset) => asset.id !== id); - if (album.albumThumbnailAssetId === id) { - album.albumThumbnailAssetId = null; - } + results.push({ id: assetId, success: true }); } - const hasSuccess = results.find(({ success }) => success); - if (hasSuccess) { - await this.albumRepository.update({ - id, - assets: album.assets, - updatedAt: new Date(), - albumThumbnailAssetId: album.albumThumbnailAssetId || album.assets[0]?.id || null, - }); + const removedIds = results.filter(({ success }) => success).map(({ id }) => id); + if (removedIds.length > 0) { + await this.albumRepository.removeAssets({ albumId: id, assetIds: removedIds }); + await this.albumRepository.update({ id, updatedAt: new Date() }); + if (album.albumThumbnailAssetId && removedIds.includes(album.albumThumbnailAssetId)) { + await this.albumRepository.updateThumbnails(); + } } return results; diff --git a/server/src/domain/repositories/album.repository.ts b/server/src/domain/repositories/album.repository.ts index 5a54dbc80f..276ab796bd 100644 --- a/server/src/domain/repositories/album.repository.ts +++ b/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; getByIds(ids: string[]): Promise; getByAssetId(ownerId: string, assetId: string): Promise; - hasAsset(id: string, assetId: string): Promise; - /** Remove an asset from _all_ albums */ - removeAsset(id: string): Promise; + addAssets(assets: AlbumAssets): Promise; + hasAsset(asset: AlbumAsset): Promise; + removeAsset(assetId: string): Promise; + removeAssets(assets: AlbumAssets): Promise; getAssetCountForIds(ids: string[]): Promise; getInvalidThumbnail(): Promise; getOwned(ownerId: string): Promise; diff --git a/server/src/infra/repositories/album.repository.ts b/server/src/infra/repositories/album.repository.ts index b53c934713..a8cd504146 100644 --- a/server/src/infra/repositories/album.repository.ts +++ b/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 { + await this.dataSource + .createQueryBuilder() + .delete() + .from('albums_assets_assets') + .where({ + albumsId: asset.albumId, + assetsId: In(asset.assetIds), + }) .execute(); } - hasAsset(id: string, assetId: string): Promise { + hasAsset(asset: AlbumAsset): Promise { 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 { + await this.dataSource + .createQueryBuilder() + .insert() + .into('albums_assets_assets', ['albumsId', 'assetsId']) + .values(assetIds.map((assetId) => ({ albumsId: albumId, assetsId: assetId }))) + .execute(); + } + async create(album: Partial): Promise { return this.save(album); } diff --git a/server/test/repositories/album.repository.mock.ts b/server/test/repositories/album.repository.mock.ts index 25206f0280..20c3552699 100644 --- a/server/test/repositories/album.repository.mock.ts +++ b/server/test/repositories/album.repository.mock.ts @@ -14,7 +14,9 @@ export const newAlbumRepositoryMock = (): jest.Mocked => { 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(),