From d634ef2d2b683d019e263d17f34ee4cd735bcb10 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Tue, 10 Sep 2024 09:48:29 -0400 Subject: [PATCH] fix(server): person repo methods (#12524) --- server/src/cores/storage.core.ts | 2 +- server/src/interfaces/person.interface.ts | 6 +- server/src/repositories/person.repository.ts | 22 +++++-- server/src/services/audit.service.ts | 2 +- server/src/services/media.service.ts | 2 +- server/src/services/metadata.service.spec.ts | 30 +++++---- server/src/services/metadata.service.ts | 9 +-- server/src/services/person.service.spec.ts | 62 +++++++++---------- server/src/services/person.service.ts | 27 ++++---- .../repositories/person.repository.mock.ts | 4 +- 10 files changed, 85 insertions(+), 81 deletions(-) diff --git a/server/src/cores/storage.core.ts b/server/src/cores/storage.core.ts index a4d0d06152..e20a0c658d 100644 --- a/server/src/cores/storage.core.ts +++ b/server/src/cores/storage.core.ts @@ -301,7 +301,7 @@ export class StorageCore { return this.assetRepository.update({ id, sidecarPath: newPath }); } case PersonPathType.FACE: { - return this.personRepository.update([{ id, thumbnailPath: newPath }]); + return this.personRepository.update({ id, thumbnailPath: newPath }); } } } diff --git a/server/src/interfaces/person.interface.ts b/server/src/interfaces/person.interface.ts index fc6a389f3c..5708274a6e 100644 --- a/server/src/interfaces/person.interface.ts +++ b/server/src/interfaces/person.interface.ts @@ -54,7 +54,8 @@ export interface IPersonRepository { getAssets(personId: string): Promise; - create(entities: Partial[]): Promise; + create(person: Partial): Promise; + createAll(people: Partial[]): Promise; createFaces(entities: Partial[]): Promise; delete(entities: PersonEntity[]): Promise; deleteAll(): Promise; @@ -74,6 +75,7 @@ export interface IPersonRepository { reassignFace(assetFaceId: string, newPersonId: string): Promise; getNumberOfPeople(userId: string): Promise; reassignFaces(data: UpdateFacesData): Promise; - update(entities: Partial[]): Promise; + update(person: Partial): Promise; + updateAll(people: Partial[]): Promise; getLatestFaceDate(): Promise; } diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index 1290df740e..c0bfee5398 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -280,8 +280,13 @@ export class PersonRepository implements IPersonRepository { return result; } - create(entities: Partial[]): Promise { - return this.personRepository.save(entities); + create(person: Partial): Promise { + return this.save(person); + } + + async createAll(people: Partial[]): Promise { + const results = await this.personRepository.save(people); + return results.map((person) => person.id); } async createFaces(entities: AssetFaceEntity[]): Promise { @@ -297,8 +302,12 @@ export class PersonRepository implements IPersonRepository { }); } - async update(entities: Partial[]): Promise { - return await this.personRepository.save(entities); + async update(person: Partial): Promise { + return this.save(person); + } + + async updateAll(people: Partial[]): Promise { + await this.personRepository.save(people); } @GenerateSql({ params: [[{ assetId: DummyValue.UUID, personId: DummyValue.UUID }]] }) @@ -320,4 +329,9 @@ export class PersonRepository implements IPersonRepository { .getRawOne(); return result?.latestDate; } + + private async save(person: Partial): Promise { + const { id } = await this.personRepository.save(person); + return this.personRepository.findOneByOrFail({ id }); + } } diff --git a/server/src/services/audit.service.ts b/server/src/services/audit.service.ts index 4f292f7cc1..72db2b6eb5 100644 --- a/server/src/services/audit.service.ts +++ b/server/src/services/audit.service.ts @@ -115,7 +115,7 @@ export class AuditService { } case PersonPathType.FACE: { - await this.personRepository.update([{ id, thumbnailPath: pathValue }]); + await this.personRepository.update({ id, thumbnailPath: pathValue }); break; } diff --git a/server/src/services/media.service.ts b/server/src/services/media.service.ts index 919348b53e..e74335bdc3 100644 --- a/server/src/services/media.service.ts +++ b/server/src/services/media.service.ts @@ -117,7 +117,7 @@ export class MediaService { continue; } - await this.personRepository.update([{ id: person.id, faceAssetId: face.id }]); + await this.personRepository.update({ id: person.id, faceAssetId: face.id }); } jobs.push({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id: person.id } }); diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 114c3db8ab..ea7254f53f 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -1002,13 +1002,12 @@ describe(MetadataService.name, () => { systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); metadataMock.readTags.mockResolvedValue(metadataStub.withFaceNoName); personMock.getDistinctNames.mockResolvedValue([]); - personMock.create.mockResolvedValue([]); + personMock.createAll.mockResolvedValue([]); personMock.replaceFaces.mockResolvedValue([]); - personMock.update.mockResolvedValue([]); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(personMock.create).toHaveBeenCalledWith([]); + expect(personMock.createAll).toHaveBeenCalledWith([]); expect(personMock.replaceFaces).toHaveBeenCalledWith(assetStub.primaryImage.id, [], SourceType.EXIF); - expect(personMock.update).toHaveBeenCalledWith([]); + expect(personMock.updateAll).toHaveBeenCalledWith([]); }); it('should skip importing faces with empty name', async () => { @@ -1016,13 +1015,12 @@ describe(MetadataService.name, () => { systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); metadataMock.readTags.mockResolvedValue(metadataStub.withFaceEmptyName); personMock.getDistinctNames.mockResolvedValue([]); - personMock.create.mockResolvedValue([]); + personMock.createAll.mockResolvedValue([]); personMock.replaceFaces.mockResolvedValue([]); - personMock.update.mockResolvedValue([]); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(personMock.create).toHaveBeenCalledWith([]); + expect(personMock.createAll).toHaveBeenCalledWith([]); expect(personMock.replaceFaces).toHaveBeenCalledWith(assetStub.primaryImage.id, [], SourceType.EXIF); - expect(personMock.update).toHaveBeenCalledWith([]); + expect(personMock.updateAll).toHaveBeenCalledWith([]); }); it('should apply metadata face tags creating new persons', async () => { @@ -1030,13 +1028,13 @@ describe(MetadataService.name, () => { systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); metadataMock.readTags.mockResolvedValue(metadataStub.withFace); personMock.getDistinctNames.mockResolvedValue([]); - personMock.create.mockResolvedValue([personStub.withName]); + personMock.createAll.mockResolvedValue([personStub.withName.id]); personMock.replaceFaces.mockResolvedValue(['face-asset-uuid']); - personMock.update.mockResolvedValue([personStub.withName]); + personMock.update.mockResolvedValue(personStub.withName); await sut.handleMetadataExtraction({ id: assetStub.primaryImage.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.primaryImage.id]); expect(personMock.getDistinctNames).toHaveBeenCalledWith(assetStub.primaryImage.ownerId, { withHidden: true }); - expect(personMock.create).toHaveBeenCalledWith([expect.objectContaining({ name: personStub.withName.name })]); + expect(personMock.createAll).toHaveBeenCalledWith([expect.objectContaining({ name: personStub.withName.name })]); expect(personMock.replaceFaces).toHaveBeenCalledWith( assetStub.primaryImage.id, [ @@ -1055,7 +1053,7 @@ describe(MetadataService.name, () => { ], SourceType.EXIF, ); - expect(personMock.update).toHaveBeenCalledWith([{ id: 'random-uuid', faceAssetId: 'random-uuid' }]); + expect(personMock.updateAll).toHaveBeenCalledWith([{ id: 'random-uuid', faceAssetId: 'random-uuid' }]); expect(jobMock.queueAll).toHaveBeenCalledWith([ { name: JobName.GENERATE_PERSON_THUMBNAIL, @@ -1069,13 +1067,13 @@ describe(MetadataService.name, () => { systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); metadataMock.readTags.mockResolvedValue(metadataStub.withFace); personMock.getDistinctNames.mockResolvedValue([{ id: personStub.withName.id, name: personStub.withName.name }]); - personMock.create.mockResolvedValue([]); + personMock.createAll.mockResolvedValue([]); personMock.replaceFaces.mockResolvedValue(['face-asset-uuid']); - personMock.update.mockResolvedValue([personStub.withName]); + personMock.update.mockResolvedValue(personStub.withName); await sut.handleMetadataExtraction({ id: assetStub.primaryImage.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.primaryImage.id]); expect(personMock.getDistinctNames).toHaveBeenCalledWith(assetStub.primaryImage.ownerId, { withHidden: true }); - expect(personMock.create).toHaveBeenCalledWith([]); + expect(personMock.createAll).toHaveBeenCalledWith([]); expect(personMock.replaceFaces).toHaveBeenCalledWith( assetStub.primaryImage.id, [ @@ -1094,7 +1092,7 @@ describe(MetadataService.name, () => { ], SourceType.EXIF, ); - expect(personMock.update).toHaveBeenCalledWith([]); + expect(personMock.updateAll).toHaveBeenCalledWith([]); expect(jobMock.queueAll).toHaveBeenCalledWith([]); }); }); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index 0522c883dd..4ffbd7f09b 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -584,18 +584,15 @@ export class MetadataService { this.logger.debug(`Creating missing persons: ${missing.map((p) => `${p.name}/${p.id}`)}`); } - const newPersons = await this.personRepository.create(missing); + const newPersonIds = await this.personRepository.createAll(missing); const faceIds = await this.personRepository.replaceFaces(asset.id, discoveredFaces, SourceType.EXIF); this.logger.debug(`Created ${faceIds.length} faces for asset ${asset.id}`); - await this.personRepository.update(missingWithFaceAsset); + await this.personRepository.updateAll(missingWithFaceAsset); await this.jobRepository.queueAll( - newPersons.map((person) => ({ - name: JobName.GENERATE_PERSON_THUMBNAIL, - data: { id: person.id }, - })), + newPersonIds.map((id) => ({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id } })), ); } diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 51598b93d0..2b111706f1 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -241,18 +241,18 @@ describe(PersonService.name, () => { }); it("should update a person's name", async () => { - personMock.update.mockResolvedValue([personStub.withName]); + personMock.update.mockResolvedValue(personStub.withName); personMock.getAssets.mockResolvedValue([assetStub.image]); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).resolves.toEqual(responseDto); - expect(personMock.update).toHaveBeenCalledWith([{ id: 'person-1', name: 'Person 1' }]); + expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', name: 'Person 1' }); expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); it("should update a person's date of birth", async () => { - personMock.update.mockResolvedValue([personStub.withBirthDate]); + personMock.update.mockResolvedValue(personStub.withBirthDate); personMock.getAssets.mockResolvedValue([assetStub.image]); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); @@ -264,25 +264,25 @@ describe(PersonService.name, () => { isHidden: false, updatedAt: expect.any(Date), }); - expect(personMock.update).toHaveBeenCalledWith([{ id: 'person-1', birthDate: '1976-06-30' }]); + expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', birthDate: '1976-06-30' }); expect(jobMock.queue).not.toHaveBeenCalled(); expect(jobMock.queueAll).not.toHaveBeenCalled(); expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); it('should update a person visibility', async () => { - personMock.update.mockResolvedValue([personStub.withName]); + personMock.update.mockResolvedValue(personStub.withName); personMock.getAssets.mockResolvedValue([assetStub.image]); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); await expect(sut.update(authStub.admin, 'person-1', { isHidden: false })).resolves.toEqual(responseDto); - expect(personMock.update).toHaveBeenCalledWith([{ id: 'person-1', isHidden: false }]); + expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', isHidden: false }); expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); it("should update a person's thumbnailPath", async () => { - personMock.update.mockResolvedValue([personStub.withName]); + personMock.update.mockResolvedValue(personStub.withName); personMock.getFacesByIds.mockResolvedValue([faceStub.face1]); accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); @@ -291,7 +291,7 @@ describe(PersonService.name, () => { sut.update(authStub.admin, 'person-1', { featureFaceAssetId: faceStub.face1.assetId }), ).resolves.toEqual(responseDto); - expect(personMock.update).toHaveBeenCalledWith([{ id: 'person-1', faceAssetId: faceStub.face1.id }]); + expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', faceAssetId: faceStub.face1.id }); expect(personMock.getFacesByIds).toHaveBeenCalledWith([ { assetId: faceStub.face1.assetId, @@ -441,11 +441,11 @@ describe(PersonService.name, () => { describe('createPerson', () => { it('should create a new person', async () => { - personMock.create.mockResolvedValue([personStub.primaryPerson]); + personMock.create.mockResolvedValue(personStub.primaryPerson); await expect(sut.create(authStub.admin, {})).resolves.toBe(personStub.primaryPerson); - expect(personMock.create).toHaveBeenCalledWith([{ ownerId: authStub.admin.user.id }]); + expect(personMock.create).toHaveBeenCalledWith({ ownerId: authStub.admin.user.id }); }); }); @@ -819,7 +819,7 @@ describe(PersonService.name, () => { systemMock.get.mockResolvedValue({ machineLearning: { facialRecognition: { minFaces: 1 } } }); searchMock.searchFaces.mockResolvedValue(faces); personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.noPerson1); - personMock.create.mockResolvedValue([faceStub.primaryFace1.person]); + personMock.create.mockResolvedValue(faceStub.primaryFace1.person); await sut.handleRecognizeFaces({ id: faceStub.noPerson1.id }); @@ -844,16 +844,14 @@ describe(PersonService.name, () => { systemMock.get.mockResolvedValue({ machineLearning: { facialRecognition: { minFaces: 1 } } }); searchMock.searchFaces.mockResolvedValue(faces); personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.noPerson1); - personMock.create.mockResolvedValue([personStub.withName]); + personMock.create.mockResolvedValue(personStub.withName); await sut.handleRecognizeFaces({ id: faceStub.noPerson1.id }); - expect(personMock.create).toHaveBeenCalledWith([ - { - ownerId: faceStub.noPerson1.asset.ownerId, - faceAssetId: faceStub.noPerson1.id, - }, - ]); + expect(personMock.create).toHaveBeenCalledWith({ + ownerId: faceStub.noPerson1.asset.ownerId, + faceAssetId: faceStub.noPerson1.id, + }); expect(personMock.reassignFaces).toHaveBeenCalledWith({ faceIds: [faceStub.noPerson1.id], newPersonId: personStub.withName.id, @@ -865,7 +863,7 @@ describe(PersonService.name, () => { searchMock.searchFaces.mockResolvedValue(faces); personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.noPerson1); - personMock.create.mockResolvedValue([personStub.withName]); + personMock.create.mockResolvedValue(personStub.withName); await sut.handleRecognizeFaces({ id: faceStub.noPerson1.id }); @@ -884,7 +882,7 @@ describe(PersonService.name, () => { systemMock.get.mockResolvedValue({ machineLearning: { facialRecognition: { minFaces: 3 } } }); searchMock.searchFaces.mockResolvedValue(faces); personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.noPerson1); - personMock.create.mockResolvedValue([personStub.withName]); + personMock.create.mockResolvedValue(personStub.withName); await sut.handleRecognizeFaces({ id: faceStub.noPerson1.id }); @@ -906,7 +904,7 @@ describe(PersonService.name, () => { systemMock.get.mockResolvedValue({ machineLearning: { facialRecognition: { minFaces: 3 } } }); searchMock.searchFaces.mockResolvedValueOnce(faces).mockResolvedValueOnce([]); personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.noPerson1); - personMock.create.mockResolvedValue([personStub.withName]); + personMock.create.mockResolvedValue(personStub.withName); await sut.handleRecognizeFaces({ id: faceStub.noPerson1.id, deferred: true }); @@ -979,12 +977,10 @@ describe(PersonService.name, () => { processInvalidImages: false, }, ); - expect(personMock.update).toHaveBeenCalledWith([ - { - id: 'person-1', - thumbnailPath: 'upload/thumbs/admin_id/pe/rs/person-1.jpeg', - }, - ]); + expect(personMock.update).toHaveBeenCalledWith({ + id: 'person-1', + thumbnailPath: 'upload/thumbs/admin_id/pe/rs/person-1.jpeg', + }); }); it('should generate a thumbnail without going negative', async () => { @@ -1103,7 +1099,7 @@ describe(PersonService.name, () => { it('should merge two people with smart merge', async () => { personMock.getById.mockResolvedValueOnce(personStub.randomPerson); personMock.getById.mockResolvedValueOnce(personStub.primaryPerson); - personMock.update.mockResolvedValue([{ ...personStub.randomPerson, name: personStub.primaryPerson.name }]); + personMock.update.mockResolvedValue({ ...personStub.randomPerson, name: personStub.primaryPerson.name }); accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-3'])); accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-1'])); @@ -1116,12 +1112,10 @@ describe(PersonService.name, () => { oldPersonId: personStub.primaryPerson.id, }); - expect(personMock.update).toHaveBeenCalledWith([ - { - id: personStub.randomPerson.id, - name: personStub.primaryPerson.name, - }, - ]); + expect(personMock.update).toHaveBeenCalledWith({ + id: personStub.randomPerson.id, + name: personStub.primaryPerson.name, + }); expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index c4b5df5719..dd4a4cecf2 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -173,7 +173,7 @@ export class PersonService { const assetFace = await this.repository.getRandomFace(personId); if (assetFace !== null) { - await this.repository.update([{ id: personId, faceAssetId: assetFace.id }]); + await this.repository.update({ id: personId, faceAssetId: assetFace.id }); jobs.push({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id: personId } }); } } @@ -211,16 +211,13 @@ export class PersonService { return assets.map((asset) => mapAsset(asset)); } - async create(auth: AuthDto, dto: PersonCreateDto): Promise { - const [created] = await this.repository.create([ - { - ownerId: auth.user.id, - name: dto.name, - birthDate: dto.birthDate, - isHidden: dto.isHidden, - }, - ]); - return created; + create(auth: AuthDto, dto: PersonCreateDto): Promise { + return this.repository.create({ + ownerId: auth.user.id, + name: dto.name, + birthDate: dto.birthDate, + isHidden: dto.isHidden, + }); } async update(auth: AuthDto, id: string, dto: PersonUpdateDto): Promise { @@ -239,7 +236,7 @@ export class PersonService { faceId = face.id; } - const [person] = await this.repository.update([{ id, faceAssetId: faceId, name, birthDate, isHidden }]); + const person = await this.repository.update({ id, faceAssetId: faceId, name, birthDate, isHidden }); if (assetId) { await this.jobRepository.queue({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id } }); @@ -501,7 +498,7 @@ export class PersonService { if (isCore && !personId) { this.logger.log(`Creating new person for face ${id}`); - const [newPerson] = await this.repository.create([{ ownerId: face.asset.ownerId, faceAssetId: face.id }]); + const newPerson = await this.repository.create({ ownerId: face.asset.ownerId, faceAssetId: face.id }); await this.jobRepository.queue({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id: newPerson.id } }); personId = newPerson.id; } @@ -577,7 +574,7 @@ export class PersonService { } as const; await this.mediaRepository.generateThumbnail(inputPath, thumbnailPath, thumbnailOptions); - await this.repository.update([{ id: person.id, thumbnailPath }]); + await this.repository.update({ id: person.id, thumbnailPath }); return JobStatus.SUCCESS; } @@ -624,7 +621,7 @@ export class PersonService { } if (Object.keys(update).length > 0) { - [primaryPerson] = await this.repository.update([{ id: primaryPerson.id, ...update }]); + primaryPerson = await this.repository.update({ id: primaryPerson.id, ...update }); } const mergeName = mergePerson.name || mergePerson.id; diff --git a/server/test/repositories/person.repository.mock.ts b/server/test/repositories/person.repository.mock.ts index 6547a54339..77e8ccf010 100644 --- a/server/test/repositories/person.repository.mock.ts +++ b/server/test/repositories/person.repository.mock.ts @@ -13,9 +13,11 @@ export const newPersonRepositoryMock = (): Mocked => { getDistinctNames: vitest.fn(), create: vitest.fn(), + createAll: vitest.fn(), update: vitest.fn(), - deleteAll: vitest.fn(), + updateAll: vitest.fn(), delete: vitest.fn(), + deleteAll: vitest.fn(), deleteAllFaces: vitest.fn(), getStatistics: vitest.fn(),