Skip to content

Commit

Permalink
fix(server): avoid duplicate rows in album queries (#15670)
Browse files Browse the repository at this point in the history 
* avoid duplicate rows

* left join, handle null vs. undefined

* update sql
  • Loading branch information
@mertalev
mertalev authored Jan 26, 2025
1 parent 4f725b9 commit 05a446c
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 130 deletions.
5 changes: 4 additions & 1 deletion e2e/src/api/specs/album.e2e-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
4 changes: 2 additions & 2 deletions server/src/interfaces/album.interface.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
131 changes: 50 additions & 81 deletions server/src/queries/album.repository.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -180,30 +180,31 @@ 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,
"albums"."createdAt" desc

-- 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"
Expand Down Expand Up @@ -306,8 +307,8 @@ order by
"albums"."createdAt" desc

-- AlbumRepository.getShared
select distinct
on ("albums"."createdAt") "albums".*,
select
"albums".*,
(
select
coalesce(json_agg(agg), '[]')
Expand Down Expand Up @@ -390,64 +391,35 @@ 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
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)
Expand All @@ -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

Expand Down
69 changes: 38 additions & 31 deletions server/src/repositories/album.repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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] })
Expand All @@ -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)
Expand All @@ -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[]>;
}
Expand Down Expand Up @@ -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>;
Expand All @@ -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)
Expand Down Expand Up @@ -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([
Expand Down
8 changes: 4 additions & 4 deletions server/src/services/album.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, {});
Expand Down Expand Up @@ -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 });
Expand All @@ -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 });
Expand Down
Loading

0 comments on commit 05a446c

Please sign in to comment.