From b26b0cc806a0cc0d586e95a359e0debce1bffc4f Mon Sep 17 00:00:00 2001 From: "Lin, Chiang-Yu" <110763842+RxChi1d@users.noreply.github.com> Date: Sun, 23 Aug 2026 01:58:05 +0800 Subject: [PATCH] fix(server): let a second metadata extraction replace stored AV metadata (#30900) `upsertExif` writes derived audio, video and keyframe metadata with an `ON CONFLICT DO UPDATE`, but every column in those three update lists assigns the stored row back to itself: bitrate: ref('asset_audio.bitrate') `SET bitrate = asset_audio.bitrate` is a self-assignment, so once a row exists nothing can change it. Re-running metadata extraction re-reads the file, builds a fresh snapshot, and then quietly discards it. The rest of the file already uses `excluded` for this, as does `plugin.repository.ts`. `asset_video.frameCount` had a second problem: it is supplied on insert but was missing from the update list, so it would have stayed stale even after the reference was corrected. Extraction is meant to be repeatable. Probing improves between releases, a file can be repaired or replaced, and a fix to how a stream is chosen is worthless if it cannot reach the assets that were already imported. An `upsert` that silently degrades to insert-only defeats all of that. Metadata extraction is the only caller that passes these three objects, and it passes a complete snapshot or nothing: each object is built from a single probe behind a guard, and `upsertExif` skips the branch entirely when the object is absent. So taking the incoming row cannot write partial values over good ones. The other three callers pass `exif` only, and every other reference to these tables is a read. --- server/src/repositories/asset.repository.ts | 51 +++---- .../repositories/asset.repository.spec.ts | 124 ++++++++++++++++++ 2 files changed, 150 insertions(+), 25 deletions(-) diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 189c5c621fb88..a7c520aab30b8 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -195,10 +195,10 @@ export class AssetRepository { .values(audio) .onConflict((oc) => oc.column('assetId').doUpdateSet(({ ref }) => ({ - bitrate: ref('asset_audio.bitrate'), - index: ref('asset_audio.index'), - profile: ref('asset_audio.profile'), - codecName: ref('asset_audio.codecName'), + bitrate: ref('excluded.bitrate'), + index: ref('excluded.index'), + profile: ref('excluded.profile'), + codecName: ref('excluded.codecName'), })), ), ); @@ -211,21 +211,22 @@ export class AssetRepository { .values(video) .onConflict((oc) => oc.column('assetId').doUpdateSet(({ ref }) => ({ - bitrate: ref('asset_video.bitrate'), - timeBase: ref('asset_video.timeBase'), - index: ref('asset_video.index'), - profile: ref('asset_video.profile'), - level: ref('asset_video.level'), - colorPrimaries: ref('asset_video.colorPrimaries'), - colorTransfer: ref('asset_video.colorTransfer'), - colorMatrix: ref('asset_video.colorMatrix'), - dvProfile: ref('asset_video.dvProfile'), - dvLevel: ref('asset_video.dvLevel'), - dvBlSignalCompatibilityId: ref('asset_video.dvBlSignalCompatibilityId'), - codecName: ref('asset_video.codecName'), - formatName: ref('asset_video.formatName'), - formatLongName: ref('asset_video.formatLongName'), - pixelFormat: ref('asset_video.pixelFormat'), + bitrate: ref('excluded.bitrate'), + frameCount: ref('excluded.frameCount'), + timeBase: ref('excluded.timeBase'), + index: ref('excluded.index'), + profile: ref('excluded.profile'), + level: ref('excluded.level'), + colorPrimaries: ref('excluded.colorPrimaries'), + colorTransfer: ref('excluded.colorTransfer'), + colorMatrix: ref('excluded.colorMatrix'), + dvProfile: ref('excluded.dvProfile'), + dvLevel: ref('excluded.dvLevel'), + dvBlSignalCompatibilityId: ref('excluded.dvBlSignalCompatibilityId'), + codecName: ref('excluded.codecName'), + formatName: ref('excluded.formatName'), + formatLongName: ref('excluded.formatLongName'), + pixelFormat: ref('excluded.pixelFormat'), })), ), ); @@ -238,12 +239,12 @@ export class AssetRepository { .values(keyframes) .onConflict((oc) => oc.column('assetId').doUpdateSet(({ ref }) => ({ - pts: ref('asset_keyframe.pts'), - accDuration: ref('asset_keyframe.accDuration'), - ownDuration: ref('asset_keyframe.ownDuration'), - totalDuration: ref('asset_keyframe.totalDuration'), - packetCount: ref('asset_keyframe.packetCount'), - outputFrames: ref('asset_keyframe.outputFrames'), + pts: ref('excluded.pts'), + accDuration: ref('excluded.accDuration'), + ownDuration: ref('excluded.ownDuration'), + totalDuration: ref('excluded.totalDuration'), + packetCount: ref('excluded.packetCount'), + outputFrames: ref('excluded.outputFrames'), })), ), ); diff --git a/server/test/medium/specs/repositories/asset.repository.spec.ts b/server/test/medium/specs/repositories/asset.repository.spec.ts index b623270b312e9..b3d3e1517c11b 100644 --- a/server/test/medium/specs/repositories/asset.repository.spec.ts +++ b/server/test/medium/specs/repositories/asset.repository.spec.ts @@ -23,6 +23,46 @@ beforeAll(async () => { defaultDatabase = await getKyselyDB(); }); +// Metadata extraction is repeatable: probing improves, files get repaired, +// and a re-run has to be able to correct what an earlier run stored. +const audioRow = (assetId: string, n: number) => ({ + assetId, + bitrate: 100_000 + n, + index: n, + profile: n, + codecName: `codec-${n}`, +}); + +const videoRow = (assetId: string, n: number) => ({ + assetId, + bitrate: 200_000 + n, + frameCount: 300 + n, + timeBase: 600 + n, + index: n, + profile: n, + level: n, + colorPrimaries: n, + colorTransfer: n, + colorMatrix: n, + dvProfile: n, + dvLevel: n, + dvBlSignalCompatibilityId: n, + codecName: `vcodec-${n}`, + formatName: `format-${n}`, + formatLongName: `format long ${n}`, + pixelFormat: `pixfmt-${n}`, +}); + +const keyframeRow = (assetId: string, n: number) => ({ + assetId, + pts: [n], + accDuration: [n], + ownDuration: [n], + totalDuration: 1000 + n, + packetCount: 10 + n, + outputFrames: 20 + n, +}); + describe(AssetRepository.name, () => { describe('getTimeBucket', () => { it('should order assets by local day first and fileCreatedAt within each day', async () => { @@ -80,6 +120,90 @@ describe(AssetRepository.name, () => { }); describe('upsertExif', () => { + it('should replace stored audio metadata on a second extraction', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'first' }, + audio: audioRow(asset.id, 2), + lockedPropertiesBehavior: 'skip', + }); + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'second' }, + audio: audioRow(asset.id, 1), + lockedPropertiesBehavior: 'skip', + }); + + await expect( + ctx.database.selectFrom('asset_audio').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(), + ).resolves.toEqual(audioRow(asset.id, 1)); + }); + + it('should replace stored video metadata on a second extraction', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'first' }, + video: videoRow(asset.id, 2), + lockedPropertiesBehavior: 'skip', + }); + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'second' }, + video: videoRow(asset.id, 1), + lockedPropertiesBehavior: 'skip', + }); + + await expect( + ctx.database.selectFrom('asset_video').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(), + ).resolves.toEqual(videoRow(asset.id, 1)); + }); + + it('should replace stored keyframe metadata on a second extraction', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'first' }, + keyframes: keyframeRow(asset.id, 2), + lockedPropertiesBehavior: 'skip', + }); + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'second' }, + keyframes: keyframeRow(asset.id, 1), + lockedPropertiesBehavior: 'skip', + }); + + await expect( + ctx.database.selectFrom('asset_keyframe').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(), + ).resolves.toEqual(keyframeRow(asset.id, 1)); + }); + + // A probe that could not read a stream sends no object at all, and that must + // not be read as "delete what is already known". + it('should leave stored media metadata alone when an extraction omits it', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { asset } = await ctx.newAsset({ ownerId: user.id }); + + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'first' }, + audio: audioRow(asset.id, 2), + lockedPropertiesBehavior: 'skip', + }); + await sut.upsertExif({ + exif: { assetId: asset.id, description: 'second' }, + lockedPropertiesBehavior: 'skip', + }); + + await expect( + ctx.database.selectFrom('asset_audio').selectAll().where('assetId', '=', asset.id).executeTakeFirstOrThrow(), + ).resolves.toEqual(audioRow(asset.id, 2)); + }); it('should append to locked columns', async () => { const { ctx, sut } = setup(); const { user } = await ctx.newUser();