From f094ff2aa1fdd048f0174f0bc7110fb106b0070c Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Nov 2023 10:07:49 -0600 Subject: [PATCH] fix(server): album perf query (#5232) * Revert "fix: album performances (#5224)" This reverts commit c438e179543fe4f079dfa4ec15a227ad89b359ae. * Revert "fix: album sorting options (#5127)" This reverts commit 725f30c49448689f781d6d25374e6d08d1874c4b. --- server/src/domain/album/album-response.dto.ts | 15 ++- server/src/domain/album/album.service.spec.ts | 14 ++ server/src/domain/album/album.service.ts | 12 +- .../domain/repositories/album.repository.ts | 1 + .../shared-link/shared-link-response.dto.ts | 2 +- server/src/infra/entities/album.entity.ts | 31 ----- .../infra/repositories/album.repository.ts | 28 +++- server/test/fixtures/album.stub.ts | 30 ----- server/test/fixtures/shared-link.stub.ts | 3 - .../repositories/album.repository.mock.ts | 1 + .../components/elements/table-header.svelte | 8 +- .../sharedlinks-page/shared-link-card.svelte | 25 +--- web/src/routes/(user)/albums/+page.svelte | 122 +++++------------- 13 files changed, 108 insertions(+), 184 deletions(-) diff --git a/server/src/domain/album/album-response.dto.ts b/server/src/domain/album/album-response.dto.ts index e6967ec6c7..671922408e 100644 --- a/server/src/domain/album/album-response.dto.ts +++ b/server/src/domain/album/album-response.dto.ts @@ -37,6 +37,15 @@ export const mapAlbum = (entity: AlbumEntity, withAssets: boolean): AlbumRespons const hasSharedLink = entity.sharedLinks?.length > 0; const hasSharedUser = sharedUsers.length > 0; + let startDate = assets.at(0)?.fileCreatedAt || undefined; + let endDate = assets.at(-1)?.fileCreatedAt || undefined; + // Swap dates if start date is greater than end date. + if (startDate && endDate && startDate > endDate) { + const temp = startDate; + startDate = endDate; + endDate = temp; + } + return { albumName: entity.albumName, description: entity.description, @@ -49,10 +58,10 @@ export const mapAlbum = (entity: AlbumEntity, withAssets: boolean): AlbumRespons sharedUsers, shared: hasSharedUser || hasSharedLink, hasSharedLink, - startDate: entity.startDate ? entity.startDate : undefined, - endDate: entity.endDate ? entity.endDate : undefined, + startDate, + endDate, assets: (withAssets ? assets : []).map((asset) => mapAsset(asset)), - assetCount: entity.assetCount, + assetCount: entity.assets?.length || 0, isActivityEnabled: entity.isActivityEnabled, }; }; diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index 8182df0402..a93cb0ad17 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/server/src/domain/album/album.service.spec.ts @@ -58,6 +58,10 @@ describe(AlbumService.name, () => { describe('getAll', () => { it('gets list of albums for auth user', async () => { albumMock.getOwned.mockResolvedValue([albumStub.empty, albumStub.sharedWithUser]); + albumMock.getAssetCountForIds.mockResolvedValue([ + { albumId: albumStub.empty.id, assetCount: 0 }, + { albumId: albumStub.sharedWithUser.id, assetCount: 0 }, + ]); albumMock.getInvalidThumbnail.mockResolvedValue([]); const result = await sut.getAll(authStub.admin, {}); @@ -68,6 +72,7 @@ describe(AlbumService.name, () => { it('gets list of albums that have a specific asset', async () => { albumMock.getByAssetId.mockResolvedValue([albumStub.oneAsset]); + albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.oneAsset.id, assetCount: 1 }]); albumMock.getInvalidThumbnail.mockResolvedValue([]); const result = await sut.getAll(authStub.admin, { assetId: albumStub.oneAsset.id }); @@ -78,6 +83,7 @@ describe(AlbumService.name, () => { it('gets list of albums that are shared', async () => { albumMock.getShared.mockResolvedValue([albumStub.sharedWithUser]); + albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.sharedWithUser.id, assetCount: 0 }]); albumMock.getInvalidThumbnail.mockResolvedValue([]); const result = await sut.getAll(authStub.admin, { shared: true }); @@ -88,6 +94,7 @@ describe(AlbumService.name, () => { it('gets list of albums that are NOT shared', async () => { albumMock.getNotShared.mockResolvedValue([albumStub.empty]); + albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.empty.id, assetCount: 0 }]); albumMock.getInvalidThumbnail.mockResolvedValue([]); const result = await sut.getAll(authStub.admin, { shared: false }); @@ -99,6 +106,7 @@ describe(AlbumService.name, () => { it('counts assets correctly', async () => { albumMock.getOwned.mockResolvedValue([albumStub.oneAsset]); + albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.oneAsset.id, assetCount: 1 }]); albumMock.getInvalidThumbnail.mockResolvedValue([]); const result = await sut.getAll(authStub.admin, {}); @@ -110,6 +118,9 @@ describe(AlbumService.name, () => { it('updates the album thumbnail by listing all albums', async () => { albumMock.getOwned.mockResolvedValue([albumStub.oneAssetInvalidThumbnail]); + albumMock.getAssetCountForIds.mockResolvedValue([ + { albumId: albumStub.oneAssetInvalidThumbnail.id, assetCount: 1 }, + ]); albumMock.getInvalidThumbnail.mockResolvedValue([albumStub.oneAssetInvalidThumbnail.id]); albumMock.update.mockResolvedValue(albumStub.oneAssetValidThumbnail); assetMock.getFirstAssetForAlbumId.mockResolvedValue(albumStub.oneAssetInvalidThumbnail.assets[0]); @@ -123,6 +134,9 @@ describe(AlbumService.name, () => { it('removes the thumbnail for an empty album', async () => { albumMock.getOwned.mockResolvedValue([albumStub.emptyWithInvalidThumbnail]); + albumMock.getAssetCountForIds.mockResolvedValue([ + { albumId: albumStub.emptyWithInvalidThumbnail.id, assetCount: 1 }, + ]); albumMock.getInvalidThumbnail.mockResolvedValue([albumStub.emptyWithInvalidThumbnail.id]); albumMock.update.mockResolvedValue(albumStub.emptyWithValidThumbnail); assetMock.getFirstAssetForAlbumId.mockResolvedValue(null); diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 9033bcd1c6..0fb0391ef4 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -66,12 +66,21 @@ export class AlbumService { albums = await this.albumRepository.getOwned(ownerId); } + // Get asset count for each album. Then map the result to an object: + // { [albumId]: assetCount } + const albumsAssetCount = await this.albumRepository.getAssetCountForIds(albums.map((album) => album.id)); + const albumsAssetCountObj = albumsAssetCount.reduce((obj: Record, { albumId, assetCount }) => { + obj[albumId] = assetCount; + return obj; + }, {}); + return Promise.all( albums.map(async (album) => { const lastModifiedAsset = await this.assetRepository.getLastUpdatedAssetForAlbumId(album.id); return { ...mapAlbumWithoutAssets(album), sharedLinks: undefined, + assetCount: albumsAssetCountObj[album.id], lastModifiedAssetTimestamp: lastModifiedAsset?.fileModifiedAt, }; }), @@ -81,8 +90,7 @@ export class AlbumService { async get(authUser: AuthUserDto, id: string, dto: AlbumInfoDto): Promise { await this.access.requirePermission(authUser, Permission.ALBUM_READ, id); await this.albumRepository.updateThumbnails(); - const withAssets = dto.withoutAssets === undefined ? true : !dto.withoutAssets; - return mapAlbum(await this.findOrFail(id, { withAssets }), !dto.withoutAssets); + return mapAlbum(await this.findOrFail(id, { withAssets: true }), !dto.withoutAssets); } async create(authUser: AuthUserDto, dto: CreateAlbumDto): Promise { diff --git a/server/src/domain/repositories/album.repository.ts b/server/src/domain/repositories/album.repository.ts index 3f37be69f7..d3ca62da12 100644 --- a/server/src/domain/repositories/album.repository.ts +++ b/server/src/domain/repositories/album.repository.ts @@ -30,6 +30,7 @@ export interface IAlbumRepository { hasAsset(asset: AlbumAsset): Promise; removeAsset(assetId: string): Promise; removeAssets(assets: AlbumAssets): Promise; + getAssetCountForIds(ids: string[]): Promise; getInvalidThumbnail(): Promise; getOwned(ownerId: string): Promise; getShared(ownerId: string): Promise; diff --git a/server/src/domain/shared-link/shared-link-response.dto.ts b/server/src/domain/shared-link/shared-link-response.dto.ts index 67e8e66c69..b16a578f41 100644 --- a/server/src/domain/shared-link/shared-link-response.dto.ts +++ b/server/src/domain/shared-link/shared-link-response.dto.ts @@ -40,7 +40,7 @@ export function mapSharedLink(sharedLink: SharedLinkEntity): SharedLinkResponseD createdAt: sharedLink.createdAt, expiresAt: sharedLink.expiresAt, assets: assets.map((asset) => mapAsset(asset)), - album: sharedLink.album?.id ? mapAlbumWithoutAssets(sharedLink.album) : undefined, + album: sharedLink.album ? mapAlbumWithoutAssets(sharedLink.album) : undefined, allowUpload: sharedLink.allowUpload, allowDownload: sharedLink.allowDownload, showMetadata: sharedLink.showExif, diff --git a/server/src/infra/entities/album.entity.ts b/server/src/infra/entities/album.entity.ts index 2e16b7f7c2..fbc125351a 100644 --- a/server/src/infra/entities/album.entity.ts +++ b/server/src/infra/entities/album.entity.ts @@ -9,7 +9,6 @@ import { OneToMany, PrimaryGeneratedColumn, UpdateDateColumn, - VirtualColumn, } from 'typeorm'; import { AssetEntity } from './asset.entity'; import { SharedLinkEntity } from './shared-link.entity'; @@ -60,34 +59,4 @@ export class AlbumEntity { @Column({ default: true }) isActivityEnabled!: boolean; - - @VirtualColumn({ - query: (alias) => ` - SELECT MIN(assets."fileCreatedAt") - FROM "assets" assets - JOIN "albums_assets_assets" aa ON aa."assetsId" = assets.id - WHERE aa."albumsId" = ${alias}.id - `, - }) - startDate!: Date | null; - - @VirtualColumn({ - query: (alias) => ` - SELECT MAX(assets."fileCreatedAt") - FROM "assets" assets - JOIN "albums_assets_assets" aa ON aa."assetsId" = assets.id - WHERE aa."albumsId" = ${alias}.id - `, - }) - endDate!: Date | null; - - @VirtualColumn({ - query: (alias) => ` - SELECT COUNT(assets."id") - FROM "assets" assets - JOIN "albums_assets_assets" aa ON aa."assetsId" = assets.id - WHERE aa."albumsId" = ${alias}.id - `, - }) - assetCount!: number; } diff --git a/server/src/infra/repositories/album.repository.ts b/server/src/infra/repositories/album.repository.ts index c71ce1c38a..69df226859 100644 --- a/server/src/infra/repositories/album.repository.ts +++ b/server/src/infra/repositories/album.repository.ts @@ -1,4 +1,4 @@ -import { AlbumAsset, AlbumAssets, AlbumInfoOptions, IAlbumRepository } from '@app/domain'; +import { AlbumAsset, AlbumAssetCount, AlbumAssets, AlbumInfoOptions, IAlbumRepository } from '@app/domain'; import { Injectable } from '@nestjs/common'; import { InjectDataSource, InjectRepository } from '@nestjs/typeorm'; import { DataSource, FindOptionsOrder, FindOptionsRelations, In, IsNull, Not, Repository } from 'typeorm'; @@ -56,10 +56,31 @@ export class AlbumRepository implements IAlbumRepository { ], relations: { owner: true, sharedUsers: true }, order: { createdAt: 'DESC' }, - relationLoadStrategy: 'query', }); } + async getAssetCountForIds(ids: string[]): Promise { + // Guard against running invalid query when ids list is empty. + if (!ids.length) { + return []; + } + + // Only possible with query builder because of GROUP BY. + const countByAlbums = await this.repository + .createQueryBuilder('album') + .select('album.id') + .addSelect('COUNT(albums_assets.assetsId)', 'asset_count') + .leftJoin('albums_assets_assets', 'albums_assets', 'albums_assets.albumsId = album.id') + .where('album.id IN (:...ids)', { ids }) + .groupBy('album.id') + .getRawMany(); + + return countByAlbums.map((albumCount) => ({ + albumId: albumCount['album_id'], + assetCount: Number(albumCount['asset_count']), + })); + } + /** * Returns the album IDs that have an invalid thumbnail, when: * - Thumbnail references an asset outside the album @@ -92,7 +113,6 @@ export class AlbumRepository implements IAlbumRepository { relations: { sharedUsers: true, sharedLinks: true, owner: true }, where: { ownerId }, order: { createdAt: 'DESC' }, - relationLoadStrategy: 'query', }); } @@ -108,7 +128,6 @@ export class AlbumRepository implements IAlbumRepository { { ownerId, sharedUsers: { id: Not(IsNull()) } }, ], order: { createdAt: 'DESC' }, - relationLoadStrategy: 'query', }); } @@ -120,7 +139,6 @@ export class AlbumRepository implements IAlbumRepository { relations: { sharedUsers: true, sharedLinks: true, owner: true }, where: { ownerId, sharedUsers: { id: IsNull() }, sharedLinks: { id: IsNull() } }, order: { createdAt: 'DESC' }, - relationLoadStrategy: 'query', }); } diff --git a/server/test/fixtures/album.stub.ts b/server/test/fixtures/album.stub.ts index ea7a3887e0..fd4464d191 100644 --- a/server/test/fixtures/album.stub.ts +++ b/server/test/fixtures/album.stub.ts @@ -19,9 +19,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), sharedWithUser: Object.freeze({ id: 'album-2', @@ -38,9 +35,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [userStub.user1], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), sharedWithMultiple: Object.freeze({ id: 'album-3', @@ -57,9 +51,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [userStub.user1, userStub.user2], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), sharedWithAdmin: Object.freeze({ id: 'album-3', @@ -76,9 +67,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [userStub.admin], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), oneAsset: Object.freeze({ id: 'album-4', @@ -95,9 +83,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: assetStub.image.fileCreatedAt, - endDate: assetStub.image.fileCreatedAt, - assetCount: 1, }), twoAssets: Object.freeze({ id: 'album-4a', @@ -114,9 +99,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: assetStub.withLocation.fileCreatedAt, - endDate: assetStub.image.fileCreatedAt, - assetCount: 2, }), emptyWithInvalidThumbnail: Object.freeze({ id: 'album-5', @@ -133,9 +115,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), emptyWithValidThumbnail: Object.freeze({ id: 'album-5', @@ -152,9 +131,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: null, - endDate: null, - assetCount: 0, }), oneAssetInvalidThumbnail: Object.freeze({ id: 'album-6', @@ -171,9 +147,6 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: assetStub.image.fileCreatedAt, - endDate: assetStub.image.fileCreatedAt, - assetCount: 1, }), oneAssetValidThumbnail: Object.freeze({ id: 'album-6', @@ -190,8 +163,5 @@ export const albumStub = { sharedLinks: [], sharedUsers: [], isActivityEnabled: true, - startDate: assetStub.image.fileCreatedAt, - endDate: assetStub.image.fileCreatedAt, - assetCount: 1, }), }; diff --git a/server/test/fixtures/shared-link.stub.ts b/server/test/fixtures/shared-link.stub.ts index ea34534616..56a0c10450 100644 --- a/server/test/fixtures/shared-link.stub.ts +++ b/server/test/fixtures/shared-link.stub.ts @@ -181,9 +181,6 @@ export const sharedLinkStub = { sharedUsers: [], sharedLinks: [], isActivityEnabled: true, - startDate: today, - endDate: today, - assetCount: 1, assets: [ { id: 'id_1', diff --git a/server/test/repositories/album.repository.mock.ts b/server/test/repositories/album.repository.mock.ts index a97393c0b0..7cd0a846b3 100644 --- a/server/test/repositories/album.repository.mock.ts +++ b/server/test/repositories/album.repository.mock.ts @@ -5,6 +5,7 @@ export const newAlbumRepositoryMock = (): jest.Mocked => { getById: jest.fn(), getByIds: jest.fn(), getByAssetId: jest.fn(), + getAssetCountForIds: jest.fn(), getInvalidThumbnail: jest.fn(), getOwned: jest.fn(), getShared: jest.fn(), diff --git a/web/src/lib/components/elements/table-header.svelte b/web/src/lib/components/elements/table-header.svelte index 0b68dd0e52..c89bff3db2 100644 --- a/web/src/lib/components/elements/table-header.svelte +++ b/web/src/lib/components/elements/table-header.svelte @@ -5,10 +5,10 @@ export let option: Sort; const handleSort = () => { - if (albumViewSettings === option.title) { + if (albumViewSettings === option.sortTitle) { option.sortDesc = !option.sortDesc; } else { - albumViewSettings = option.title; + albumViewSettings = option.sortTitle; } }; @@ -18,12 +18,12 @@ class="rounded-lg p-2 hover:bg-immich-dark-primary hover:dark:bg-immich-dark-primary/50" on:click={() => handleSort()} > - {#if albumViewSettings === option.title} + {#if albumViewSettings === option.sortTitle} {#if option.sortDesc} ↓ {:else} ↑ {/if} - {/if}{option.title} diff --git a/web/src/lib/components/sharedlinks-page/shared-link-card.svelte b/web/src/lib/components/sharedlinks-page/shared-link-card.svelte index 1dda332c2a..621c981326 100644 --- a/web/src/lib/components/sharedlinks-page/shared-link-card.svelte +++ b/web/src/lib/components/sharedlinks-page/shared-link-card.svelte @@ -7,7 +7,6 @@ import { createEventDispatcher } from 'svelte'; import { goto } from '$app/navigation'; import { mdiCircleEditOutline, mdiContentCopy, mdiDelete, mdiOpenInNew } from '@mdi/js'; - import noThumbnailUrl from '$lib/assets/no-thumbnail.png'; export let link: SharedLinkResponseDto; @@ -61,28 +60,18 @@ class="flex w-full gap-4 border-b border-gray-200 py-4 transition-all hover:border-immich-primary dark:border-gray-600 dark:text-immich-gray dark:hover:border-immich-dark-primary" >
- {#if (link?.album?.assetCount && link?.album?.assetCount > 0) || link.assets.length > 0} - {#await getAssetInfo()} - - {:then asset} - {asset.id} - {/await} - {:else} + {#await getAssetInfo()} + + {:then asset} {'Album - {/if} + {/await}
diff --git a/web/src/routes/(user)/albums/+page.svelte b/web/src/routes/(user)/albums/+page.svelte index f5b47b6bbe..3db0851689 100644 --- a/web/src/routes/(user)/albums/+page.svelte +++ b/web/src/routes/(user)/albums/+page.svelte @@ -1,6 +1,9 @@