chore: refactor more queries (#25572)

* refactor: asset service queries

* chore: refactor more queries

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

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();
+  }
 }

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

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);
 

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,

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)]);
     });
 

server/src/services/person.service.ts

@@ -128,10 +128,10 @@ export class PersonService extends BaseService {
   async getFacesById(auth: AuthDto, dto: FaceDto): Promise<AssetFaceResponseDto[]> {
     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[]) {

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

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',
+    });
   }
 }

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();

server/test/repositories/asset.repository.mock.ts

@@ -56,5 +56,8 @@ export const newAssetRepositoryMock = (): Mocked<RepositoryInterface<AssetReposi
     getForVideo: vitest.fn(),
     getForEdit: vitest.fn(),
     getForOcr: vitest.fn(),
+    getForMetadataExtractionTags: vitest.fn(),
+    getForFaces: vitest.fn(),
+    getForUpdateTags: vitest.fn(),
   };
 };