From db4e7abf6d095c8d91d59e9f7ddbdd9f11dada1f Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Thu, 19 Feb 2026 16:48:30 +0100 Subject: [PATCH] chore: refactor more queries (#25572) * refactor: asset service queries * chore: refactor more queries --- server/src/queries/asset.repository.sql | 54 ++++++++++++++++ server/src/repositories/asset.repository.ts | 38 ++++++++++++ server/src/services/asset.service.ts | 8 ++- server/src/services/metadata.service.spec.ts | 62 +++++++------------ server/src/services/metadata.service.ts | 4 +- server/src/services/person.service.spec.ts | 2 +- server/src/services/person.service.ts | 6 +- server/src/services/tag.service.spec.ts | 13 ++-- server/src/services/tag.service.ts | 9 ++- .../specs/services/asset.service.spec.ts | 51 +++++++++++++++ .../repositories/asset.repository.mock.ts | 3 + 11 files changed, 188 insertions(+), 62 deletions(-) diff --git a/server/src/queries/asset.repository.sql b/server/src/queries/asset.repository.sql index 01c580fb13..4b8323cd59 100644 --- a/server/src/queries/asset.repository.sql +++ b/server/src/queries/asset.repository.sql @@ -677,3 +677,57 @@ from inner join "asset_exif" on "asset_exif"."assetId" = "asset"."id" where "asset"."id" = $1 + +-- AssetRepository.getForMetadataExtractionTags +select + "asset_exif"."tags" +from + "asset_exif" +where + "asset_exif"."assetId" = $1 + +-- AssetRepository.getForFaces +select + "asset_exif"."exifImageHeight", + "asset_exif"."exifImageWidth", + "asset_exif"."orientation", + ( + select + coalesce(json_agg(agg), '[]') + from + ( + select + "asset_edit"."action", + "asset_edit"."parameters" + from + "asset_edit" + where + "asset_edit"."assetId" = "asset"."id" + ) as agg + ) as "edits" +from + "asset" + inner join "asset_exif" on "asset_exif"."assetId" = "asset"."id" +where + "asset"."id" = $1 + +-- AssetRepository.getForUpdateTags +select + ( + select + coalesce(json_agg(agg), '[]') + from + ( + select + "tag"."value" + from + "tag" + inner join "tag_asset" on "tag"."id" = "tag_asset"."tagId" + where + "asset"."id" = "tag_asset"."assetId" + ) as agg + ) as "tags" +from + "asset" +where + "asset"."id" = $1 diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 3165aed023..e424d4e0b8 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { ExpressionBuilder, Insertable, Kysely, NotNull, Selectable, sql, Updateable, UpdateResult } from 'kysely'; +import { jsonArrayFrom } from 'kysely/helpers/postgres'; import { isEmpty, isUndefined, omitBy } from 'lodash'; import { InjectKysely } from 'nestjs-kysely'; import { LockableProperty, Stack } from 'src/database'; @@ -1086,4 +1087,41 @@ export class AssetRepository { ]) .executeTakeFirst(); } + + @GenerateSql({ params: [DummyValue.UUID] }) + async getForMetadataExtractionTags(id: string) { + return this.db + .selectFrom('asset_exif') + .select('asset_exif.tags') + .where('asset_exif.assetId', '=', id) + .executeTakeFirst(); + } + + @GenerateSql({ params: [DummyValue.UUID] }) + async getForFaces(id: string) { + return this.db + .selectFrom('asset') + .innerJoin('asset_exif', (join) => join.onRef('asset_exif.assetId', '=', 'asset.id')) + .select(['asset_exif.exifImageHeight', 'asset_exif.exifImageWidth', 'asset_exif.orientation']) + .select(withEdits) + .where('asset.id', '=', id) + .executeTakeFirstOrThrow(); + } + + @GenerateSql({ params: [DummyValue.UUID] }) + async getForUpdateTags(id: string) { + return this.db + .selectFrom('asset') + .select((eb) => + jsonArrayFrom( + eb + .selectFrom('tag') + .select('tag.value') + .innerJoin('tag_asset', 'tag.id', 'tag_asset.tagId') + .whereRef('asset.id', '=', 'tag_asset.assetId'), + ).as('tags'), + ) + .where('asset.id', '=', id) + .executeTakeFirstOrThrow(); + } } diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index bda2d122fe..f6098248ed 100644 --- a/server/src/services/asset.service.ts +++ b/server/src/services/asset.service.ts @@ -580,11 +580,17 @@ export class AssetService extends BaseService { throw new BadRequestException('Editing SVG images is not supported'); } + // check that crop parameters will not go out of bounds + const { width: assetWidth, height: assetHeight } = getDimensions(asset); + + if (!assetWidth || !assetHeight) { + throw new BadRequestException('Asset dimensions are not available for editing'); + } + const cropIndex = dto.edits.findIndex((e) => e.action === AssetEditAction.Crop); if (cropIndex > 0) { throw new BadRequestException('Crop action must be the first edit action'); } - const crop = cropIndex === -1 ? null : (dto.edits[cropIndex] as AssetEditActionCrop); if (crop) { // check that crop parameters will not go out of bounds diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 7a5d5037fe..1080407922 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -382,11 +382,9 @@ describe(MetadataService.name, () => { }); it('should extract tags from TagsList', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent'] }); mockReadTags({ TagsList: ['Parent'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -396,11 +394,9 @@ describe(MetadataService.name, () => { }); it('should extract hierarchy from TagsList', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent/Child'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent/Child'] }); mockReadTags({ TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.childUpsert); @@ -420,11 +416,9 @@ describe(MetadataService.name, () => { }); it('should extract tags from Keywords as a string', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent'] }); mockReadTags({ Keywords: 'Parent' }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -434,11 +428,9 @@ describe(MetadataService.name, () => { }); it('should extract tags from Keywords as a list', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent'] }); mockReadTags({ Keywords: ['Parent'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -448,11 +440,9 @@ describe(MetadataService.name, () => { }); it('should extract tags from Keywords as a list with a number', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent', '2024'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent', '2024'] }); mockReadTags({ Keywords: ['Parent', 2024] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -463,11 +453,9 @@ describe(MetadataService.name, () => { }); it('should extract hierarchal tags from Keywords', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent/Child'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent/Child'] }); mockReadTags({ Keywords: 'Parent/Child' }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -485,11 +473,9 @@ describe(MetadataService.name, () => { }); it('should ignore Keywords when TagsList is present', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent/Child', 'Child'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent/Child', 'Child'] }); mockReadTags({ Keywords: 'Child', TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -508,11 +494,9 @@ describe(MetadataService.name, () => { }); it('should extract hierarchy from HierarchicalSubject', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent/Child', 'TagA'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent/Child', 'TagA'] }); mockReadTags({ HierarchicalSubject: ['Parent|Child', 'TagA'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.childUpsert); @@ -537,11 +521,9 @@ describe(MetadataService.name, () => { }); it('should extract tags from HierarchicalSubject as a list with a number', async () => { - const asset = AssetFactory.from() - .exif({ tags: ['Parent', '2024'] }) - .build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent', '2024'] }); mockReadTags({ HierarchicalSubject: ['Parent', 2024] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); @@ -554,7 +536,7 @@ describe(MetadataService.name, () => { it('should extract ignore / characters in a HierarchicalSubject tag', async () => { const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue({ ...factory.asset(), exifInfo: factory.exif({ tags: ['Mom|Dad'] }) }); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Mom|Dad'] }); mockReadTags({ HierarchicalSubject: ['Mom/Dad'] }); mocks.tag.upsertValue.mockResolvedValueOnce(tagStub.parentUpsert); @@ -568,11 +550,9 @@ describe(MetadataService.name, () => { }); it('should ignore HierarchicalSubject when TagsList is present', async () => { - const baseAsset = AssetFactory.from(); - const asset = baseAsset.build(); - const updatedAsset = baseAsset.exif({ tags: ['Parent/Child', 'Parent2/Child2'] }).build(); + const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(asset); - mocks.asset.getById.mockResolvedValue(updatedAsset); + mocks.asset.getForMetadataExtractionTags.mockResolvedValue({ tags: ['Parent/Child', 'Parent2/Child2'] }); mockReadTags({ HierarchicalSubject: ['Parent2|Child2'], TagsList: ['Parent/Child'] }); mocks.tag.upsertValue.mockResolvedValue(tagStub.parentUpsert); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index dac79343e0..3937237ff6 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -590,10 +590,10 @@ export class MetadataService extends BaseService { } private async applyTagList({ id, ownerId }: { id: string; ownerId: string }) { - const asset = await this.assetRepository.getById(id, { exifInfo: true }); + const asset = await this.assetRepository.getForMetadataExtractionTags(id); const results = await upsertTags(this.tagRepository, { userId: ownerId, - tags: asset?.exifInfo?.tags ?? [], + tags: asset?.tags ?? [], }); await this.tagRepository.replaceAssetTags( id, diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 4b60cd8e7f..d7c9fa9f59 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -331,7 +331,7 @@ describe(PersonService.name, () => { const asset = AssetFactory.from({ id: face.assetId }).exif().build(); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset.id])); mocks.person.getFaces.mockResolvedValue([face]); - mocks.asset.getById.mockResolvedValue(asset); + mocks.asset.getForFaces.mockResolvedValue({ edits: [], ...asset.exifInfo }); await expect(sut.getFacesById(auth, { id: face.assetId })).resolves.toStrictEqual([mapFaces(face, auth)]); }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index ea7b3f9e78..090b358223 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -128,10 +128,10 @@ export class PersonService extends BaseService { async getFacesById(auth: AuthDto, dto: FaceDto): Promise { await this.requireAccess({ auth, permission: Permission.AssetRead, ids: [dto.id] }); const faces = await this.personRepository.getFaces(dto.id); - const asset = await this.assetRepository.getById(dto.id, { edits: true, exifInfo: true }); - const assetDimensions = getDimensions(asset!.exifInfo!); + const asset = await this.assetRepository.getForFaces(dto.id); + const assetDimensions = getDimensions(asset); - return faces.map((face) => mapFaces(face, auth, asset!.edits!, assetDimensions)); + return faces.map((face) => mapFaces(face, auth, asset.edits, assetDimensions)); } async createNewFeaturePhoto(changeFeaturePhoto: string[]) { diff --git a/server/src/services/tag.service.spec.ts b/server/src/services/tag.service.spec.ts index f42f40940d..6fc472bb87 100644 --- a/server/src/services/tag.service.spec.ts +++ b/server/src/services/tag.service.spec.ts @@ -4,7 +4,6 @@ import { JobStatus } from 'src/enum'; import { TagService } from 'src/services/tag.service'; import { authStub } from 'test/fixtures/auth.stub'; import { tagResponseStub, tagStub } from 'test/fixtures/tag.stub'; -import { factory } from 'test/small.factory'; import { newTestService, ServiceMocks } from 'test/utils'; describe(TagService.name, () => { @@ -192,10 +191,7 @@ describe(TagService.name, () => { it('should upsert records', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set(['tag-1', 'tag-2'])); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3'])); - mocks.asset.getById.mockResolvedValue({ - ...factory.asset(), - tags: [factory.tag({ value: 'tag-1' }), factory.tag({ value: 'tag-2' })], - }); + mocks.asset.getForUpdateTags.mockResolvedValue({ tags: [{ value: 'tag-1' }, { value: 'tag-2' }] }); mocks.tag.upsertAssetIds.mockResolvedValue([ { tagId: 'tag-1', assetId: 'asset-1' }, { tagId: 'tag-1', assetId: 'asset-2' }, @@ -246,10 +242,7 @@ describe(TagService.name, () => { mocks.tag.get.mockResolvedValue(tagStub.tag); mocks.tag.getAssetIds.mockResolvedValue(new Set(['asset-1'])); mocks.tag.addAssetIds.mockResolvedValue(); - mocks.asset.getById.mockResolvedValue({ - ...factory.asset(), - tags: [factory.tag({ value: 'tag-1' })], - }); + mocks.asset.getForUpdateTags.mockResolvedValue({ tags: [{ value: 'tag-1' }] }); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-2'])); await expect( @@ -278,6 +271,7 @@ describe(TagService.name, () => { it('should throw an error for an invalid id', async () => { mocks.tag.getAssetIds.mockResolvedValue(new Set()); mocks.tag.removeAssetIds.mockResolvedValue(); + mocks.asset.getForUpdateTags.mockResolvedValue({ tags: [] }); await expect(sut.removeAssets(authStub.admin, 'tag-1', { ids: ['asset-1'] })).resolves.toEqual([ { id: 'asset-1', success: false, error: 'not_found' }, @@ -288,6 +282,7 @@ describe(TagService.name, () => { mocks.tag.get.mockResolvedValue(tagStub.tag); mocks.tag.getAssetIds.mockResolvedValue(new Set(['asset-1'])); mocks.tag.removeAssetIds.mockResolvedValue(); + mocks.asset.getForUpdateTags.mockResolvedValue({ tags: [] }); await expect( sut.removeAssets(authStub.admin, 'tag-1', { diff --git a/server/src/services/tag.service.ts b/server/src/services/tag.service.ts index 20303421c1..d34cd84ecd 100644 --- a/server/src/services/tag.service.ts +++ b/server/src/services/tag.service.ts @@ -151,10 +151,9 @@ export class TagService extends BaseService { } private async updateTags(assetId: string) { - const asset = await this.assetRepository.getById(assetId, { tags: true }); - await this.assetRepository.upsertExif( - updateLockedColumns({ assetId, tags: asset?.tags?.map(({ value }) => value) ?? [] }), - { lockedPropertiesBehavior: 'append' }, - ); + const { tags } = await this.assetRepository.getForUpdateTags(assetId); + await this.assetRepository.upsertExif(updateLockedColumns({ assetId, tags: tags.map(({ value }) => value) }), { + lockedPropertiesBehavior: 'append', + }); } } diff --git a/server/test/medium/specs/services/asset.service.spec.ts b/server/test/medium/specs/services/asset.service.spec.ts index d97551d154..477414dafe 100644 --- a/server/test/medium/specs/services/asset.service.spec.ts +++ b/server/test/medium/specs/services/asset.service.spec.ts @@ -641,6 +641,57 @@ describe(AssetService.name, () => { }); }); + describe('getOcr', () => { + it('should require access', async () => { + const { sut, ctx } = setup(); + const { user } = await ctx.newUser(); + const { user: user2 } = await ctx.newUser(); + const auth = factory.auth({ user }); + const { asset } = await ctx.newAsset({ ownerId: user2.id }); + + await expect(sut.getOcr(auth, asset.id)).rejects.toThrow('Not found or no asset.read access'); + }); + + it('should work', async () => { + const { sut, ctx } = setup(); + const { user } = await ctx.newUser(); + const auth = factory.auth({ user }); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + await ctx.newExif({ assetId: asset.id, exifImageHeight: 42, exifImageWidth: 69, orientation: '1' }); + ctx.getMock(OcrRepository).getByAssetId.mockResolvedValue([factory.assetOcr()]); + + await expect(sut.getOcr(auth, asset.id)).resolves.toEqual([ + expect.objectContaining({ x1: 0.1, x2: 0.3, x3: 0.3, x4: 0.1, y1: 0.2, y2: 0.2, y3: 0.4, y4: 0.4 }), + ]); + }); + + it('should apply rotation', async () => { + const { sut, ctx } = setup(); + const { user } = await ctx.newUser(); + const auth = factory.auth({ user }); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + await ctx.newExif({ assetId: asset.id, exifImageHeight: 42, exifImageWidth: 69, orientation: '1' }); + await ctx.database + .insertInto('asset_edit') + .values({ assetId: asset.id, action: AssetEditAction.Rotate, parameters: { angle: 90 }, sequence: 1 }) + .execute(); + ctx.getMock(OcrRepository).getByAssetId.mockResolvedValue([factory.assetOcr()]); + + await expect(sut.getOcr(auth, asset.id)).resolves.toEqual([ + expect.objectContaining({ + x1: 0.6, + x2: 0.8, + x3: 0.8, + x4: 0.6, + y1: expect.any(Number), + y2: expect.any(Number), + y3: 0.3, + y4: 0.3, + }), + ]); + }); + }); + describe('upsertBulkMetadata', () => { it('should work', async () => { const { sut, ctx } = setup(); diff --git a/server/test/repositories/asset.repository.mock.ts b/server/test/repositories/asset.repository.mock.ts index b1ced1f874..68667fa109 100644 --- a/server/test/repositories/asset.repository.mock.ts +++ b/server/test/repositories/asset.repository.mock.ts @@ -56,5 +56,8 @@ export const newAssetRepositoryMock = (): Mocked