From 0abbd85134035b04ec3b980661e9b497d63394e0 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Tue, 15 Aug 2023 14:34:02 -0400 Subject: [PATCH] fix(web,server): album share performance (#3698) --- server/src/domain/album/album.repository.ts | 6 ++- server/src/domain/album/album.service.spec.ts | 7 +-- server/src/domain/album/album.service.ts | 24 +++++----- .../infra/repositories/album.repository.ts | 44 ++++++++++--------- .../(user)/albums/[albumId]/+page.svelte | 6 +-- 5 files changed, 47 insertions(+), 40 deletions(-) diff --git a/server/src/domain/album/album.repository.ts b/server/src/domain/album/album.repository.ts index 811b85ec9a..bc6fa37524 100644 --- a/server/src/domain/album/album.repository.ts +++ b/server/src/domain/album/album.repository.ts @@ -7,8 +7,12 @@ export interface AlbumAssetCount { assetCount: number; } +export interface AlbumInfoOptions { + withAssets: boolean; +} + export interface IAlbumRepository { - getById(id: string): Promise; + getById(id: string, options: AlbumInfoOptions): Promise; getByIds(ids: string[]): Promise; getByAssetId(ownerId: string, assetId: string): Promise; hasAsset(id: string, assetId: string): Promise; diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index 463d94d7f4..f9ab153744 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/server/src/domain/album/album.service.spec.ts @@ -364,6 +364,7 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), sharedUsers: [], }); + expect(albumMock.getById).toHaveBeenCalledWith(albumStub.sharedWithUser.id, { withAssets: false }); }); it('should prevent removing a shared user from a not-owned album (shared with auth user)', async () => { @@ -432,7 +433,7 @@ describe(AlbumService.name, () => { await sut.get(authStub.admin, albumStub.oneAsset.id, {}); - expect(albumMock.getById).toHaveBeenCalledWith(albumStub.oneAsset.id); + expect(albumMock.getById).toHaveBeenCalledWith(albumStub.oneAsset.id, { withAssets: true }); expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, albumStub.oneAsset.id); }); @@ -442,7 +443,7 @@ describe(AlbumService.name, () => { await sut.get(authStub.adminSharedLink, 'album-123', {}); - expect(albumMock.getById).toHaveBeenCalledWith('album-123'); + expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true }); expect(accessMock.album.hasSharedLinkAccess).toHaveBeenCalledWith( authStub.adminSharedLink.sharedLinkId, 'album-123', @@ -455,7 +456,7 @@ describe(AlbumService.name, () => { await sut.get(authStub.user1, 'album-123', {}); - expect(albumMock.getById).toHaveBeenCalledWith('album-123'); + expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true }); expect(accessMock.album.hasSharedAlbumAccess).toHaveBeenCalledWith(authStub.user1.id, 'album-123'); }); diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 925b87ac9a..71c11d9357 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -12,7 +12,7 @@ import { mapAlbumWithAssets, mapAlbumWithoutAssets, } from './album-response.dto'; -import { IAlbumRepository } from './album.repository'; +import { AlbumInfoOptions, IAlbumRepository } from './album.repository'; import { AddUsersDto, AlbumInfoDto, CreateAlbumDto, GetAlbumsDto, UpdateAlbumDto } from './dto'; @Injectable() @@ -84,7 +84,7 @@ export class AlbumService { async get(authUser: AuthUserDto, id: string, dto: AlbumInfoDto) { await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); await this.albumRepository.updateThumbnails(); - return mapAlbum(await this.findOrFail(id), !dto.withoutAssets); + return mapAlbum(await this.findOrFail(id, { withAssets: true }), !dto.withoutAssets); } async create(authUser: AuthUserDto, dto: CreateAlbumDto): Promise { @@ -111,7 +111,7 @@ export class AlbumService { async update(authUser: AuthUserDto, id: string, dto: UpdateAlbumDto): Promise { await this.access.requirePermission(authUser, Permission.ALBUM_UPDATE, id); - const album = await this.findOrFail(id); + const album = await this.findOrFail(id, { withAssets: true }); if (dto.albumThumbnailAssetId) { const valid = await this.albumRepository.hasAsset(id, dto.albumThumbnailAssetId); @@ -129,13 +129,13 @@ export class AlbumService { await this.jobRepository.queue({ name: JobName.SEARCH_INDEX_ALBUM, data: { ids: [updatedAlbum.id] } }); - return mapAlbumWithAssets(updatedAlbum); + return mapAlbumWithoutAssets(updatedAlbum); } async delete(authUser: AuthUserDto, id: string): Promise { await this.access.requirePermission(authUser, Permission.ALBUM_DELETE, id); - const album = await this.albumRepository.getById(id); + const album = await this.findOrFail(id, { withAssets: false }); if (!album) { throw new BadRequestException('Album not found'); } @@ -145,7 +145,7 @@ export class AlbumService { } async addAssets(authUser: AuthUserDto, id: string, dto: BulkIdsDto): Promise { - const album = await this.findOrFail(id); + const album = await this.findOrFail(id, { withAssets: true }); await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); @@ -181,7 +181,7 @@ export class AlbumService { } async removeAssets(authUser: AuthUserDto, id: string, dto: BulkIdsDto): Promise { - const album = await this.findOrFail(id); + const album = await this.findOrFail(id, { withAssets: true }); await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); @@ -225,7 +225,7 @@ export class AlbumService { async addUsers(authUser: AuthUserDto, id: string, dto: AddUsersDto): Promise { await this.access.requirePermission(authUser, Permission.ALBUM_SHARE, id); - const album = await this.findOrFail(id); + const album = await this.findOrFail(id, { withAssets: false }); for (const userId of dto.sharedUserIds) { const exists = album.sharedUsers.find((user) => user.id === userId); @@ -247,7 +247,7 @@ export class AlbumService { updatedAt: new Date(), sharedUsers: album.sharedUsers, }) - .then(mapAlbumWithAssets); + .then(mapAlbumWithoutAssets); } async removeUser(authUser: AuthUserDto, id: string, userId: string | 'me'): Promise { @@ -255,7 +255,7 @@ export class AlbumService { userId = authUser.id; } - const album = await this.findOrFail(id); + const album = await this.findOrFail(id, { withAssets: false }); if (album.ownerId === userId) { throw new BadRequestException('Cannot remove album owner'); @@ -278,8 +278,8 @@ export class AlbumService { }); } - private async findOrFail(id: string) { - const album = await this.albumRepository.getById(id); + private async findOrFail(id: string, options: AlbumInfoOptions) { + const album = await this.albumRepository.getById(id, options); if (!album) { throw new BadRequestException('Album not found'); } diff --git a/server/src/infra/repositories/album.repository.ts b/server/src/infra/repositories/album.repository.ts index fcf8c54c1f..5e4ac06ca0 100644 --- a/server/src/infra/repositories/album.repository.ts +++ b/server/src/infra/repositories/album.repository.ts @@ -1,7 +1,7 @@ -import { AlbumAssetCount, IAlbumRepository } from '@app/domain'; +import { AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from '@app/domain'; import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { In, IsNull, Not, Repository } from 'typeorm'; +import { FindOptionsOrder, FindOptionsRelations, In, IsNull, Not, Repository } from 'typeorm'; import { dataSource } from '../database.config'; import { AlbumEntity, AssetEntity } from '../entities'; @@ -12,25 +12,27 @@ export class AlbumRepository implements IAlbumRepository { @InjectRepository(AlbumEntity) private repository: Repository, ) {} - getById(id: string): Promise { - return this.repository.findOne({ - where: { - id, - }, - relations: { - owner: true, - sharedUsers: true, - assets: { - exifInfo: true, - }, - sharedLinks: true, - }, - order: { - assets: { - fileCreatedAt: 'DESC', - }, - }, - }); + getById(id: string, options: AlbumInfoOptions): Promise { + const relations: FindOptionsRelations = { + owner: true, + sharedUsers: true, + assets: false, + sharedLinks: true, + }; + + const order: FindOptionsOrder = {}; + + if (options.withAssets) { + relations.assets = { + exifInfo: true, + }; + + order.assets = { + fileCreatedAt: 'DESC', + }; + } + + return this.repository.findOne({ where: { id }, relations, order }); } getByIds(ids: string[]): Promise { diff --git a/web/src/routes/(user)/albums/[albumId]/+page.svelte b/web/src/routes/(user)/albums/[albumId]/+page.svelte index fb169c3998..8922ab073b 100644 --- a/web/src/routes/(user)/albums/[albumId]/+page.svelte +++ b/web/src/routes/(user)/albums/[albumId]/+page.svelte @@ -100,7 +100,7 @@ }); const refreshAlbum = async () => { - const { data } = await api.albumApi.getAlbumInfo({ id: album.id, withoutAssets: false }); + const { data } = await api.albumApi.getAlbumInfo({ id: album.id, withoutAssets: true }); album = data; }; @@ -261,9 +261,9 @@ } }; - const handleUpdateDescription = (description: string) => { + const handleUpdateDescription = async (description: string) => { try { - api.albumApi.updateAlbumInfo({ + await api.albumApi.updateAlbumInfo({ id: album.id, updateAlbumDto: { description,