From a618c9fb6e866a9d8d33687f7a62b1d91a206352 Mon Sep 17 00:00:00 2001 From: Daniel Dietzler Date: Thu, 4 Dec 2025 15:33:44 +0100 Subject: [PATCH] fix: asset update race condition --- server/src/database.ts | 2 +- server/src/queries/asset.job.repository.sql | 12 +- server/src/queries/asset.repository.sql | 16 +- .../src/repositories/asset-job.repository.ts | 11 ++ server/src/repositories/asset.repository.ts | 145 ++++++++++++------ ...57138636-AddLockedPropertiesToAssetExif.ts | 9 ++ server/src/schema/tables/asset-exif.table.ts | 6 + server/src/services/asset-media.service.ts | 12 +- server/src/services/asset.service.spec.ts | 13 +- server/src/services/asset.service.ts | 25 ++- server/src/services/metadata.service.spec.ts | 115 +++++++++----- server/src/services/metadata.service.ts | 18 ++- server/src/types.ts | 5 - server/test/medium.factory.ts | 2 +- .../specs/services/metadata.service.spec.ts | 1 + .../specs/sync/sync-album-asset-exif.spec.ts | 22 ++- server/test/small.factory.ts | 35 +++-- 17 files changed, 316 insertions(+), 133 deletions(-) create mode 100644 server/src/schema/migrations/1764957138636-AddLockedPropertiesToAssetExif.ts diff --git a/server/src/database.ts b/server/src/database.ts index a3c38ae61e..a9429ac001 100644 --- a/server/src/database.ts +++ b/server/src/database.ts @@ -240,7 +240,7 @@ export type Session = { isPendingSyncReset: boolean; }; -export type Exif = Omit, 'updatedAt' | 'updateId'>; +export type Exif = Omit, 'updatedAt' | 'updateId' | 'lockedProperties'>; export type Person = { createdAt: Date; diff --git a/server/src/queries/asset.job.repository.sql b/server/src/queries/asset.job.repository.sql index d1b4e5a72e..b736998e28 100644 --- a/server/src/queries/asset.job.repository.sql +++ b/server/src/queries/asset.job.repository.sql @@ -50,9 +50,11 @@ select where "asset"."id" = "tag_asset"."assetId" ) as agg - ) as "tags" + ) as "tags", + to_json("asset_exif") as "exifInfo" from "asset" + inner join "asset_exif" on "asset"."id" = "asset_exif"."assetId" where "asset"."id" = $2::uuid limit @@ -224,6 +226,14 @@ from where "asset"."id" = $2 +-- AssetJobRepository.getLockedPropertiesForMetadataExtraction +select + "asset_exif"."lockedProperties" +from + "asset_exif" +where + "asset_exif"."assetId" = $1 + -- AssetJobRepository.getAlbumThumbnailFiles select "asset_file"."id", diff --git a/server/src/queries/asset.repository.sql b/server/src/queries/asset.repository.sql index 01cc6a7a89..a61d75304f 100644 --- a/server/src/queries/asset.repository.sql +++ b/server/src/queries/asset.repository.sql @@ -3,17 +3,25 @@ -- AssetRepository.updateAllExif update "asset_exif" set - "model" = $1 + "model" = $1, + "lockedProperties" = array( + select distinct + unnest(array_cat("lockedProperties", $2)) + ) where - "assetId" in ($2) + "assetId" in ($3) -- AssetRepository.updateDateTimeOriginal update "asset_exif" set "dateTimeOriginal" = "dateTimeOriginal" + $1::interval, - "timeZone" = $2 + "timeZone" = $2, + "lockedProperties" = array( + select distinct + unnest(array_cat("lockedProperties", $3)) + ) where - "assetId" in ($3) + "assetId" in ($4) returning "assetId", "dateTimeOriginal", diff --git a/server/src/repositories/asset-job.repository.ts b/server/src/repositories/asset-job.repository.ts index de994b08cd..214d42747f 100644 --- a/server/src/repositories/asset-job.repository.ts +++ b/server/src/repositories/asset-job.repository.ts @@ -50,6 +50,7 @@ export class AssetJobRepository { .whereRef('asset.id', '=', 'tag_asset.assetId'), ).as('tags'), ) + .$call(withExifInner) .limit(1) .executeTakeFirst(); } @@ -128,6 +129,16 @@ export class AssetJobRepository { .executeTakeFirst(); } + @GenerateSql({ params: [DummyValue.UUID] }) + async getLockedPropertiesForMetadataExtraction(assetId: string) { + return this.db + .selectFrom('asset_exif') + .select('asset_exif.lockedProperties') + .where('asset_exif.assetId', '=', assetId) + .executeTakeFirst() + .then((row) => row?.lockedProperties ?? []); + } + @GenerateSql({ params: [DummyValue.UUID, AssetFileType.Thumbnail] }) getAlbumThumbnailFiles(id: string, fileType?: AssetFileType) { return this.db diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 4b8cbd7a7a..3f7568b572 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -1,13 +1,13 @@ import { Injectable } from '@nestjs/common'; import { Insertable, Kysely, NotNull, Selectable, sql, Updateable, UpdateResult } from 'kysely'; -import { isEmpty, isUndefined, omitBy } from 'lodash'; +import { intersection, isEmpty, isUndefined, omit, omitBy, union } from 'lodash'; import { InjectKysely } from 'nestjs-kysely'; import { Stack } from 'src/database'; import { Chunked, ChunkedArray, DummyValue, GenerateSql } from 'src/decorators'; import { AuthDto } from 'src/dtos/auth.dto'; import { AssetFileType, AssetMetadataKey, AssetOrder, AssetStatus, AssetType, AssetVisibility } from 'src/enum'; import { DB } from 'src/schema'; -import { AssetExifTable } from 'src/schema/tables/asset-exif.table'; +import { AssetExifTable, lockableProperties, LockableProperty } from 'src/schema/tables/asset-exif.table'; import { AssetFileTable } from 'src/schema/tables/asset-file.table'; import { AssetJobStatusTable } from 'src/schema/tables/asset-job-status.table'; import { AssetTable } from 'src/schema/tables/asset.table'; @@ -117,49 +117,83 @@ interface GetByIdsRelations { export class AssetRepository { constructor(@InjectKysely() private db: Kysely) {} - async upsertExif(exif: Insertable): Promise { - const value = { ...exif, assetId: asUuid(exif.assetId) }; - await this.db - .insertInto('asset_exif') - .values(value) - .onConflict((oc) => - oc.column('assetId').doUpdateSet((eb) => - removeUndefinedKeys( - { - description: eb.ref('excluded.description'), - exifImageWidth: eb.ref('excluded.exifImageWidth'), - exifImageHeight: eb.ref('excluded.exifImageHeight'), - fileSizeInByte: eb.ref('excluded.fileSizeInByte'), - orientation: eb.ref('excluded.orientation'), - dateTimeOriginal: eb.ref('excluded.dateTimeOriginal'), - modifyDate: eb.ref('excluded.modifyDate'), - timeZone: eb.ref('excluded.timeZone'), - latitude: eb.ref('excluded.latitude'), - longitude: eb.ref('excluded.longitude'), - projectionType: eb.ref('excluded.projectionType'), - city: eb.ref('excluded.city'), - livePhotoCID: eb.ref('excluded.livePhotoCID'), - autoStackId: eb.ref('excluded.autoStackId'), - state: eb.ref('excluded.state'), - country: eb.ref('excluded.country'), - make: eb.ref('excluded.make'), - model: eb.ref('excluded.model'), - lensModel: eb.ref('excluded.lensModel'), - fNumber: eb.ref('excluded.fNumber'), - focalLength: eb.ref('excluded.focalLength'), - iso: eb.ref('excluded.iso'), - exposureTime: eb.ref('excluded.exposureTime'), - profileDescription: eb.ref('excluded.profileDescription'), - colorspace: eb.ref('excluded.colorspace'), - bitsPerSample: eb.ref('excluded.bitsPerSample'), - rating: eb.ref('excluded.rating'), - fps: eb.ref('excluded.fps'), - }, - value, + async upsertExif( + exif: Insertable, + { lockedPropertiesBehavior }: { lockedPropertiesBehavior: 'none' | 'update' | 'skip' }, + ): Promise { + await this.db.transaction().execute(async (tx) => { + const lockedProperties = await tx + .selectFrom('asset_exif') + .select('asset_exif.lockedProperties') + .where('asset_exif.assetId', '=', exif.assetId) + .executeTakeFirst() + .then((result) => result?.lockedProperties ?? []); + + let value = { ...exif, assetId: asUuid(exif.assetId) }; + + switch (lockedPropertiesBehavior) { + case 'skip': { + value = omit(value, [...lockedProperties, 'lockedProperties']); + break; + } + + case 'update': { + const updatedLockableProperties = intersection(lockableProperties, Object.keys(exif)) as LockableProperty[]; + value = { + ...value, + lockedProperties: union(updatedLockableProperties, lockedProperties), + }; + break; + } + } + + if (Object.keys(value).length <= 1) { + return; + } + + return tx + .insertInto('asset_exif') + .values(value) + .onConflict((oc) => + oc.column('assetId').doUpdateSet((eb) => + removeUndefinedKeys( + { + description: eb.ref('excluded.description'), + exifImageWidth: eb.ref('excluded.exifImageWidth'), + exifImageHeight: eb.ref('excluded.exifImageHeight'), + fileSizeInByte: eb.ref('excluded.fileSizeInByte'), + orientation: eb.ref('excluded.orientation'), + dateTimeOriginal: eb.ref('excluded.dateTimeOriginal'), + modifyDate: eb.ref('excluded.modifyDate'), + timeZone: eb.ref('excluded.timeZone'), + latitude: eb.ref('excluded.latitude'), + longitude: eb.ref('excluded.longitude'), + projectionType: eb.ref('excluded.projectionType'), + city: eb.ref('excluded.city'), + livePhotoCID: eb.ref('excluded.livePhotoCID'), + autoStackId: eb.ref('excluded.autoStackId'), + state: eb.ref('excluded.state'), + country: eb.ref('excluded.country'), + make: eb.ref('excluded.make'), + model: eb.ref('excluded.model'), + lensModel: eb.ref('excluded.lensModel'), + fNumber: eb.ref('excluded.fNumber'), + focalLength: eb.ref('excluded.focalLength'), + iso: eb.ref('excluded.iso'), + exposureTime: eb.ref('excluded.exposureTime'), + profileDescription: eb.ref('excluded.profileDescription'), + colorspace: eb.ref('excluded.colorspace'), + bitsPerSample: eb.ref('excluded.bitsPerSample'), + rating: eb.ref('excluded.rating'), + fps: eb.ref('excluded.fps'), + lockedProperties: eb.ref('excluded.lockedProperties'), + }, + value, + ), ), - ), - ) - .execute(); + ) + .execute(); + }); } @GenerateSql({ params: [[DummyValue.UUID], { model: DummyValue.STRING }] }) @@ -169,7 +203,18 @@ export class AssetRepository { return; } - await this.db.updateTable('asset_exif').set(options).where('assetId', 'in', ids).execute(); + await this.db + .updateTable('asset_exif') + .set((eb) => ({ + ...options, + lockedProperties: eb + .fn< + LockableProperty[] + >('array', [sql`select distinct unnest(${eb.fn('array_cat', ['lockedProperties', eb.val(Object.keys(options))])})`]) + .as('lockedProperties').expression, + })) + .where('assetId', 'in', ids) + .execute(); } @GenerateSql({ params: [[DummyValue.UUID], DummyValue.NUMBER, DummyValue.STRING] }) @@ -181,7 +226,15 @@ export class AssetRepository { ): Promise<{ assetId: string; dateTimeOriginal: Date | null; timeZone: string | null }[]> { return await this.db .updateTable('asset_exif') - .set({ dateTimeOriginal: sql`"dateTimeOriginal" + ${(delta ?? 0) + ' minute'}::interval`, timeZone }) + .set((eb) => ({ + dateTimeOriginal: sql`"dateTimeOriginal" + ${(delta ?? 0) + ' minute'}::interval`, + timeZone, + lockedProperties: eb + .fn< + LockableProperty[] + >('array', [sql`select distinct unnest(${eb.fn('array_cat', ['lockedProperties', eb.val(['dateTimeOriginal', 'timeZone'])])})`]) + .as('lockedProperties').expression, + })) .where('assetId', 'in', ids) .returning(['assetId', 'dateTimeOriginal', 'timeZone']) .execute(); diff --git a/server/src/schema/migrations/1764957138636-AddLockedPropertiesToAssetExif.ts b/server/src/schema/migrations/1764957138636-AddLockedPropertiesToAssetExif.ts new file mode 100644 index 0000000000..0cd024a4d8 --- /dev/null +++ b/server/src/schema/migrations/1764957138636-AddLockedPropertiesToAssetExif.ts @@ -0,0 +1,9 @@ +import { Kysely, sql } from 'kysely'; + +export async function up(db: Kysely): Promise { + await sql`ALTER TABLE "asset_exif" ADD "lockedProperties" character varying[];`.execute(db); +} + +export async function down(db: Kysely): Promise { + await sql`ALTER TABLE "asset_exif" DROP COLUMN "lockedProperties";`.execute(db); +} diff --git a/server/src/schema/tables/asset-exif.table.ts b/server/src/schema/tables/asset-exif.table.ts index f28735a2db..e25131193a 100644 --- a/server/src/schema/tables/asset-exif.table.ts +++ b/server/src/schema/tables/asset-exif.table.ts @@ -2,6 +2,9 @@ import { UpdatedAtTrigger, UpdateIdColumn } from 'src/decorators'; import { AssetTable } from 'src/schema/tables/asset.table'; import { Column, ForeignKeyColumn, Generated, Int8, Table, Timestamp, UpdateDateColumn } from 'src/sql-tools'; +export type LockableProperty = (typeof lockableProperties)[number]; +export const lockableProperties = ['description', 'dateTimeOriginal', 'latitude', 'longitude', 'rating'] as const; + @Table('asset_exif') @UpdatedAtTrigger('asset_exif_updatedAt') export class AssetExifTable { @@ -97,4 +100,7 @@ export class AssetExifTable { @UpdateIdColumn({ index: true }) updateId!: Generated; + + @Column({ type: 'character varying', array: true, nullable: true }) + lockedProperties!: Array | null; } diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index d2e1c14210..8481308c67 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -370,7 +370,7 @@ export class AssetMediaService extends BaseService { : this.assetRepository.deleteFile({ assetId, type: AssetFileType.Sidecar })); await this.storageRepository.utimes(file.originalPath, new Date(), new Date(dto.fileModifiedAt)); - await this.assetRepository.upsertExif({ assetId, fileSizeInByte: file.size }); + await this.assetRepository.upsertExif({ assetId, fileSizeInByte: file.size }, { lockedPropertiesBehavior: 'none' }); await this.jobRepository.queue({ name: JobName.AssetExtractMetadata, data: { id: assetId, source: 'upload' }, @@ -399,7 +399,10 @@ export class AssetMediaService extends BaseService { }); const { size } = await this.storageRepository.stat(created.originalPath); - await this.assetRepository.upsertExif({ assetId: created.id, fileSizeInByte: size }); + await this.assetRepository.upsertExif( + { assetId: created.id, fileSizeInByte: size }, + { lockedPropertiesBehavior: 'none' }, + ); await this.jobRepository.queue({ name: JobName.AssetExtractMetadata, data: { id: created.id, source: 'copy' } }); return created; } @@ -440,7 +443,10 @@ export class AssetMediaService extends BaseService { await this.storageRepository.utimes(sidecarFile.originalPath, new Date(), new Date(dto.fileModifiedAt)); } await this.storageRepository.utimes(file.originalPath, new Date(), new Date(dto.fileModifiedAt)); - await this.assetRepository.upsertExif({ assetId: asset.id, fileSizeInByte: file.size }); + await this.assetRepository.upsertExif( + { assetId: asset.id, fileSizeInByte: file.size }, + { lockedPropertiesBehavior: 'none' }, + ); await this.eventRepository.emit('AssetCreate', { asset }); diff --git a/server/src/services/asset.service.spec.ts b/server/src/services/asset.service.spec.ts index 878721e0a7..e58c868c5e 100755 --- a/server/src/services/asset.service.spec.ts +++ b/server/src/services/asset.service.spec.ts @@ -225,7 +225,10 @@ describe(AssetService.name, () => { await sut.update(authStub.admin, 'asset-1', { description: 'Test description' }); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith({ assetId: 'asset-1', description: 'Test description' }); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { assetId: 'asset-1', description: 'Test description' }, + { lockedPropertiesBehavior: 'update' }, + ); }); it('should update the exif rating', async () => { @@ -235,7 +238,13 @@ describe(AssetService.name, () => { await sut.update(authStub.admin, 'asset-1', { rating: 3 }); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith({ assetId: 'asset-1', rating: 3 }); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { + assetId: 'asset-1', + rating: 3, + }, + { lockedPropertiesBehavior: 'update' }, + ); }); it('should fail linking a live video if the motion part could not be found', async () => { diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index 32c6526394..6a9c71094a 100644 --- a/server/src/services/asset.service.ts +++ b/server/src/services/asset.service.ts @@ -30,7 +30,7 @@ import { QueueName, } from 'src/enum'; import { BaseService } from 'src/services/base.service'; -import { ISidecarWriteJob, JobItem, JobOf } from 'src/types'; +import { JobItem, JobOf } from 'src/types'; import { requireElevatedPermission } from 'src/utils/access'; import { getAssetFiles, getMyPartnerIds, onAfterUnlink, onBeforeLink, onBeforeUnlink } from 'src/utils/asset.util'; @@ -143,9 +143,9 @@ export class AssetService extends BaseService { await this.requireAccess({ auth, permission: Permission.AssetUpdate, ids }); const assetDto = { isFavorite, visibility, duplicateId }; - const exifDto = { latitude, longitude, rating, description, dateTimeOriginal }; + const exifDto = _.omitBy({ latitude, longitude, rating, description, dateTimeOriginal }, _.isUndefined); - const isExifChanged = Object.values(exifDto).some((v) => v !== undefined); + const isExifChanged = Object.keys(exifDto).length > 0; if (isExifChanged) { await this.assetRepository.updateAllExif(ids, exifDto); } @@ -456,12 +456,25 @@ export class AssetService extends BaseService { return asset; } - private async updateExif(dto: ISidecarWriteJob) { + private async updateExif(dto: { + id: string; + description?: string; + dateTimeOriginal?: string; + latitude?: number; + longitude?: number; + rating?: number; + }) { const { id, description, dateTimeOriginal, latitude, longitude, rating } = dto; const writes = _.omitBy({ description, dateTimeOriginal, latitude, longitude, rating }, _.isUndefined); if (Object.keys(writes).length > 0) { - await this.assetRepository.upsertExif({ assetId: id, ...writes }); - await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id, ...writes } }); + await this.assetRepository.upsertExif( + { + assetId: id, + ...writes, + }, + { lockedPropertiesBehavior: 'update' }, + ); + await this.jobRepository.queue({ name: JobName.SidecarWrite, data: { id } }); } } } diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index c0f930d3a6..98c906d9c7 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -187,7 +187,9 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.sidecar.id); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ dateTimeOriginal: sidecarDate })); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ dateTimeOriginal: sidecarDate }), { + lockedPropertiesBehavior: 'skip', + }); expect(mocks.asset.update).toHaveBeenCalledWith( expect.objectContaining({ id: assetStub.image.id, @@ -214,6 +216,7 @@ describe(MetadataService.name, () => { expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); expect(mocks.asset.upsertExif).toHaveBeenCalledWith( expect.objectContaining({ dateTimeOriginal: fileModifiedAt }), + { lockedPropertiesBehavior: 'skip' }, ); expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.image.id, @@ -238,7 +241,10 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ dateTimeOriginal: fileCreatedAt })); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + expect.objectContaining({ dateTimeOriginal: fileCreatedAt }), + { lockedPropertiesBehavior: 'skip' }, + ); expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.image.id, duration: null, @@ -258,6 +264,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ dateTimeOriginal: new Date('2022-01-01T00:00:00.000Z'), }), + { lockedPropertiesBehavior: 'skip' }, ); expect(mocks.asset.update).toHaveBeenCalledWith( @@ -281,7 +288,9 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ iso: 160 })); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ iso: 160 }), { + lockedPropertiesBehavior: 'skip', + }); expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.image.id, duration: null, @@ -310,6 +319,7 @@ describe(MetadataService.name, () => { expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); expect(mocks.asset.upsertExif).toHaveBeenCalledWith( expect.objectContaining({ city: null, state: null, country: null }), + { lockedPropertiesBehavior: 'skip' }, ); expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.withLocation.id, @@ -339,6 +349,7 @@ describe(MetadataService.name, () => { expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); expect(mocks.asset.upsertExif).toHaveBeenCalledWith( expect.objectContaining({ city: 'City', state: 'State', country: 'Country' }), + { lockedPropertiesBehavior: 'skip' }, ); expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.withLocation.id, @@ -358,7 +369,10 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining({ latitude: null, longitude: null })); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + expect.objectContaining({ latitude: null, longitude: null }), + { lockedPropertiesBehavior: 'skip' }, + ); }); it('should extract tags from TagsList', async () => { @@ -571,6 +585,7 @@ describe(MetadataService.name, () => { expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.video.id); expect(mocks.asset.upsertExif).toHaveBeenCalledWith( expect.objectContaining({ orientation: ExifOrientation.Rotate270CW.toString() }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -879,37 +894,40 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(mocks.assetJob.getForMetadataExtraction).toHaveBeenCalledWith(assetStub.image.id); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith({ - assetId: assetStub.image.id, - bitsPerSample: expect.any(Number), - autoStackId: null, - colorspace: tags.ColorSpace, - dateTimeOriginal: dateForTest, - description: tags.ImageDescription, - exifImageHeight: null, - exifImageWidth: null, - exposureTime: tags.ExposureTime, - fNumber: null, - fileSizeInByte: 123_456, - focalLength: tags.FocalLength, - fps: null, - iso: tags.ISO, - latitude: null, - lensModel: tags.LensModel, - livePhotoCID: tags.MediaGroupUUID, - longitude: null, - make: tags.Make, - model: tags.Model, - modifyDate: expect.any(Date), - orientation: tags.Orientation?.toString(), - profileDescription: tags.ProfileDescription, - projectionType: 'EQUIRECTANGULAR', - timeZone: tags.tz, - rating: tags.Rating, - country: null, - state: null, - city: null, - }); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith( + { + assetId: assetStub.image.id, + bitsPerSample: expect.any(Number), + autoStackId: null, + colorspace: tags.ColorSpace, + dateTimeOriginal: dateForTest, + description: tags.ImageDescription, + exifImageHeight: null, + exifImageWidth: null, + exposureTime: tags.ExposureTime, + fNumber: null, + fileSizeInByte: 123_456, + focalLength: tags.FocalLength, + fps: null, + iso: tags.ISO, + latitude: null, + lensModel: tags.LensModel, + livePhotoCID: tags.MediaGroupUUID, + longitude: null, + make: tags.Make, + model: tags.Model, + modifyDate: expect.any(Date), + orientation: tags.Orientation?.toString(), + profileDescription: tags.ProfileDescription, + projectionType: 'EQUIRECTANGULAR', + timeZone: tags.tz, + rating: tags.Rating, + country: null, + state: null, + city: null, + }, + { lockedPropertiesBehavior: 'skip' }, + ); expect(mocks.asset.update).toHaveBeenCalledWith( expect.objectContaining({ id: assetStub.image.id, @@ -943,6 +961,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ timeZone: 'UTC+0', }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1103,6 +1122,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ description: '', }), + { lockedPropertiesBehavior: 'skip' }, ); mockReadTags({ ImageDescription: ' my\n description' }); @@ -1111,6 +1131,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ description: 'my\n description', }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1123,6 +1144,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ description: '1000', }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1346,6 +1368,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ modifyDate: expect.any(Date), }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1358,6 +1381,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ rating: null, }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1370,6 +1394,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ rating: 5, }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1382,6 +1407,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ rating: -1, }), + { lockedPropertiesBehavior: 'skip' }, ); }); @@ -1503,7 +1529,9 @@ describe(MetadataService.name, () => { mockReadTags(exif); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining(expected)); + expect(mocks.asset.upsertExif).toHaveBeenCalledWith(expect.objectContaining(expected), { + lockedPropertiesBehavior: 'skip', + }); }); it.each([ @@ -1529,6 +1557,7 @@ describe(MetadataService.name, () => { expect.objectContaining({ lensModel: expected, }), + { lockedPropertiesBehavior: 'skip' }, ); }); }); @@ -1637,12 +1666,14 @@ describe(MetadataService.name, () => { describe('handleSidecarWrite', () => { it('should skip assets that no longer exist', async () => { + mocks.assetJob.getLockedPropertiesForMetadataExtraction.mockResolvedValue([]); mocks.assetJob.getForSidecarWriteJob.mockResolvedValue(void 0); await expect(sut.handleSidecarWrite({ id: 'asset-123' })).resolves.toBe(JobStatus.Failed); expect(mocks.metadata.writeTags).not.toHaveBeenCalled(); }); it('should skip jobs with no metadata', async () => { + mocks.assetJob.getLockedPropertiesForMetadataExtraction.mockResolvedValue([]); const asset = factory.jobAssets.sidecarWrite(); mocks.assetJob.getForSidecarWriteJob.mockResolvedValue(asset); await expect(sut.handleSidecarWrite({ id: asset.id })).resolves.toBe(JobStatus.Skipped); @@ -1655,20 +1686,22 @@ describe(MetadataService.name, () => { const gps = 12; const date = '2023-11-22T04:56:12.196Z'; + mocks.assetJob.getLockedPropertiesForMetadataExtraction.mockResolvedValue([ + 'description', + 'latitude', + 'longitude', + 'dateTimeOriginal', + ]); mocks.assetJob.getForSidecarWriteJob.mockResolvedValue(asset); await expect( sut.handleSidecarWrite({ id: asset.id, - description, - latitude: gps, - longitude: gps, - dateTimeOriginal: date, }), ).resolves.toBe(JobStatus.Success); expect(mocks.metadata.writeTags).toHaveBeenCalledWith(asset.files[0].path, { + DateTimeOriginal: date, Description: description, ImageDescription: description, - DateTimeOriginal: date, GPSLatitude: gps, GPSLongitude: gps, }); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index a1267604b2..b442e2b79e 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -290,7 +290,7 @@ export class MetadataService extends BaseService { }; const promises: Promise[] = [ - this.assetRepository.upsertExif(exifData), + this.assetRepository.upsertExif(exifData, { lockedPropertiesBehavior: 'skip' }), this.assetRepository.update({ id: asset.id, duration: this.getDuration(exifTags), @@ -393,22 +393,34 @@ export class MetadataService extends BaseService { @OnJob({ name: JobName.SidecarWrite, queue: QueueName.Sidecar }) async handleSidecarWrite(job: JobOf): Promise { - const { id, description, dateTimeOriginal, latitude, longitude, rating, tags } = job; + const { id, tags } = job; const asset = await this.assetJobRepository.getForSidecarWriteJob(id); if (!asset) { return JobStatus.Failed; } + const lockedProperties = await this.assetJobRepository.getLockedPropertiesForMetadataExtraction(id); const tagsList = (asset.tags || []).map((tag) => tag.value); const { sidecarFile } = getAssetFiles(asset.files); const sidecarPath = sidecarFile?.path || `${asset.originalPath}.xmp`; + const { description, dateTimeOriginal, latitude, longitude, rating } = _.pick( + { + description: asset.exifInfo.description, + dateTimeOriginal: asset.exifInfo.dateTimeOriginal, + latitude: asset.exifInfo.latitude, + longitude: asset.exifInfo.longitude, + rating: asset.exifInfo.rating, + }, + lockedProperties, + ); + const exif = _.omitBy( { Description: description, ImageDescription: description, - DateTimeOriginal: dateTimeOriginal, + DateTimeOriginal: dateTimeOriginal?.toISOString(), GPSLatitude: latitude, GPSLongitude: longitude, Rating: rating, diff --git a/server/src/types.ts b/server/src/types.ts index a33dba490c..e404332fac 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -222,11 +222,6 @@ export interface IDeleteFilesJob extends IBaseJob { } export interface ISidecarWriteJob extends IEntityJob { - description?: string; - dateTimeOriginal?: string; - latitude?: number; - longitude?: number; - rating?: number; tags?: true; } diff --git a/server/test/medium.factory.ts b/server/test/medium.factory.ts index efcdc59793..1fd255ef75 100644 --- a/server/test/medium.factory.ts +++ b/server/test/medium.factory.ts @@ -202,7 +202,7 @@ export class MediumTestContext { } async newExif(dto: Insertable) { - const result = await this.get(AssetRepository).upsertExif(dto); + const result = await this.get(AssetRepository).upsertExif(dto, { lockedPropertiesBehavior: 'none' }); return { result }; } diff --git a/server/test/medium/specs/services/metadata.service.spec.ts b/server/test/medium/specs/services/metadata.service.spec.ts index 1d144e9c9c..4c3499857d 100644 --- a/server/test/medium/specs/services/metadata.service.spec.ts +++ b/server/test/medium/specs/services/metadata.service.spec.ts @@ -95,6 +95,7 @@ describe(MetadataService.name, () => { dateTimeOriginal: new Date(expected.dateTimeOriginal), timeZone: expected.timeZone, }), + { lockedPropertiesBehavior: 'skip' }, ); expect(mocks.asset.update).toHaveBeenCalledWith( diff --git a/server/test/medium/specs/sync/sync-album-asset-exif.spec.ts b/server/test/medium/specs/sync/sync-album-asset-exif.spec.ts index fd563f4db1..2b53e612e2 100644 --- a/server/test/medium/specs/sync/sync-album-asset-exif.spec.ts +++ b/server/test/medium/specs/sync/sync-album-asset-exif.spec.ts @@ -288,10 +288,13 @@ describe(SyncRequestType.AlbumAssetExifsV1, () => { // update the asset const assetRepository = ctx.get(AssetRepository); - await assetRepository.upsertExif({ - assetId: asset.id, - city: 'New City', - }); + await assetRepository.upsertExif( + { + assetId: asset.id, + city: 'New City', + }, + { lockedPropertiesBehavior: 'update' }, + ); await expect(ctx.syncStream(auth, [SyncRequestType.AlbumAssetExifsV1])).resolves.toEqual([ { @@ -346,10 +349,13 @@ describe(SyncRequestType.AlbumAssetExifsV1, () => { // update the asset const assetRepository = ctx.get(AssetRepository); - await assetRepository.upsertExif({ - assetId: assetDelayedExif.id, - city: 'Delayed Exif', - }); + await assetRepository.upsertExif( + { + assetId: assetDelayedExif.id, + city: 'Delayed Exif', + }, + { lockedPropertiesBehavior: 'update' }, + ); await expect(ctx.syncStream(auth, [SyncRequestType.AlbumAssetExifsV1])).resolves.toEqual([ { diff --git a/server/test/small.factory.ts b/server/test/small.factory.ts index 13cccce176..53d7c70794 100644 --- a/server/test/small.factory.ts +++ b/server/test/small.factory.ts @@ -4,6 +4,7 @@ import { AuthApiKey, AuthSharedLink, AuthUser, + Exif, Library, Memory, Partner, @@ -319,18 +320,28 @@ const versionHistoryFactory = () => ({ version: '1.123.45', }); -const assetSidecarWriteFactory = () => ({ - id: newUuid(), - originalPath: '/path/to/original-path.jpg.xmp', - tags: [], - files: [ - { - id: newUuid(), - path: '/path/to/original-path.jpg.xmp', - type: AssetFileType.Sidecar, - }, - ], -}); +const assetSidecarWriteFactory = () => { + const id = newUuid(); + return { + id, + originalPath: '/path/to/original-path.jpg.xmp', + tags: [], + files: [ + { + id: newUuid(), + path: '/path/to/original-path.jpg.xmp', + type: AssetFileType.Sidecar, + }, + ], + exifInfo: { + assetId: id, + description: 'this is a description', + latitude: 12, + longitude: 12, + dateTimeOriginal: '2023-11-22T04:56:12.196Z', + } as unknown as Exif, + }; +}; const assetOcrFactory = ( ocr: {