diff --git a/mobile/lib/infrastructure/repositories/people.repository.dart b/mobile/lib/infrastructure/repositories/people.repository.dart index cb32279a07..0dd73cc81d 100644 --- a/mobile/lib/infrastructure/repositories/people.repository.dart +++ b/mobile/lib/infrastructure/repositories/people.repository.dart @@ -16,20 +16,21 @@ class DriftPeopleRepository extends DriftDatabaseRepository { } Future> getAssetPeople(String assetId) async { - final query = - _db.select(_db.assetFaceEntity).join([ - innerJoin(_db.personEntity, _db.personEntity.id.equalsExp(_db.assetFaceEntity.personId)), - ])..where( - _db.assetFaceEntity.assetId.equals(assetId) & - _db.assetFaceEntity.isVisible.equals(true) & - _db.assetFaceEntity.deletedAt.isNull() & - _db.personEntity.isHidden.equals(false), - ); + // An asset can have multiple face records for the same person (e.g., metadata + // imports alongside ML detections). Use a subquery instead of a join so each + // person is returned once, regardless of how many of their faces are on the asset + final faceQuery = _db.assetFaceEntity.selectOnly() + ..addColumns([_db.assetFaceEntity.personId]) + ..where( + _db.assetFaceEntity.assetId.equals(assetId) & + _db.assetFaceEntity.isVisible.equals(true) & + _db.assetFaceEntity.deletedAt.isNull(), + ); - return query.map((row) { - final person = row.readTable(_db.personEntity); - return person.toDto(); - }).get(); + final query = _db.select(_db.personEntity) + ..where((row) => row.id.isInQuery(faceQuery) & row.isHidden.equals(false)); + + return query.map((row) => row.toDto()).get(); } Future> getAllPeople({int minFaces = 3}) async { diff --git a/mobile/test/medium/repositories/people_repository_test.dart b/mobile/test/medium/repositories/people_repository_test.dart new file mode 100644 index 0000000000..4b51862a7d --- /dev/null +++ b/mobile/test/medium/repositories/people_repository_test.dart @@ -0,0 +1,77 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/infrastructure/repositories/people.repository.dart'; + +import '../repository_context.dart'; + +void main() { + late MediumRepositoryContext ctx; + late DriftPeopleRepository sut; + + setUp(() { + ctx = MediumRepositoryContext(); + sut = DriftPeopleRepository(ctx.db); + }); + + tearDown(() async { + await ctx.dispose(); + }); + + group('getAssetPeople', () { + test('does not duplicate a person with multiple face records on the same asset', () async { + // Regression check for #20585: a join on asset_face_entity returned one row + // per face, so a person appeared twice in the asset details panel when the + // same face was on the asset more than once (e.g., metadata import + ML) + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + + final person = await ctx.newPerson(ownerId: user.id); + await ctx.newFace(assetId: asset.id, personId: person.id); + await ctx.newFace(assetId: asset.id, personId: person.id); + + final people = await sut.getAssetPeople(asset.id); + + expect(people, hasLength(1)); + expect(people.single.id, person.id); + }); + + test('returns all distinct people of an asset', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + + final person1 = await ctx.newPerson(ownerId: user.id); + final person2 = await ctx.newPerson(ownerId: user.id); + await ctx.newFace(assetId: asset.id, personId: person1.id); + await ctx.newFace(assetId: asset.id, personId: person2.id); + + final people = await sut.getAssetPeople(asset.id); + + expect(people, hasLength(2)); + expect(people.map((person) => person.id), containsAll([person1.id, person2.id])); + }); + + test('does not return hidden people', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + + final hidden = await ctx.newPerson(ownerId: user.id, isHidden: true); + await ctx.newFace(assetId: asset.id, personId: hidden.id); + + final people = await sut.getAssetPeople(asset.id); + + expect(people, isEmpty); + }); + + test('does not return people from other assets', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + final otherAsset = await ctx.newRemoteAsset(ownerId: user.id); + + final person = await ctx.newPerson(ownerId: user.id); + await ctx.newFace(assetId: otherAsset.id, personId: person.id); + + final people = await sut.getAssetPeople(asset.id); + + expect(people, isEmpty); + }); + }); +}