From 6ac3d8e4c6f9958a5699102d727e9d07fcbc55d8 Mon Sep 17 00:00:00 2001 From: Yaros Date: Fri, 12 Dec 2025 17:51:34 +0100 Subject: [PATCH] refactor: merge into one query --- server/src/queries/person.repository.sql | 69 ----------- server/src/repositories/person.repository.ts | 122 ++++++++----------- server/src/services/person.service.spec.ts | 45 ++++++- server/src/services/person.service.ts | 15 ++- 4 files changed, 100 insertions(+), 151 deletions(-) diff --git a/server/src/queries/person.repository.sql b/server/src/queries/person.repository.sql index d987bf3f3f..095e12a194 100644 --- a/server/src/queries/person.repository.sql +++ b/server/src/queries/person.repository.sql @@ -23,38 +23,6 @@ where limit 3 --- PersonRepository.getAllForUser -select - "person".* -from - "person" - inner join "asset_face" on "asset_face"."personId" = "person"."id" - inner join "asset" on "asset_face"."assetId" = "asset"."id" - and "asset"."visibility" = 'timeline' - and "asset"."deletedAt" is null -where - "person"."ownerId" = $1 - and "asset_face"."deletedAt" is null - and "person"."isHidden" = $2 -group by - "person"."id" -having - ( - "person"."name" != $3 - or count("asset_face"."assetId") >= $4 - ) -order by - "person"."isHidden" asc, - "person"."isFavorite" desc, - NULLIF(person.name, '') is null asc, - count("asset_face"."assetId") desc, - NULLIF(person.name, '') asc nulls last, - "person"."createdAt" -limit - $5 -offset - $6 - -- PersonRepository.getAllWithoutFaces select "person".* @@ -230,43 +198,6 @@ from where "asset_face"."deletedAt" is null --- PersonRepository.getNumberOfPeople -select - coalesce(count(*), 0) as "total", - coalesce( - count(*) filter ( - where - "isHidden" = $1 - ), - 0 - ) as "hidden" -from - "person" -where - exists ( - select - from - "asset_face" - where - "asset_face"."personId" = "person"."id" - and "asset_face"."deletedAt" is null - and exists ( - select - from - "asset" - where - "asset"."id" = "asset_face"."assetId" - and "asset"."visibility" = 'timeline' - and "asset"."deletedAt" is null - ) - having - ( - "person"."name" != $2 - or count("asset_face"."assetId") >= $3 - ) - ) - and "person"."ownerId" = $4 - -- PersonRepository.refreshFaces with "added_embeddings" as ( diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index cff4d8909e..39ba0fec1f 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -147,10 +147,9 @@ export class PersonRepository { } @GenerateSql({ params: [{ take: 1, skip: 0 }, DummyValue.UUID] }) - async getAllForUser(pagination: PaginationOptions, userId: string, options?: PersonSearchOptions) { - const items = await this.db + async getAllForUser(pagination: PaginationOptions, userId: string, options: PersonSearchOptions) { + const baseQuery = this.db .selectFrom('person') - .selectAll('person') .innerJoin('asset_face', 'asset_face.personId', 'person.id') .innerJoin('asset', (join) => join @@ -160,45 +159,62 @@ export class PersonRepository { ) .where('person.ownerId', '=', userId) .where('asset_face.deletedAt', 'is', null) - .orderBy('person.isHidden', 'asc') - .orderBy('person.isFavorite', 'desc') - .having((eb) => - eb.or([ - eb('person.name', '!=', ''), - eb((innerEb) => innerEb.fn.count('asset_face.assetId'), '>=', options?.minimumFaceCount || 1), - ]), - ) .groupBy('person.id') - .$if(!!options?.closestFaceAssetId, (qb) => - qb.orderBy((eb) => - eb( - (eb) => - eb - .selectFrom('face_search') - .select('face_search.embedding') - .whereRef('face_search.faceId', '=', 'person.faceAssetId'), - '<=>', - (eb) => - eb - .selectFrom('face_search') - .select('face_search.embedding') - .where('face_search.faceId', '=', options!.closestFaceAssetId!), - ), + .having((eb) => + eb.or([eb('person.name', '!=', ''), eb(eb.fn.count('asset_face.assetId'), '>=', options.minimumFaceCount)]), + ) + .selectAll('person') + .select((eb) => [ + eb.fn.count('asset_face.assetId').as('faceCount'), + + eb.fn.countAll().over().as('totalCount'), + + eb.fn + .sum(sql`CASE WHEN ${sql.ref('person.isHidden')} THEN 1 ELSE 0 END`) + .over() + .as('hiddenCount'), + ]); + + let sorted = baseQuery.orderBy('person.isHidden', 'asc').orderBy('person.isFavorite', 'desc'); + + if (options.closestFaceAssetId) { + const closestId = options.closestFaceAssetId!; + + sorted = sorted.orderBy((eb) => + eb( + (eb) => + eb + .selectFrom('face_search') + .select('face_search.embedding') + .whereRef('face_search.faceId', '=', 'person.faceAssetId'), + '<=>', + (eb) => + eb.selectFrom('face_search').select('face_search.embedding').where('face_search.faceId', '=', closestId), ), - ) - .$if(!options?.closestFaceAssetId, (qb) => - qb - .orderBy(sql`NULLIF(person.name, '') is null`, 'asc') - .orderBy((eb) => eb.fn.count('asset_face.assetId'), 'desc') - .orderBy(sql`NULLIF(person.name, '')`, (om) => om.asc().nullsLast()) - .orderBy('person.createdAt'), - ) - .$if(!options?.withHidden, (qb) => qb.where('person.isHidden', '=', false)) + ); + } else { + sorted = sorted + .orderBy(sql`NULLIF(person.name, '') IS NULL`, 'asc') + .orderBy((eb) => eb.fn.count('asset_face.assetId'), 'desc') + .orderBy(sql`NULLIF(person.name, '')`, (o) => o.asc().nullsLast()) + .orderBy('person.createdAt'); + } + + if (!options.withHidden) { + sorted = sorted.where('person.isHidden', '=', false); + } + + const rows = await sorted .offset(pagination.skip ?? 0) .limit(pagination.take + 1) .execute(); - return paginationHelper(items, pagination.take); + const total = rows[0]?.totalCount ?? 0; + const hidden = rows[0]?.hiddenCount ?? 0; + + const { items, hasNextPage } = paginationHelper(rows, pagination.take); + + return { items, hasNextPage, total, hidden }; } @GenerateSql() @@ -357,40 +373,6 @@ export class PersonRepository { }; } - @GenerateSql({ params: [DummyValue.UUID] }) - getNumberOfPeople(userId: string, options?: PersonSearchOptions) { - const zero = sql.lit(0); - return this.db - .selectFrom('person') - .where((eb) => - eb.exists((eb) => - eb - .selectFrom('asset_face') - .whereRef('asset_face.personId', '=', 'person.id') - .where('asset_face.deletedAt', 'is', null) - .having((eb) => - eb.or([ - eb('person.name', '!=', ''), - eb((innerEb) => innerEb.fn.count('asset_face.assetId'), '>=', options?.minimumFaceCount || 1), - ]), - ) - .where((eb) => - eb.exists((eb) => - eb - .selectFrom('asset') - .whereRef('asset.id', '=', 'asset_face.assetId') - .where('asset.visibility', '=', sql.lit(AssetVisibility.Timeline)) - .where('asset.deletedAt', 'is', null), - ), - ), - ), - ) - .where('person.ownerId', '=', userId) - .select((eb) => eb.fn.coalesce(eb.fn.countAll(), zero).as('total')) - .select((eb) => eb.fn.coalesce(eb.fn.countAll().filterWhere('isHidden', '=', true), zero).as('hidden')) - .executeTakeFirstOrThrow(); - } - create(person: Insertable) { return this.db.insertInto('person').values(person).returningAll().executeTakeFirstOrThrow(); } diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 41c44ea476..32a600e383 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -70,11 +70,28 @@ describe(PersonService.name, () => { describe('getAll', () => { it('should get all hidden and visible people with thumbnails', async () => { + // Updated stubs with the required aggregated fields + const personWithCounts = { + ...personStub.withName, + faceCount: 3, + totalCount: 2, + hiddenCount: 1, + }; + + const hiddenPersonWithCounts = { + ...personStub.hidden, + faceCount: 1, + totalCount: 2, + hiddenCount: 1, + }; + mocks.person.getAllForUser.mockResolvedValue({ - items: [personStub.withName, personStub.hidden], + items: [personWithCounts, hiddenPersonWithCounts], hasNextPage: false, + total: 2, + hidden: 1, }); - mocks.person.getNumberOfPeople.mockResolvedValue({ total: 2, hidden: 1 }); + await expect(sut.getAll(authStub.admin, { withHidden: true, page: 1, size: 10 })).resolves.toEqual({ hasNextPage: false, total: 2, @@ -93,18 +110,36 @@ describe(PersonService.name, () => { }, ], }); + expect(mocks.person.getAllForUser).toHaveBeenCalledWith({ skip: 0, take: 10 }, authStub.admin.user.id, { minimumFaceCount: 3, withHidden: true, + closestFaceAssetId: undefined, }); }); it('should get all visible people and favorites should be first in the array', async () => { + const favoritePersonWithCounts = { + ...personStub.isFavorite, + faceCount: 3, + totalCount: 2, + hiddenCount: 1, + }; + + const namedPersonWithCounts = { + ...personStub.withName, + faceCount: 3, + totalCount: 2, + hiddenCount: 1, + }; + mocks.person.getAllForUser.mockResolvedValue({ - items: [personStub.isFavorite, personStub.withName], + items: [favoritePersonWithCounts, namedPersonWithCounts], hasNextPage: false, + total: 2, + hidden: 1, }); - mocks.person.getNumberOfPeople.mockResolvedValue({ total: 2, hidden: 1 }); + await expect(sut.getAll(authStub.admin, { withHidden: false, page: 1, size: 10 })).resolves.toEqual({ hasNextPage: false, total: 2, @@ -123,9 +158,11 @@ describe(PersonService.name, () => { responseDto, ], }); + expect(mocks.person.getAllForUser).toHaveBeenCalledWith({ skip: 0, take: 10 }, authStub.admin.user.id, { minimumFaceCount: 3, withHidden: false, + closestFaceAssetId: undefined, }); }); }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 87278b8291..4f30d0660a 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -48,12 +48,13 @@ import { isFacialRecognitionEnabled } from 'src/utils/misc'; export class PersonService extends BaseService { async getAll(auth: AuthDto, dto: PersonSearchDto): Promise { const { withHidden = false, closestAssetId, closestPersonId, page, size } = dto; - let closestFaceAssetId = closestAssetId; + const pagination = { take: size, skip: (page - 1) * size, }; + let closestFaceAssetId = closestAssetId; if (closestPersonId) { const person = await this.personRepository.getById(closestPersonId); if (!person?.faceAssetId) { @@ -61,22 +62,20 @@ export class PersonService extends BaseService { } closestFaceAssetId = person.faceAssetId; } + const { machineLearning } = await this.getConfig({ withCache: false }); - const { items, hasNextPage } = await this.personRepository.getAllForUser(pagination, auth.user.id, { + + const { items, hasNextPage, total, hidden } = await this.personRepository.getAllForUser(pagination, auth.user.id, { minimumFaceCount: machineLearning.facialRecognition.minFaces, withHidden, closestFaceAssetId, }); - const { total, hidden } = await this.personRepository.getNumberOfPeople(auth.user.id, { - minimumFaceCount: machineLearning.facialRecognition.minFaces, - withHidden, - }); return { people: items.map((person) => mapPerson(person)), hasNextPage, - total, - hidden, + total: Number(total), + hidden: Number(hidden), }; }