From 05a446c259a3c1658934c85abdad9574fbdb2cf3 Mon Sep 17 00:00:00 2001 From: Mert <101130780+mertalev@users.noreply.github.com> Date: Sat, 25 Jan 2025 23:37:19 -0500 Subject: [PATCH] fix(server): avoid duplicate rows in album queries (#15670) * avoid duplicate rows * left join, handle null vs. undefined * update sql --- e2e/src/api/specs/album.e2e-spec.ts | 5 +- server/src/interfaces/album.interface.ts | 4 +- server/src/queries/album.repository.sql | 131 ++++++++------------ server/src/repositories/album.repository.ts | 69 ++++++----- server/src/services/album.service.spec.ts | 8 +- server/src/services/album.service.ts | 16 +-- 6 files changed, 103 insertions(+), 130 deletions(-) diff --git a/e2e/src/api/specs/album.e2e-spec.ts b/e2e/src/api/specs/album.e2e-spec.ts index 1d142ac468dbe..5c087d126977b 100644 --- a/e2e/src/api/specs/album.e2e-spec.ts +++ b/e2e/src/api/specs/album.e2e-spec.ts @@ -52,7 +52,10 @@ describe('/albums', () => { user1Albums = await Promise.all([ utils.createAlbum(user1.accessToken, { albumName: user1SharedEditorUser, - albumUsers: [{ userId: user2.userId, role: AlbumUserRole.Editor }], + albumUsers: [ + { userId: admin.userId, role: AlbumUserRole.Editor }, + { userId: user2.userId, role: AlbumUserRole.Editor }, + ], assetIds: [user1Asset1.id], }), utils.createAlbum(user1.accessToken, { diff --git a/server/src/interfaces/album.interface.ts b/server/src/interfaces/album.interface.ts index 7af1bd97e10bc..36a6d8a1d2676 100644 --- a/server/src/interfaces/album.interface.ts +++ b/server/src/interfaces/album.interface.ts @@ -9,8 +9,8 @@ export const IAlbumRepository = 'IAlbumRepository'; export interface AlbumAssetCount { albumId: string; assetCount: number; - startDate: Date | undefined; - endDate: Date | undefined; + startDate: Date | null; + endDate: Date | null; } export interface AlbumInfoOptions { diff --git a/server/src/queries/album.repository.sql b/server/src/queries/album.repository.sql index b982ea2cffd96..217b0ce77bf40 100644 --- a/server/src/queries/album.repository.sql +++ b/server/src/queries/album.repository.sql @@ -90,7 +90,7 @@ select ( select "assets".*, - to_json("exif") as "exifInfo" + "exif" as "exifInfo" from "assets" inner join "exif" on "assets"."id" = "exif"."assetId" @@ -180,19 +180,20 @@ select ) as "albumUsers" from "albums" - left join "albums_assets_assets" as "album_assets" on "album_assets"."albumsId" = "albums"."id" - left join "albums_shared_users_users" as "album_users" on "album_users"."albumsId" = "albums"."id" + inner join "albums_assets_assets" as "album_assets" on "album_assets"."albumsId" = "albums"."id" where ( - ( - "albums"."ownerId" = $1 - and "album_assets"."assetsId" = $2 - ) - or ( - "album_users"."usersId" = $3 - and "album_assets"."assetsId" = $4 + "albums"."ownerId" = $1 + or exists ( + select + from + "albums_shared_users_users" as "album_users" + where + "album_users"."albumsId" = "albums"."id" + and "album_users"."usersId" = $2 ) ) + and "album_assets"."assetsId" = $3 and "albums"."deletedAt" is null order by "albums"."createdAt" desc, @@ -200,10 +201,10 @@ order by -- AlbumRepository.getMetadataForIds select - "albums"."id", + "albums"."id" as "albumId", min("assets"."fileCreatedAt") as "startDate", max("assets"."fileCreatedAt") as "endDate", - count("assets"."id") as "assetCount" + count("assets"."id")::int as "assetCount" from "albums" left join "albums_assets_assets" as "album_assets" on "album_assets"."albumsId" = "albums"."id" @@ -306,8 +307,8 @@ order by "albums"."createdAt" desc -- AlbumRepository.getShared -select distinct - on ("albums"."createdAt") "albums".*, +select + "albums".*, ( select coalesce(json_agg(agg), '[]') @@ -390,15 +391,26 @@ select distinct ) as "sharedLinks" from "albums" - left join "albums_shared_users_users" as "shared_albums" on "shared_albums"."albumsId" = "albums"."id" - left join "shared_links" on "shared_links"."albumId" = "albums"."id" where ( - "shared_albums"."usersId" = $1 - or "shared_links"."userId" = $2 - or ( - "albums"."ownerId" = $3 - and "shared_albums"."usersId" is not null + exists ( + select + from + "albums_shared_users_users" as "album_users" + where + "album_users"."albumsId" = "albums"."id" + and ( + "albums"."ownerId" = $1 + or "album_users"."usersId" = $2 + ) + ) + or exists ( + select + from + "shared_links" + where + "shared_links"."albumId" = "albums"."id" + and "shared_links"."userId" = $3 ) ) and "albums"."deletedAt" is null @@ -406,48 +418,8 @@ order by "albums"."createdAt" desc -- AlbumRepository.getNotShared -select distinct - on ("albums"."createdAt") "albums".*, - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "album_users".*, - ( - select - to_json(obj) - from - ( - select - "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", - "name", - "quotaSizeInBytes", - "quotaUsageInBytes", - "status", - "profileChangedAt" - from - "users" - where - "users"."id" = "album_users"."usersId" - ) as obj - ) as "user" - from - "albums_shared_users_users" as "album_users" - where - "album_users"."albumsId" = "albums"."id" - ) as agg - ) as "albumUsers", +select + "albums".*, ( select to_json(obj) @@ -474,29 +446,26 @@ select distinct where "users"."id" = "albums"."ownerId" ) as obj - ) as "owner", - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - * - from - "shared_links" - where - "shared_links"."albumId" = "albums"."id" - ) as agg - ) as "sharedLinks" + ) as "owner" from "albums" - left join "albums_shared_users_users" as "shared_albums" on "shared_albums"."albumsId" = "albums"."id" - left join "shared_links" on "shared_links"."albumId" = "albums"."id" where "albums"."ownerId" = $1 - and "shared_albums"."usersId" is null - and "shared_links"."userId" is null and "albums"."deletedAt" is null + and not exists ( + select + from + "albums_shared_users_users" as "album_users" + where + "album_users"."albumsId" = "albums"."id" + ) + and not exists ( + select + from + "shared_links" + where + "shared_links"."albumId" = "albums"."id" + ) order by "albums"."createdAt" desc diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index c6e01b532e535..d63fd2ed4f2d5 100644 --- a/server/src/repositories/album.repository.ts +++ b/server/src/repositories/album.repository.ts @@ -59,7 +59,7 @@ const withAssets = (eb: ExpressionBuilder) => { .selectFrom('assets') .selectAll('assets') .innerJoin('exif', 'assets.id', 'exif.assetId') - .select((eb) => eb.fn.toJson('exif').as('exifInfo')) + .select((eb) => eb.table('exif').as('exifInfo')) .innerJoin('albums_assets_assets', 'albums_assets_assets.assetsId', 'assets.id') .whereRef('albums_assets_assets.albumsId', '=', 'albums.id') .where('assets.deletedAt', 'is', null) @@ -93,14 +93,19 @@ export class AlbumRepository implements IAlbumRepository { return this.db .selectFrom('albums') .selectAll('albums') - .leftJoin('albums_assets_assets as album_assets', 'album_assets.albumsId', 'albums.id') - .leftJoin('albums_shared_users_users as album_users', 'album_users.albumsId', 'albums.id') + .innerJoin('albums_assets_assets as album_assets', 'album_assets.albumsId', 'albums.id') .where((eb) => eb.or([ - eb.and([eb('albums.ownerId', '=', ownerId), eb('album_assets.assetsId', '=', assetId)]), - eb.and([eb('album_users.usersId', '=', ownerId), eb('album_assets.assetsId', '=', assetId)]), + eb('albums.ownerId', '=', ownerId), + eb.exists( + eb + .selectFrom('albums_shared_users_users as album_users') + .whereRef('album_users.albumsId', '=', 'albums.id') + .where('album_users.usersId', '=', ownerId), + ), ]), ) + .where('album_assets.assetsId', '=', assetId) .where('albums.deletedAt', 'is', null) .orderBy('albums.createdAt', 'desc') .select(withOwner) @@ -117,25 +122,18 @@ export class AlbumRepository implements IAlbumRepository { return []; } - const metadatas = await this.db + return this.db .selectFrom('albums') .leftJoin('albums_assets_assets as album_assets', 'album_assets.albumsId', 'albums.id') .leftJoin('assets', 'assets.id', 'album_assets.assetsId') - .select('albums.id') + .select('albums.id as albumId') .select((eb) => eb.fn.min('assets.fileCreatedAt').as('startDate')) .select((eb) => eb.fn.max('assets.fileCreatedAt').as('endDate')) - .select((eb) => eb.fn.count('assets.id').as('assetCount')) + .select((eb) => sql`${eb.fn.count('assets.id')}::int`.as('assetCount')) .where('albums.id', 'in', ids) .where('assets.deletedAt', 'is', null) .groupBy('albums.id') .execute(); - - return metadatas.map((metadatas) => ({ - albumId: metadatas.id, - assetCount: Number(metadatas.assetCount), - startDate: metadatas.startDate ? new Date(metadatas.startDate) : undefined, - endDate: metadatas.endDate ? new Date(metadatas.endDate) : undefined, - })); } @GenerateSql({ params: [DummyValue.UUID] }) @@ -160,14 +158,20 @@ export class AlbumRepository implements IAlbumRepository { return this.db .selectFrom('albums') .selectAll('albums') - .distinctOn('albums.createdAt') - .leftJoin('albums_shared_users_users as shared_albums', 'shared_albums.albumsId', 'albums.id') - .leftJoin('shared_links', 'shared_links.albumId', 'albums.id') .where((eb) => eb.or([ - eb('shared_albums.usersId', '=', ownerId), - eb('shared_links.userId', '=', ownerId), - eb.and([eb('albums.ownerId', '=', ownerId), eb('shared_albums.usersId', 'is not', null)]), + eb.exists( + eb + .selectFrom('albums_shared_users_users as album_users') + .whereRef('album_users.albumsId', '=', 'albums.id') + .where((eb) => eb.or([eb('albums.ownerId', '=', ownerId), eb('album_users.usersId', '=', ownerId)])), + ), + eb.exists( + eb + .selectFrom('shared_links') + .whereRef('shared_links.albumId', '=', 'albums.id') + .where('shared_links.userId', '=', ownerId), + ), ]), ) .where('albums.deletedAt', 'is', null) @@ -186,16 +190,21 @@ export class AlbumRepository implements IAlbumRepository { return this.db .selectFrom('albums') .selectAll('albums') - .distinctOn('albums.createdAt') - .leftJoin('albums_shared_users_users as shared_albums', 'shared_albums.albumsId', 'albums.id') - .leftJoin('shared_links', 'shared_links.albumId', 'albums.id') .where('albums.ownerId', '=', ownerId) - .where('shared_albums.usersId', 'is', null) - .where('shared_links.userId', 'is', null) .where('albums.deletedAt', 'is', null) - .select(withAlbumUsers) + .where((eb) => + eb.not( + eb.exists( + eb + .selectFrom('albums_shared_users_users as album_users') + .whereRef('album_users.albumsId', '=', 'albums.id'), + ), + ), + ) + .where((eb) => + eb.not(eb.exists(eb.selectFrom('shared_links').whereRef('shared_links.albumId', '=', 'albums.id'))), + ) .select(withOwner) - .select(withSharedLink) .orderBy('albums.createdAt', 'desc') .execute() as unknown as Promise; } @@ -282,7 +291,6 @@ export class AlbumRepository implements IAlbumRepository { .selectAll() .where('id', '=', newAlbum.id) .select(withOwner) - .select(withSharedLink) .select(withAssets) .select(withAlbumUsers) .executeTakeFirst() as unknown as Promise; @@ -292,7 +300,7 @@ export class AlbumRepository implements IAlbumRepository { update(id: string, album: Updateable): Promise { return this.db .updateTable('albums') - .set({ ...album, updatedAt: new Date() }) + .set(album) .where('id', '=', id) .returningAll('albums') .returning(withOwner) @@ -335,7 +343,6 @@ export class AlbumRepository implements IAlbumRepository { .select('album_assets.assetsId') .orderBy('assets.fileCreatedAt', 'desc') .limit(1), - updatedAt: new Date(), })) .where((eb) => eb.or([ diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index fe732843b6eb0..942615b0d9efc 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -52,8 +52,8 @@ describe(AlbumService.name, () => { it('gets list of albums for auth user', async () => { albumMock.getOwned.mockResolvedValue([albumStub.empty, albumStub.sharedWithUser]); albumMock.getMetadataForIds.mockResolvedValue([ - { albumId: albumStub.empty.id, assetCount: 0, startDate: undefined, endDate: undefined }, - { albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: undefined, endDate: undefined }, + { albumId: albumStub.empty.id, assetCount: 0, startDate: null, endDate: null }, + { albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: null, endDate: null }, ]); const result = await sut.getAll(authStub.admin, {}); @@ -82,7 +82,7 @@ describe(AlbumService.name, () => { it('gets list of albums that are shared', async () => { albumMock.getShared.mockResolvedValue([albumStub.sharedWithUser]); albumMock.getMetadataForIds.mockResolvedValue([ - { albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: undefined, endDate: undefined }, + { albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: null, endDate: null }, ]); const result = await sut.getAll(authStub.admin, { shared: true }); @@ -94,7 +94,7 @@ describe(AlbumService.name, () => { it('gets list of albums that are NOT shared', async () => { albumMock.getNotShared.mockResolvedValue([albumStub.empty]); albumMock.getMetadataForIds.mockResolvedValue([ - { albumId: albumStub.empty.id, assetCount: 0, startDate: undefined, endDate: undefined }, + { albumId: albumStub.empty.id, assetCount: 0, startDate: null, endDate: null }, ]); const result = await sut.getAll(authStub.admin, { shared: false }); diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index efc71c4c8d9a6..0b6c646801aee 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -55,13 +55,7 @@ export class AlbumService extends BaseService { const results = await this.albumRepository.getMetadataForIds(albums.map((album) => album.id)); const albumMetadata: Record = {}; for (const metadata of results) { - const { albumId, assetCount, startDate, endDate } = metadata; - albumMetadata[albumId] = { - albumId, - assetCount, - startDate, - endDate, - }; + albumMetadata[metadata.albumId] = metadata; } return Promise.all( @@ -70,8 +64,8 @@ export class AlbumService extends BaseService { return { ...mapAlbumWithoutAssets(album), sharedLinks: undefined, - startDate: albumMetadata[album.id].startDate, - endDate: albumMetadata[album.id].endDate, + startDate: albumMetadata[album.id].startDate ?? undefined, + endDate: albumMetadata[album.id].endDate ?? undefined, assetCount: albumMetadata[album.id].assetCount, lastModifiedAssetTimestamp: lastModifiedAsset?.updatedAt, }; @@ -89,8 +83,8 @@ export class AlbumService extends BaseService { return { ...mapAlbum(album, withAssets, auth), - startDate: albumMetadataForIds.startDate, - endDate: albumMetadataForIds.endDate, + startDate: albumMetadataForIds.startDate ?? undefined, + endDate: albumMetadataForIds.endDate ?? undefined, assetCount: albumMetadataForIds.assetCount, lastModifiedAssetTimestamp: lastModifiedAsset?.updatedAt, };