From 1978b77a40d027c5b30859dc72cb4ed4a9b0e2b5 Mon Sep 17 00:00:00 2001 From: bo0tzz Date: Wed, 14 Oct 2026 00:36:20 +0200 Subject: [PATCH] perf(server): stop loading every album asset to build album responses (#32413) * perf(server): stop loading every album asset to build album responses Album create, update (rename, description, cover, sort order, activity toggle) and add-users json_agg'd every asset of the album with its exif row, only for mapAlbum to read the count and the first/last localDateTime. Use getMetadataForIds for those fields, as GET /albums/:id already does, and drop the now unused withAssets option from getById. The responses now carry the same date format and lastModifiedAssetTimestamp as GET /albums/:id. PATCH /albums/:id on a 150k-asset album, PG 14 (response is ~900 B either way): 3.4-3.6 s -> 0.13-0.16 s. * refactor(server): share album metadata mapping (cherry picked from commit 2a4e876ec990a4a8d5fdca00291cbeebd3d1bccf) --- server/src/queries/album.repository.sql | 44 +---------------- server/src/repositories/album.repository.ts | 45 ++--------------- server/src/services/album.service.spec.ts | 9 ++-- server/src/services/album.service.ts | 55 +++++++++++---------- server/src/services/asset-media.service.ts | 2 +- server/src/services/notification.service.ts | 4 +- 6 files changed, 43 insertions(+), 116 deletions(-) diff --git a/server/src/queries/album.repository.sql b/server/src/queries/album.repository.sql index 35c82fdb7..4a2e7da8e 100644 --- a/server/src/queries/album.repository.sql +++ b/server/src/queries/album.repository.sql @@ -61,27 +61,7 @@ select where "shared_link"."albumId" = "album"."id" ) as agg - ) as "sharedLinks", - ( - select - json_agg("asset") as "assets" - from - ( - select - "asset".*, - "asset_exif" as "exifInfo" - from - "asset" - left join "asset_exif" on "asset"."id" = "asset_exif"."assetId" - inner join "album_asset" on "album_asset"."assetId" = "asset"."id" - where - "album_asset"."albumId" = "album"."id" - and "asset"."deletedAt" is null - and "asset"."visibility" in ('archive', 'timeline') - order by - "asset"."fileCreatedAt" desc - ) as "asset" - ) as "assets" + ) as "sharedLinks" from "album" where @@ -396,27 +376,7 @@ select "album_user"."role", "user"."name" asc ) as agg - ) as "albumUsers", - ( - select - json_agg("asset") as "assets" - from - ( - select - "asset".*, - "asset_exif" as "exifInfo" - from - "asset" - left join "asset_exif" on "asset"."id" = "asset_exif"."assetId" - inner join "album_asset" on "album_asset"."assetId" = "asset"."id" - where - "album_asset"."albumId" = "album"."id" - and "asset"."deletedAt" is null - and "asset"."visibility" in ('archive', 'timeline') - order by - "asset"."fileCreatedAt" desc - ) as "asset" - ) as "assets" + ) as "albumUsers" from "album" diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index aad1230cb..6a97043c1 100644 --- a/server/src/repositories/album.repository.ts +++ b/server/src/repositories/album.repository.ts @@ -1,14 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { - type ExpressionBuilder, - type Insertable, - type Kysely, - type NotNull, - type Selectable, - type ShallowDehydrateObject, - type Updateable, - sql, -} from 'kysely'; +import { type ExpressionBuilder, type Insertable, type Kysely, type Updateable, sql } from 'kysely'; import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres'; import { InjectKysely } from 'nestjs-kysely'; import { columns } from 'src/database.js'; @@ -17,7 +8,6 @@ import { AlbumUserCreateDto, MapAlbumDto } from 'src/dtos/album.dto.js'; import { AlbumUserRole } from 'src/enum.js'; import { DB } from 'src/schema/index.js'; import { AlbumTable } from 'src/schema/tables/album.table.js'; -import { AssetExifTable } from 'src/schema/tables/asset-exif.table.js'; import { asUuid, dummy, withDefaultVisibility } from 'src/utils/database.js'; export interface AlbumAssetCount { @@ -28,10 +18,6 @@ export interface AlbumAssetCount { lastModifiedAssetTimestamp: Date | null; } -export interface AlbumInfoOptions { - withAssets: boolean; -} - const withAlbumUsers = (authUserId?: string) => (eb: ExpressionBuilder) => jsonArrayFrom( eb @@ -52,27 +38,6 @@ const withSharedLink = (eb: ExpressionBuilder) => eb.selectFrom('shared_link').selectAll('shared_link').whereRef('shared_link.albumId', '=', 'album.id'), ).as('sharedLinks'); -const withAssets = (eb: ExpressionBuilder) => { - return eb - .selectFrom((eb) => - eb - .selectFrom('asset') - .selectAll('asset') - .leftJoin('asset_exif', 'asset.id', 'asset_exif.assetId') - .select((eb) => - eb.table('asset_exif').$castTo>>().as('exifInfo'), - ) - .innerJoin('album_asset', 'album_asset.assetId', 'asset.id') - .whereRef('album_asset.albumId', '=', 'album.id') - .where('asset.deletedAt', 'is', null) - .$call(withDefaultVisibility) - .orderBy('asset.fileCreatedAt', 'desc') - .as('asset'), - ) - .select((eb) => eb.fn.jsonAgg('asset').as('assets')) - .as('assets'); -}; - const isAlbumOwned = (ownerId: string) => (eb: ExpressionBuilder) => eb.exists( eb @@ -86,8 +51,8 @@ const isAlbumOwned = (ownerId: string) => (eb: ExpressionBuilder) = export class AlbumRepository { constructor(@InjectKysely() private db: Kysely) {} - @GenerateSql({ params: [DummyValue.UUID, { withAssets: true }, DummyValue.UUID] }) - getById(id: string, options: AlbumInfoOptions, authUserId?: string) { + @GenerateSql({ params: [DummyValue.UUID, DummyValue.UUID] }) + getById(id: string, authUserId?: string) { return this.db .with('album_user', (qb) => qb.selectFrom('album_user').selectAll().where('album_user.albumId', '=', id)) .selectFrom('album') @@ -96,8 +61,6 @@ export class AlbumRepository { .where('album.deletedAt', 'is', null) .select(withAlbumUsers(authUserId)) .select(withSharedLink) - .$if(options.withAssets, (eb) => eb.select(withAssets)) - .$narrowType<{ assets: NotNull }>() .executeTakeFirst(); } @@ -351,8 +314,6 @@ export class AlbumRepository { .selectFrom('album') .selectAll('album') .select(withAlbumUsers(authUserId)) - .select(withAssets) - .$narrowType<{ assets: NotNull }>() .executeTakeFirstOrThrow(); return result; diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 1f7662be4..f630760da 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -18,6 +18,7 @@ describe(AlbumService.name, () => { beforeEach(() => { ({ sut, mocks } = newTestService(AlbumService)); + mocks.album.getMetadataForIds.mockResolvedValue([]); }); it('should work', () => { @@ -591,7 +592,7 @@ describe(AlbumService.name, () => { expect(mocks.albumUser.delete).toHaveBeenCalledTimes(1); expect(mocks.albumUser.delete).toHaveBeenCalledWith({ albumId: album.id, userId }); - expect(mocks.album.getById).toHaveBeenCalledWith(album.id, { withAssets: false }, owner.id); + expect(mocks.album.getById).toHaveBeenCalledWith(album.id, owner.id); }); it('should prevent removing a shared user from a not-owned album (shared with auth user)', async () => { @@ -693,7 +694,7 @@ describe(AlbumService.name, () => { await sut.get(AuthFactory.create(owner), album.id); - expect(mocks.album.getById).toHaveBeenCalledWith(album.id, { withAssets: false }, owner.id); + expect(mocks.album.getById).toHaveBeenCalledWith(album.id, owner.id); expect(mocks.access.album.checkOwnerAccess).toHaveBeenCalledWith(owner.id, new Set([album.id])); }); @@ -714,7 +715,7 @@ describe(AlbumService.name, () => { const auth = AuthFactory.from().sharedLink().build(); await sut.get(auth, album.id); - expect(mocks.album.getById).toHaveBeenCalledWith(album.id, { withAssets: false }, auth.user.id); + expect(mocks.album.getById).toHaveBeenCalledWith(album.id, auth.user.id); expect(mocks.access.album.checkSharedLinkAccess).toHaveBeenCalledWith(auth.sharedLink!.id, new Set([album.id])); }); @@ -735,7 +736,7 @@ describe(AlbumService.name, () => { await sut.get(AuthFactory.create(user), album.id); - expect(mocks.album.getById).toHaveBeenCalledWith(album.id, { withAssets: false }, user.id); + expect(mocks.album.getById).toHaveBeenCalledWith(album.id, user.id); expect(mocks.access.album.checkSharedAlbumAccess).toHaveBeenCalledWith( user.id, new Set([album.id]), diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 3dfa0525a..c4a69e825 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -7,6 +7,7 @@ import { AlbumsAddAssetsResponseDto, CreateAlbumDto, GetAlbumsDto, + MapAlbumDto, UpdateAlbumDto, UpdateAlbumUserDto, mapAlbum, @@ -15,13 +16,21 @@ import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset import { AuthDto } from 'src/dtos/auth.dto.js'; import { MapMarkerResponseDto } from 'src/dtos/map.dto.js'; import { AlbumUserRole, Permission } from 'src/enum.js'; -import { AlbumAssetCount, AlbumInfoOptions } from 'src/repositories/album.repository.js'; +import { AlbumAssetCount } from 'src/repositories/album.repository.js'; import { BaseService } from 'src/services/base.service.js'; import { addAssets, removeAssets } from 'src/utils/asset.util.js'; import { asDateTimeString } from 'src/utils/date.js'; import { findOrFail } from 'src/utils/misc.js'; import { getPreferences } from 'src/utils/preferences.js'; +const mapAlbumMetadata = (metadata?: AlbumAssetCount) => ({ + startDate: asDateTimeString(metadata?.startDate ?? undefined), + endDate: asDateTimeString(metadata?.endDate ?? undefined), + assetCount: metadata?.assetCount ?? 0, + // lastModifiedAssetTimestamp is only used in mobile app, please remove if not need + lastModifiedAssetTimestamp: asDateTimeString(metadata?.lastModifiedAssetTimestamp ?? undefined), +}); + @Injectable() export class AlbumService extends BaseService { async getStatistics(auth: AuthDto): Promise { @@ -60,30 +69,21 @@ export class AlbumService extends BaseService { return albums.map((album) => ({ ...mapAlbum(album), sharedLinks: undefined, - startDate: asDateTimeString(albumMetadata[album.id]?.startDate ?? undefined), - endDate: asDateTimeString(albumMetadata[album.id]?.endDate ?? undefined), - assetCount: albumMetadata[album.id]?.assetCount ?? 0, - // lastModifiedAssetTimestamp is only used in mobile app, please remove if not need - lastModifiedAssetTimestamp: asDateTimeString(albumMetadata[album.id]?.lastModifiedAssetTimestamp ?? undefined), + ...mapAlbumMetadata(albumMetadata[album.id]), })); } async get(auth: AuthDto, id: string): Promise { await this.requireAccess({ auth, permission: Permission.AlbumRead, ids: [id] }); await this.albumRepository.updateThumbnails(); - const album = await this.findOrFail(id, auth.user.id, { withAssets: false }); - const [albumMetadataForIds] = await this.albumRepository.getMetadataForIds([album.id]); + const album = await this.findOrFail(id, auth.user.id); const hasSharedUsers = album.albumUsers && album.albumUsers.length > 1; const hasSharedLink = album.sharedLinks && album.sharedLinks.length > 0; const isShared = hasSharedUsers || hasSharedLink; return { - ...mapAlbum(album), - startDate: asDateTimeString(albumMetadataForIds?.startDate ?? undefined), - endDate: asDateTimeString(albumMetadataForIds?.endDate ?? undefined), - assetCount: albumMetadataForIds?.assetCount ?? 0, - lastModifiedAssetTimestamp: asDateTimeString(albumMetadataForIds?.lastModifiedAssetTimestamp ?? undefined), + ...(await this.mapWithMetadata(album)), contributorCounts: isShared ? await this.albumRepository.getContributorCounts(album.id) : undefined, }; } @@ -134,13 +134,13 @@ export class AlbumService extends BaseService { await this.eventRepository.emit('AlbumInvite', { id: album.id, userId, senderName: auth.user.name }); } - return mapAlbum(album); + return this.mapWithMetadata(album); } async update(auth: AuthDto, id: string, dto: UpdateAlbumDto): Promise { await this.requireAccess({ auth, permission: Permission.AlbumUpdate, ids: [id] }); - const album = await this.findOrFail(id, auth.user.id, { withAssets: true }); + const album = await this.findOrFail(id, auth.user.id); if (dto.albumThumbnailAssetId) { const results = await this.albumRepository.getAssetIds(id, [dto.albumThumbnailAssetId]); @@ -161,7 +161,7 @@ export class AlbumService extends BaseService { auth.user.id, ); - return mapAlbum({ ...updatedAlbum, assets: album.assets }); + return this.mapWithMetadata(updatedAlbum); } async delete(auth: AuthDto, id: string): Promise { @@ -170,7 +170,7 @@ export class AlbumService extends BaseService { } async addAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise { - const album = await this.findOrFail(id, auth.user.id, { withAssets: false }); + const album = await this.findOrFail(id, auth.user.id); await this.requireAccess({ auth, permission: Permission.AlbumAssetCreate, ids: [id] }); const results = await addAssets( @@ -229,7 +229,7 @@ export class AlbumService extends BaseService { if (notPresentAssetIds.length === 0) { continue; } - const album = await this.findOrFail(albumId, auth.user.id, { withAssets: false }); + const album = await this.findOrFail(albumId, auth.user.id); results.error = undefined; results.success = true; @@ -261,7 +261,7 @@ export class AlbumService extends BaseService { async removeAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise { await this.requireAccess({ auth, permission: Permission.AlbumAssetDelete, ids: [id] }); - const album = await this.findOrFail(id, auth.user.id, { withAssets: false }); + const album = await this.findOrFail(id, auth.user.id); const results = await removeAssets( auth, { access: this.accessRepository, bulk: this.albumRepository }, @@ -287,7 +287,7 @@ export class AlbumService extends BaseService { async addUsers(auth: AuthDto, id: string, { albumUsers }: AddUsersDto): Promise { await this.requireAccess({ auth, permission: Permission.AlbumShare, ids: [id] }); - const album = await this.findOrFail(id, auth.user.id, { withAssets: false }); + const album = await this.findOrFail(id, auth.user.id); for (const { userId, role } of albumUsers) { if (role === AlbumUserRole.Owner) { @@ -309,7 +309,7 @@ export class AlbumService extends BaseService { await this.eventRepository.emit('AlbumInvite', { id, userId, senderName: auth.user.name }); } - return mapAlbum(await this.findOrFail(id, auth.user.id, { withAssets: true })); + return this.mapWithMetadata(await this.findOrFail(id, auth.user.id)); } async removeUser(auth: AuthDto, id: string, userId: string | 'me'): Promise { @@ -317,7 +317,7 @@ export class AlbumService extends BaseService { userId = auth.user.id; } - const album = await this.findOrFail(id, auth.user.id, { withAssets: false }); + const album = await this.findOrFail(id, auth.user.id); const exists = album.albumUsers.find(({ user: { id } }) => id === userId); if (!exists) { @@ -342,7 +342,7 @@ export class AlbumService extends BaseService { async updateUser(auth: AuthDto, id: string, userId: string, dto: UpdateAlbumUserDto): Promise { await this.requireAccess({ auth, permission: Permission.AlbumShare, ids: [id] }); - const album = await this.findOrFail(id, userId, { withAssets: false }); + const album = await this.findOrFail(id, userId); const owner = album.albumUsers[0]; if (owner.user.id === userId) { @@ -352,7 +352,12 @@ export class AlbumService extends BaseService { await this.albumUserRepository.update({ albumId: id, userId }, { role: dto.role }); } - private findOrFail(id: string, authUserId: string, options: AlbumInfoOptions) { - return findOrFail(() => this.albumRepository.getById(id, options, authUserId), 'Album'); + private findOrFail(id: string, authUserId: string) { + return findOrFail(() => this.albumRepository.getById(id, authUserId), 'Album'); + } + + private async mapWithMetadata(album: MapAlbumDto): Promise { + const [metadata] = await this.albumRepository.getMetadataForIds([album.id]); + return { ...mapAlbum(album), ...mapAlbumMetadata(metadata) }; } } diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index b12161e61..1b28f5a4a 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -351,7 +351,7 @@ export class AssetMediaService extends BaseService { return; } - const album = await this.albumRepository.getById(sharedLink.albumId, { withAssets: false }); + const album = await this.albumRepository.getById(sharedLink.albumId); if (!album) { return; } diff --git a/server/src/services/notification.service.ts b/server/src/services/notification.service.ts index 6e4e7b0b9..24a6c7024 100644 --- a/server/src/services/notification.service.ts +++ b/server/src/services/notification.service.ts @@ -324,7 +324,7 @@ export class NotificationService extends BaseService { @OnJob({ name: JobName.NotifyAlbumInvite, queue: QueueName.Notification }) async handleAlbumInvite({ id, recipientId, senderName }: JobOf) { - const album = await this.albumRepository.getById(id, { withAssets: false }); + const album = await this.albumRepository.getById(id); if (!album) { return JobStatus.Skipped; } @@ -374,7 +374,7 @@ export class NotificationService extends BaseService { @OnJob({ name: JobName.NotifyAlbumUpdate, queue: QueueName.Notification }) async handleAlbumUpdate({ id, recipientId }: JobOf) { - const album = await this.albumRepository.getById(id, { withAssets: false }); + const album = await this.albumRepository.getById(id); if (!album) { return JobStatus.Skipped;