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

@@ -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, {

server/src/interfaces/album.interface.ts

@@ -9,8 +9,8 @@ export const IAlbumRepository = 'IAlbumRepository';
 export interface AlbumAssetCount {
   albumId: string;
   assetCount: number;
-  startDate: Date | undefined;
+  startDate: Date | null;
-  endDate: Date | undefined;
+  endDate: Date | null;
 }
 
 export interface AlbumInfoOptions {

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"
+  inner 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"
 where
   (
-    (
+    "albums"."ownerId" = $1
-      "albums"."ownerId" = $1
+    or exists (
-      and "album_assets"."assetsId" = $2
+      select
-    )
+      from
-    or (
+        "albums_shared_users_users" as "album_users"
-      "album_users"."usersId" = $3
+      where
-      and "album_assets"."assetsId" = $4
+        "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
+select
-  on ("albums"."createdAt") "albums".*,
+  "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
+    exists (
-    or "shared_links"."userId" = $2
+      select
-    or (
+      from
-      "albums"."ownerId" = $3
+        "albums_shared_users_users" as "album_users"
-      and "shared_albums"."usersId" is not null
+      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
+select
-  on ("albums"."createdAt") "albums".*,
+  "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
       to_json(obj)
@@ -474,29 +446,26 @@ select distinct
         where
           "users"."id" = "albums"."ownerId"
       ) as obj
-  ) as "owner",
+  ) as "owner"
-  (
-    select
-      coalesce(json_agg(agg), '[]')
-    from
-      (
-        select
-          *
-        from
-          "shared_links"
-        where
-          "shared_links"."albumId" = "albums"."id"
-      ) as agg
-  ) 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
   "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
 

server/src/repositories/album.repository.ts

@@ -59,7 +59,7 @@ const withAssets = (eb: ExpressionBuilder<DB, 'albums'>) => {
         .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')
+      .innerJoin('albums_assets_assets as album_assets', 'album_assets.albumsId', 'albums.id')
-      .leftJoin('albums_shared_users_users as album_users', 'album_users.albumsId', 'albums.id')
       .where((eb) =>
         eb.or([
-          eb.and([eb('albums.ownerId', '=', ownerId), eb('album_assets.assetsId', '=', assetId)]),
+          eb('albums.ownerId', '=', ownerId),
-          eb.and([eb('album_users.usersId', '=', ownerId), eb('album_assets.assetsId', '=', assetId)]),
+          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<number>`${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.exists(
-          eb('shared_links.userId', '=', ownerId),
+            eb
-          eb.and([eb('albums.ownerId', '=', ownerId), eb('shared_albums.usersId', 'is not', null)]),
+              .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<AlbumEntity[]>;
   }
@@ -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<AlbumEntity>;
@@ -292,7 +300,7 @@ export class AlbumRepository implements IAlbumRepository {
   update(id: string, album: Updateable<Albums>): Promise<AlbumEntity> {
     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([

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.empty.id, assetCount: 0, startDate: null, endDate: null },
-        { 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, {});
@@ -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 });

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<string, AlbumAssetCount> = {};
     for (const metadata of results) {
-      const { albumId, assetCount, startDate, endDate } = metadata;
+      albumMetadata[metadata.albumId] = metadata;
-      albumMetadata[albumId] = {
-        albumId,
-        assetCount,
-        startDate,
-        endDate,
-      };
     }
 
     return Promise.all(
@@ -70,8 +64,8 @@ export class AlbumService extends BaseService {
         return {
           ...mapAlbumWithoutAssets(album),
           sharedLinks: undefined,
-          startDate: albumMetadata[album.id].startDate,
+          startDate: albumMetadata[album.id].startDate ?? undefined,
-          endDate: albumMetadata[album.id].endDate,
+          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,
+      startDate: albumMetadataForIds.startDate ?? undefined,
-      endDate: albumMetadataForIds.endDate,
+      endDate: albumMetadataForIds.endDate ?? undefined,
       assetCount: albumMetadataForIds.assetCount,
       lastModifiedAssetTimestamp: lastModifiedAsset?.updatedAt,
     };