fix: do not move faces of users other than the current owner (#31145)

server/src/queries/person.repository.sql

@@ -4,8 +4,11 @@
 update "asset_face"
 set
   "personGroupId" = $1
+from
+  "asset"
 where
-  "asset_face"."personGroupId" = $2
+  "asset_face"."assetId" = "asset"."id"
+  and "asset_face"."personGroupId" = $2
 
 -- PersonRepository.unassignFaces
 update "asset_face"

server/src/repositories/person.repository.ts

@@ -100,17 +100,15 @@ export class PersonRepository {
   async reassignFaces({ oldPersonGroupId, faceIds, ownerId, newPersonGroupId }: UpdateFacesData): Promise<number> {
     const result = await this.db
       .updateTable('asset_face')
+      .from('asset')
+      .whereRef('asset_face.assetId', '=', 'asset.id')
       .set({ personGroupId: newPersonGroupId })
       .$if(!!oldPersonGroupId, (qb) => qb.where('asset_face.personGroupId', '=', oldPersonGroupId!))
       .$if(!!faceIds, (qb) => qb.where('asset_face.id', 'in', faceIds!))
-      .$if(!!ownerId, (qb) =>
-        qb.where('asset_face.personGroupId', 'in', (eb) =>
-          eb.selectFrom('person').select('person.personGroupId').where('person.ownerId', '=', ownerId!),
-        ),
-      )
+      .$if(!!ownerId, (qb) => qb.where('asset.ownerId', '=', ownerId!))
       .executeTakeFirst();
 
-    return Number(result.numChangedRows ?? 0);
+    return Number(result.numUpdatedRows ?? 0);
   }
 
   @GenerateSql({ params: [{ sourceType: SourceType.MachineLearning, clusterGroupId: DummyValue.UUID }] })

server/src/services/person.service.ts

@@ -441,7 +441,6 @@ export class PersonService extends BaseService {
     const { waiting } = await this.jobRepository.getJobCounts(QueueName.FacialRecognition);
 
     if (force) {
-      console.log('unassigning faces');
       await this.personRepository.unassignFaces({ clusterGroupId, sourceType: SourceType.MachineLearning });
       await this.handlePersonCleanup();
       await this.personRepository.vacuum({ reindexVectors: false });

server/test/medium/specs/services/person.service.spec.ts

@@ -182,6 +182,8 @@ describe(PersonService.name, () => {
         ownerId: user2.id,
         personGroupId: person2.personGroupId,
       });
+      const { asset } = await ctx.newAsset({ ownerId: user2.id });
+      await ctx.newAssetFace({ assetId: asset.id, personGroupId: person2.personGroupId });
       storageMock.unlink.mockResolvedValue();
 
       const auth = factory.auth({ user: user1 });
@@ -191,6 +193,9 @@ describe(PersonService.name, () => {
       const user2People = await Array.fromAsync(ctx.get(PersonRepository).getAll({ ownerId: user2.id }));
       expect(user1People).toEqual([expect.objectContaining({ personGroupId: person1.personGroupId })]);
       expect(user2People).toEqual([expect.objectContaining({ personGroupId: person1.personGroupId })]);
+      await expect(ctx.get(PersonRepository).getFaces(asset.id, { viewingUserId: asset.ownerId })).resolves.toEqual([
+        expect.objectContaining({ personGroupId: person1.personGroupId }),
+      ]);
     });
 
     it('should skip people with a different name', async () => {
@@ -258,6 +263,33 @@ describe(PersonService.name, () => {
         ]),
       );
     });
+
+    it('should not merge into person another user does not have', async () => {
+      const { sut, ctx } = setup();
+      const storageMock = ctx.getMock(StorageRepository);
+      const { user: user1 } = await ctx.newUser();
+      const { user: user2 } = await ctx.newUser({ clusterGroupId: user1.clusterGroupId });
+      const { person: person1 } = await ctx.newPerson({ ownerId: user1.id });
+      const { person: person2 } = await ctx.newPerson({ ownerId: user1.id });
+      await ctx.newPerson({
+        ownerId: user2.id,
+        personGroupId: person2.personGroupId,
+      });
+      const { asset } = await ctx.newAsset({ ownerId: user2.id });
+      await ctx.newAssetFace({ assetId: asset.id, personGroupId: person2.personGroupId });
+      storageMock.unlink.mockResolvedValue();
+
+      const auth = factory.auth({ user: user1 });
+
+      await sut.mergePerson(auth, person1.personGroupId, { ids: [person2.personGroupId] });
+      const user1People = await Array.fromAsync(ctx.get(PersonRepository).getAll({ ownerId: user1.id }));
+      const user2People = await Array.fromAsync(ctx.get(PersonRepository).getAll({ ownerId: user2.id }));
+      expect(user1People).toEqual([expect.objectContaining({ personGroupId: person1.personGroupId })]);
+      expect(user2People).toEqual([expect.objectContaining({ personGroupId: person2.personGroupId })]);
+      await expect(ctx.get(PersonRepository).getFaces(asset.id, { viewingUserId: asset.ownerId })).resolves.toEqual([
+        expect.objectContaining({ personGroupId: person2.personGroupId }),
+      ]);
+    });
   });
 
   describe('createFace', () => {