From 7b51c50a96cf86f918f84820f666eee02d713394 Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:10:25 +0200 Subject: [PATCH] feat: people merge improvements (#31456) --- open-api/immich-openapi-specs.json | 72 +++++++++++- packages/sdk/src/fetch-client.ts | 25 +++- server/src/controllers/person.controller.ts | 23 +++- server/src/services/person.service.ts | 111 ++++++++++-------- .../specs/services/person.service.spec.ts | 15 +-- .../modals/PersonMergeSuggestionModal.svelte | 7 +- .../[[assetId=id]]/MergeFaceSelector.svelte | 7 +- 7 files changed, 180 insertions(+), 80 deletions(-) diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index a8028f6bc..d15a8b019 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -9984,6 +9984,65 @@ "x-immich-state": "Stable" } }, + "/people/merge": { + "post": { + "description": "Merge an ordered list of people together into a single person. The final name and birth date are always the first defined value, following the order. Also automatically merges people for other users in the cluster group, skipping people that would result in overriding a previously set name or birth date.", + "operationId": "mergePeople", + "parameters": [], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/MergePersonDto" + } + } + }, + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "items": { + "$ref": "#/components/schemas/BulkIdResponseDto" + }, + "type": "array" + } + } + }, + "description": "" + } + }, + "security": [ + { + "bearer": [] + }, + { + "cookie": [] + }, + { + "api_key": [] + } + ], + "summary": "Merge people", + "tags": [ + "People" + ], + "x-immich-history": [ + { + "version": "v3.2.1", + "state": "Added" + }, + { + "version": "v3.2.1", + "state": "Stable" + } + ], + "x-immich-permission": "person.merge", + "x-immich-state": "Stable" + } + }, "/people/{id}": { "delete": { "description": "Delete an individual person.", @@ -10175,8 +10234,9 @@ }, "/people/{id}/merge": { "post": { + "deprecated": true, "description": "Merge a list of people into the person specified in the path parameter.", - "operationId": "mergePerson", + "operationId": "mergePersonLegacy", "parameters": [ { "name": "id", @@ -10227,7 +10287,8 @@ ], "summary": "Merge people", "tags": [ - "People" + "People", + "Deprecated" ], "x-immich-history": [ { @@ -10241,10 +10302,15 @@ { "version": "v2", "state": "Stable" + }, + { + "version": "v3.2.1", + "state": "Deprecated", + "replacementId": "mergePeople" } ], "x-immich-permission": "person.merge", - "x-immich-state": "Stable" + "x-immich-state": "Deprecated" } }, "/people/{id}/reassign": { diff --git a/packages/sdk/src/fetch-client.ts b/packages/sdk/src/fetch-client.ts index ec0a1de6a..11c041ab6 100644 --- a/packages/sdk/src/fetch-client.ts +++ b/packages/sdk/src/fetch-client.ts @@ -2001,6 +2001,10 @@ export type PeopleUpdateDto = { /** People to update */ people: PeopleUpdateItem[]; }; +export type MergePersonDto = { + /** Person IDs to merge */ + ids: string[]; +}; export type PersonUpdateDto = { /** Person date of birth */ birthDate?: string | null; @@ -2015,10 +2019,6 @@ export type PersonUpdateDto = { /** Person name */ name?: string; }; -export type MergePersonDto = { - /** Person IDs to merge */ - ids: string[]; -}; export type AssetFaceUpdateItem = { /** Asset ID */ assetId: string; @@ -6041,6 +6041,21 @@ export function updatePeople({ peopleUpdateDto }: { body: peopleUpdateDto }))); } +/** + * Merge people + */ +export function mergePeople({ mergePersonDto }: { + mergePersonDto: MergePersonDto; +}, opts?: Oazapfts.RequestOpts) { + return oazapfts.ok(oazapfts.fetchJson<{ + status: 200; + data: BulkIdResponseDto[]; + }>("/people/merge", oazapfts.json({ + ...opts, + method: "POST", + body: mergePersonDto + }))); +} /** * Delete person */ @@ -6084,7 +6099,7 @@ export function updatePerson({ id, personUpdateDto }: { /** * Merge people */ -export function mergePerson({ id, mergePersonDto }: { +export function mergePersonLegacy({ id, mergePersonDto }: { id: string; mergePersonDto: MergePersonDto; }, opts?: Oazapfts.RequestOpts) { diff --git a/server/src/controllers/person.controller.ts b/server/src/controllers/person.controller.ts index c9980486b..a2d8e9801 100644 --- a/server/src/controllers/person.controller.ts +++ b/server/src/controllers/person.controller.ts @@ -187,19 +187,36 @@ export class PersonController { return this.service.reassignFaces(auth, id, dto); } + @Post('merge') + @Authenticated({ permission: Permission.PersonMerge }) + @HttpCode(HttpStatus.OK) + @Endpoint({ + summary: 'Merge people', + description: + 'Merge an ordered list of people together into a single person. The final name and birth date are always the first defined value, following the order. Also automatically merges people for other users in the cluster group, skipping people that would result in overriding a previously set name or birth date.', + history: new HistoryBuilder().added('v3.2.1').stable('v3.2.1'), + }) + mergePeople(@Auth() auth: AuthDto, @Body() dto: MergePersonDto): Promise { + return this.service.mergePeople(auth, dto); + } + @Post(':id/merge') @Authenticated({ permission: Permission.PersonMerge }) @HttpCode(HttpStatus.OK) @Endpoint({ summary: 'Merge people', description: 'Merge a list of people into the person specified in the path parameter.', - history: new HistoryBuilder().added('v1').beta('v1').stable('v2'), + history: new HistoryBuilder() + .added('v1') + .beta('v1') + .stable('v2') + .deprecated('v3.2.1', { replacementId: 'mergePeople' }), }) - mergePerson( + mergePersonLegacy( @Auth() auth: AuthDto, @Param() { id }: UUIDParamDto, @Body() dto: MergePersonDto, ): Promise { - return this.service.mergePerson(auth, id, dto); + return this.service.mergePeople(auth, { ids: [id, ...dto.ids] }); } } diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 6e0b61ff9..f98dd3f2d 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -1,5 +1,6 @@ import { BadRequestException, Injectable, NotFoundException } from '@nestjs/common'; import { Insertable, Selectable, Updateable } from 'kysely'; +import { isUndefined, omitBy } from 'lodash-es'; import type { JobItem, JobOf } from 'src/types.js'; import { Person } from 'src/database.js'; import { Chunked, OnJob } from 'src/decorators.js'; @@ -578,82 +579,88 @@ export class PersonService extends BaseService { return JobStatus.Success; } - async mergePerson(auth: AuthDto, personGroupId: string, dto: MergePersonDto): Promise { - const mergeIds = dto.ids; - if (mergeIds.includes(personGroupId)) { + async mergePeople(auth: AuthDto, { ids }: MergePersonDto): Promise { + if (ids.length < 2) { + throw new BadRequestException('At least two people are required for merging'); + } + + if (new Set(ids).size !== ids.length) { throw new BadRequestException('Cannot merge a person into themselves'); } - await this.requireAccess({ auth, permission: Permission.PersonUpdate, ids: [personGroupId] }); - const results: BulkIdResponseDto[] = []; - const allowedIds = await this.checkAccess({ - auth, - permission: Permission.PersonMerge, - ids: mergeIds, - }); + const allowedIds = await this.checkAccess({ auth, permission: Permission.PersonMerge, ids }); - let primaryPerson: Selectable | undefined; + const peopleMap: Record[]> = {}; - for (const mergePerson of await this.personRepository.getForMergePerson(mergeIds)) { - const mergeId = mergePerson.personGroupId; + for (const mergePerson of await this.personRepository.getForMergePerson(ids)) { + if (!peopleMap[mergePerson.personGroupId]) { + peopleMap[mergePerson.personGroupId] = []; + } + peopleMap[mergePerson.personGroupId].push(mergePerson); + } + + const targetPeople: Record> = {}; + for (const mergeId of ids) { const hasAccess = allowedIds.has(mergeId); if (!hasAccess) { results.push({ id: mergeId, success: false, error: BulkIdErrorReason.NO_PERMISSION }); continue; } - if (!primaryPerson || primaryPerson.ownerId !== mergePerson.ownerId) { - primaryPerson = await this.personRepository.getByGroupId({ ownerId: mergePerson.ownerId, personGroupId }); - if (!primaryPerson) { + for (const mergePerson of peopleMap[mergeId]) { + if (!targetPeople[mergePerson.ownerId]) { + targetPeople[mergePerson.ownerId] = mergePerson; continue; } - } - const changes: Updateable = {}; - if (!primaryPerson.name && mergePerson.name) { - changes.name = mergePerson.name; - } + const targetPerson = targetPeople[mergePerson.ownerId]; - if (!primaryPerson.birthDate && mergePerson.birthDate) { - changes.birthDate = mergePerson.birthDate; - } + if ( + mergePerson.ownerId !== auth.user.id && + ((targetPerson.name && mergePerson.name) || (targetPerson.birthDate && mergePerson.birthDate)) + ) { + continue; + } - if ( - (mergePerson.name && mergePerson.name !== primaryPerson.name) || - (mergePerson.birthDate && mergePerson.birthDate !== primaryPerson.birthDate) - ) { - continue; - } + const changes: Updateable = omitBy( + { + name: mergePerson.name && !targetPerson.name ? mergePerson.name : undefined, + birthDate: mergePerson.birthDate && !targetPerson.birthDate ? mergePerson.birthDate : undefined, + }, + isUndefined, + ); - if (Object.keys(changes).length > 0) { - primaryPerson = await this.personRepository.update({ - ownerId: primaryPerson.ownerId, - personGroupId: primaryPerson.personGroupId, - ...changes, - }); - } + if (Object.keys(changes).length > 0) { + targetPeople[mergePerson.ownerId] = await this.personRepository.update({ + ownerId: targetPerson.ownerId, + personGroupId: targetPerson.personGroupId, + ...changes, + }); + } - const mergeName = mergePerson.name || mergePerson.personGroupId; - const mergeData: UpdateFacesData = { - oldPersonGroupId: mergeId, - newPersonGroupId: primaryPerson.personGroupId, - ownerId: primaryPerson.ownerId, - }; - this.logger.log(`Merging ${mergeName} into ${primaryPerson.name || primaryPerson.personGroupId}`); + const mergeName = mergePerson.name || mergePerson.personGroupId; + const mergeData: UpdateFacesData = { + oldPersonGroupId: mergeId, + newPersonGroupId: targetPerson.personGroupId, + ownerId: targetPerson.ownerId, + }; + this.logger.log(`Merging ${mergeName} into ${targetPerson.name || targetPerson.personGroupId}`); - try { - await this.personRepository.reassignFaces(mergeData); - await this.removeAllPersonGroups([mergeId], primaryPerson.ownerId); + try { + await this.personRepository.reassignFaces(mergeData); + await this.removeAllPersonGroups([mergeId], targetPerson.ownerId); - this.logger.log(`Merged ${mergeName} into ${primaryPerson.name || primaryPerson.personGroupId}`); - results.push({ id: mergeId, success: true }); - } catch (error: any) { - this.logger.error(`Unable to merge ${mergeId} into ${personGroupId}: ${error}`, error?.stack); - results.push({ id: mergeId, success: false, error: BulkIdErrorReason.UNKNOWN }); + this.logger.log(`Merged ${mergeName} into ${targetPerson.name || targetPerson.personGroupId}`); + results.push({ id: mergeId, success: true }); + } catch (error: any) { + this.logger.error(`Unable to merge ${mergeId} into ${targetPerson.personGroupId}: ${error}`, error?.stack); + results.push({ id: mergeId, success: false, error: BulkIdErrorReason.UNKNOWN }); + } } } + return results; } diff --git a/server/test/medium/specs/services/person.service.spec.ts b/server/test/medium/specs/services/person.service.spec.ts index 1fc795e93..f898849f9 100644 --- a/server/test/medium/specs/services/person.service.spec.ts +++ b/server/test/medium/specs/services/person.service.spec.ts @@ -201,13 +201,13 @@ describe(PersonService.name, () => { }); }); - describe('mergePerson', () => { + describe('mergePeople', () => { it('should merge people of multiple users', 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: person1 } = await ctx.newPerson({ ownerId: user1.id, name: undefined }); const { person: person2 } = await ctx.newPerson({ ownerId: user1.id }); await ctx.newPerson({ ownerId: user2.id, @@ -216,6 +216,7 @@ describe(PersonService.name, () => { await ctx.newPerson({ ownerId: user2.id, personGroupId: person2.personGroupId, + name: undefined, }); const { asset } = await ctx.newAsset({ ownerId: user2.id }); await ctx.newAssetFace({ assetId: asset.id, personGroupId: person2.personGroupId }); @@ -223,7 +224,7 @@ describe(PersonService.name, () => { const auth = factory.auth({ user: user1 }); - await sut.mergePerson(auth, person1.personGroupId, { ids: [person2.personGroupId] }); + await sut.mergePeople(auth, { ids: [person1.personGroupId, 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 })]); @@ -254,7 +255,7 @@ describe(PersonService.name, () => { const auth = factory.auth({ user: user1 }); - await sut.mergePerson(auth, person1.personGroupId, { ids: [person2.personGroupId] }); + await sut.mergePeople(auth, { ids: [person1.personGroupId, 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 })]); @@ -266,7 +267,7 @@ describe(PersonService.name, () => { ); }); - it('should skip people with a different birthdate', async () => { + it('should skip people with a different birth date', async () => { const { sut, ctx } = setup(); const storageMock = ctx.getMock(StorageRepository); const { user: user1 } = await ctx.newUser(); @@ -287,7 +288,7 @@ describe(PersonService.name, () => { const auth = factory.auth({ user: user1 }); - await sut.mergePerson(auth, person1.personGroupId, { ids: [person2.personGroupId] }); + await sut.mergePeople(auth, { ids: [person1.personGroupId, 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 })]); @@ -316,7 +317,7 @@ describe(PersonService.name, () => { const auth = factory.auth({ user: user1 }); - await sut.mergePerson(auth, person1.personGroupId, { ids: [person2.personGroupId] }); + await sut.mergePeople(auth, { ids: [person1.personGroupId, 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 })]); diff --git a/web/src/lib/modals/PersonMergeSuggestionModal.svelte b/web/src/lib/modals/PersonMergeSuggestionModal.svelte index 94006ed5e..0fc637ad0 100644 --- a/web/src/lib/modals/PersonMergeSuggestionModal.svelte +++ b/web/src/lib/modals/PersonMergeSuggestionModal.svelte @@ -1,7 +1,7 @@