From 29c3a826c5510f8dea44ed97ee8e5896fa869c51 Mon Sep 17 00:00:00 2001 From: Kokul Shanmugharajah Date: Wed, 13 Mar 2024 10:14:26 -0700 Subject: [PATCH] feat(server): Update XMP sidecar search to look for both photo.ext.xmp and photo.xmp (#7813) * Add support for photo.xmp sidecars * format * Add comment * Proper handling * Handle mocking better * Address PR feedback * Add test coverage if both xmp files exist * Update server/src/domain/metadata/metadata.service.ts Co-authored-by: Jason Rasmussen * Update server/src/domain/metadata/metadata.service.ts Co-authored-by: Jason Rasmussen * Update server/src/domain/metadata/metadata.service.ts Co-authored-by: Jason Rasmussen --------- Co-authored-by: Alex Co-authored-by: Jason Rasmussen --- .../domain/metadata/metadata.service.spec.ts | 37 ++++++++++++++++++- .../src/domain/metadata/metadata.service.ts | 27 ++++++++++++-- server/test/fixtures/asset.stub.ts | 36 ++++++++++++++++++ 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/server/src/domain/metadata/metadata.service.spec.ts b/server/src/domain/metadata/metadata.service.spec.ts index 3da9ba3718..36315cf726 100644 --- a/server/src/domain/metadata/metadata.service.spec.ts +++ b/server/src/domain/metadata/metadata.service.spec.ts @@ -646,7 +646,7 @@ describe(MetadataService.name, () => { expect(assetMock.save).not.toHaveBeenCalled(); }); - it('should set sidecar path if exists', async () => { + it('should set sidecar path if exists (sidecar named photo.ext.xmp)', async () => { assetMock.getByIds.mockResolvedValue([assetStub.sidecar]); storageMock.checkFileExists.mockResolvedValue(true); @@ -658,6 +658,41 @@ describe(MetadataService.name, () => { }); }); + it('should set sidecar path if exists (sidecar named photo.xmp)', async () => { + assetMock.getByIds.mockResolvedValue([assetStub.sidecarWithoutExt]); + storageMock.checkFileExists.mockResolvedValueOnce(false); + storageMock.checkFileExists.mockResolvedValueOnce(true); + + await expect(sut.handleSidecarSync({ id: assetStub.sidecarWithoutExt.id })).resolves.toBe(true); + expect(storageMock.checkFileExists).toHaveBeenNthCalledWith( + 2, + assetStub.sidecarWithoutExt.sidecarPath, + constants.R_OK, + ); + expect(assetMock.save).toHaveBeenCalledWith({ + id: assetStub.sidecarWithoutExt.id, + sidecarPath: assetStub.sidecarWithoutExt.sidecarPath, + }); + }); + + it('should set sidecar path if exists (two sidecars named photo.ext.xmp and photo.xmp, should pick photo.ext.xmp)', async () => { + assetMock.getByIds.mockResolvedValue([assetStub.sidecar]); + storageMock.checkFileExists.mockResolvedValueOnce(true); + storageMock.checkFileExists.mockResolvedValueOnce(true); + + await expect(sut.handleSidecarSync({ id: assetStub.sidecar.id })).resolves.toBe(true); + expect(storageMock.checkFileExists).toHaveBeenNthCalledWith(1, assetStub.sidecar.sidecarPath, constants.R_OK); + expect(storageMock.checkFileExists).toHaveBeenNthCalledWith( + 2, + assetStub.sidecarWithoutExt.sidecarPath, + constants.R_OK, + ); + expect(assetMock.save).toHaveBeenCalledWith({ + id: assetStub.sidecar.id, + sidecarPath: assetStub.sidecar.sidecarPath, + }); + }); + it('should unset sidecar path if file does not exist anymore', async () => { assetMock.getByIds.mockResolvedValue([assetStub.sidecar]); storageMock.checkFileExists.mockResolvedValue(false); diff --git a/server/src/domain/metadata/metadata.service.ts b/server/src/domain/metadata/metadata.service.ts index 7d22485118..39919f78f2 100644 --- a/server/src/domain/metadata/metadata.service.ts +++ b/server/src/domain/metadata/metadata.service.ts @@ -6,6 +6,7 @@ import { firstDateTime } from 'exiftool-vendored/dist/FirstDateTime'; import _ from 'lodash'; import { Duration } from 'luxon'; import { constants } from 'node:fs/promises'; +import path from 'node:path'; import { Subscription } from 'rxjs'; import { handlePromiseError, usePagination } from '../domain.util'; import { IBaseJob, IEntityJob, ISidecarWriteJob, JOBS_ASSET_PAGINATION_SIZE, JobName, QueueName } from '../job'; @@ -566,9 +567,25 @@ export class MetadataService { return false; } - const sidecarPath = `${asset.originalPath}.xmp`; - const exists = await this.storageRepository.checkFileExists(sidecarPath, constants.R_OK); - if (exists) { + // XMP sidecars can come in two filename formats. For a photo named photo.ext, the filenames are photo.ext.xmp and photo.xmp + const assetPath = path.parse(asset.originalPath); + const assetPathWithoutExt = path.join(assetPath.dir, assetPath.name); + const sidecarPathWithoutExt = `${assetPathWithoutExt}.xmp`; + const sidecarPathWithExt = `${asset.originalPath}.xmp`; + + const [sidecarPathWithExtExists, sidecarPathWithoutExtExists] = await Promise.all([ + this.storageRepository.checkFileExists(sidecarPathWithExt, constants.R_OK), + this.storageRepository.checkFileExists(sidecarPathWithoutExt, constants.R_OK), + ]); + + let sidecarPath = null; + if (sidecarPathWithExtExists) { + sidecarPath = sidecarPathWithExt; + } else if (sidecarPathWithoutExtExists) { + sidecarPath = sidecarPathWithoutExt; + } + + if (sidecarPath) { await this.assetRepository.save({ id: asset.id, sidecarPath }); return true; } @@ -577,7 +594,9 @@ export class MetadataService { return false; } - this.logger.debug(`Sidecar File '${sidecarPath}' was not found, removing sidecarPath for asset ${asset.id}`); + this.logger.debug( + `Sidecar file was not found. Checked paths '${sidecarPathWithExt}' and '${sidecarPathWithoutExt}'. Removing sidecarPath for asset ${asset.id}`, + ); await this.assetRepository.save({ id: asset.id, sidecarPath: null }); return true; diff --git a/server/test/fixtures/asset.stub.ts b/server/test/fixtures/asset.stub.ts index ea1617d6ae..d72a295d42 100644 --- a/server/test/fixtures/asset.stub.ts +++ b/server/test/fixtures/asset.stub.ts @@ -524,6 +524,42 @@ export const assetStub = { sidecarPath: '/original/path.ext.xmp', deletedAt: null, }), + sidecarWithoutExt: Object.freeze({ + id: 'asset-id', + deviceAssetId: 'device-asset-id', + fileModifiedAt: new Date('2023-02-23T05:06:29.716Z'), + fileCreatedAt: new Date('2023-02-23T05:06:29.716Z'), + owner: userStub.user1, + ownerId: 'user-id', + deviceId: 'device-id', + originalPath: '/original/path.ext', + resizePath: '/uploads/user-id/thumbs/path.ext', + thumbhash: null, + checksum: Buffer.from('file hash', 'utf8'), + type: AssetType.IMAGE, + webpPath: null, + encodedVideoPath: null, + createdAt: new Date('2023-02-23T05:06:29.716Z'), + updatedAt: new Date('2023-02-23T05:06:29.716Z'), + localDateTime: new Date('2023-02-23T05:06:29.716Z'), + isFavorite: true, + isArchived: false, + isReadOnly: false, + isExternal: false, + isOffline: false, + libraryId: 'library-id', + library: libraryStub.uploadLibrary1, + duration: null, + isVisible: true, + livePhotoVideo: null, + livePhotoVideoId: null, + tags: [], + sharedLinks: [], + originalFileName: 'asset-id.ext', + faces: [], + sidecarPath: '/original/path.xmp', + deletedAt: null, + }), readOnly: Object.freeze({ id: 'read-only-asset',