From 70f9d4ae02bc42a08d5c10257149e45c1ad944f3 Mon Sep 17 00:00:00 2001 From: Yaros Date: Fri, 24 Jul 2026 16:25:26 +0200 Subject: [PATCH] fix: minFaces not filtering correctly --- .../repositories/people.repository.dart | 6 ++- .../repositories/people_repository_test.dart | 49 +++++++++++++++++++ server/src/queries/person.repository.sql | 4 +- server/src/repositories/person.repository.ts | 4 +- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/mobile/lib/infrastructure/repositories/people.repository.dart b/mobile/lib/infrastructure/repositories/people.repository.dart index 0dd73cc81d..c365c3d6a6 100644 --- a/mobile/lib/infrastructure/repositories/people.repository.dart +++ b/mobile/lib/infrastructure/repositories/people.repository.dart @@ -50,10 +50,12 @@ class DriftPeopleRepository extends DriftDatabaseRepository { faces.isVisible.equals(true) & faces.deletedAt.isNull(), ) - ..groupBy([people.id], having: faces.id.count().isBiggerOrEqualValue(minFaces) | people.name.equals('').not()) + ..groupBy([ + people.id, + ], having: faces.assetId.count(distinct: true).isBiggerOrEqualValue(minFaces) | people.name.equals('').not()) ..orderBy([ OrderingTerm(expression: people.name.equals('').not(), mode: OrderingMode.desc), - OrderingTerm(expression: faces.id.count(), mode: OrderingMode.desc), + OrderingTerm(expression: faces.assetId.count(distinct: true), mode: OrderingMode.desc), ]); return query.map((row) { diff --git a/mobile/test/medium/repositories/people_repository_test.dart b/mobile/test/medium/repositories/people_repository_test.dart index 4b51862a7d..bd187ae8f2 100644 --- a/mobile/test/medium/repositories/people_repository_test.dart +++ b/mobile/test/medium/repositories/people_repository_test.dart @@ -74,4 +74,53 @@ void main() { expect(people, isEmpty); }); }); + + group('getAllPeople', () { + test('counts distinct assets, not face records, against minFaces', () async { + // Regression check: a person can have multiple face records on the same asset + // (e.g., metadata import + ML detection), which must not inflate the count used + // to compare against minFaces. An unnamed person with 2 distinct photos but 3 + // face records (2 of them on the same photo) must not pass a minFaces of 3. + final user = await ctx.newUser(); + final asset1 = await ctx.newRemoteAsset(ownerId: user.id); + final asset2 = await ctx.newRemoteAsset(ownerId: user.id); + + final person = await ctx.newPerson(ownerId: user.id, name: ''); + await ctx.newFace(assetId: asset1.id, personId: person.id); + await ctx.newFace(assetId: asset1.id, personId: person.id); + await ctx.newFace(assetId: asset2.id, personId: person.id); + + final people = await sut.getAllPeople(minFaces: 3); + + expect(people, isEmpty); + }); + + test('returns unnamed people who meet minFaces based on distinct assets', () async { + final user = await ctx.newUser(); + final asset1 = await ctx.newRemoteAsset(ownerId: user.id); + final asset2 = await ctx.newRemoteAsset(ownerId: user.id); + final asset3 = await ctx.newRemoteAsset(ownerId: user.id); + + final person = await ctx.newPerson(ownerId: user.id, name: ''); + await ctx.newFace(assetId: asset1.id, personId: person.id); + await ctx.newFace(assetId: asset2.id, personId: person.id); + await ctx.newFace(assetId: asset3.id, personId: person.id); + + final people = await sut.getAllPeople(minFaces: 3); + + expect(people.map((p) => p.id), [person.id]); + }); + + test('always returns named people regardless of minFaces', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + + final person = await ctx.newPerson(ownerId: user.id, name: 'Jane'); + await ctx.newFace(assetId: asset.id, personId: person.id); + + final people = await sut.getAllPeople(minFaces: 3); + + expect(people.map((p) => p.id), [person.id]); + }); + }); } diff --git a/server/src/queries/person.repository.sql b/server/src/queries/person.repository.sql index a2f3f64442..3056ddaa69 100644 --- a/server/src/queries/person.repository.sql +++ b/server/src/queries/person.repository.sql @@ -42,7 +42,7 @@ group by having ( "person"."name" != $3 - or count("asset_face"."assetId") >= COALESCE( + or count(distinct ("asset_face"."assetId")) >= COALESCE( ( SELECT value -> 'people' ->> 'minimumFaces' @@ -59,7 +59,7 @@ order by "person"."isHidden" asc, "person"."isFavorite" desc, NULLIF(person.name, '') is null asc, - count("asset_face"."assetId") desc, + count(distinct ("asset_face"."assetId")) desc, NULLIF(person.name, '') asc nulls last, "person"."createdAt" limit diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index 0db03a18c7..17e08dff14 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -168,7 +168,7 @@ export class PersonRepository { eb.or([ eb('person.name', '!=', ''), eb( - (innerEb) => innerEb.fn.count('asset_face.assetId'), + (innerEb) => innerEb.fn.count(innerEb.fn('distinct', ['asset_face.assetId'])), '>=', sql`COALESCE( (SELECT value -> 'people' ->> 'minimumFaces' @@ -201,7 +201,7 @@ export class PersonRepository { .$if(!options?.closestFaceAssetId, (qb) => qb .orderBy(sql`NULLIF(person.name, '') is null`, 'asc') - .orderBy((eb) => eb.fn.count('asset_face.assetId'), 'desc') + .orderBy((eb) => eb.fn.count(eb.fn('distinct', ['asset_face.assetId'])), 'desc') .orderBy(sql`NULLIF(person.name, '')`, (om) => om.asc().nullsLast()) .orderBy('person.createdAt'), )