From 4105c099c947a30ea406349db57a96f5b629275b Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sat, 29 Aug 2026 12:15:31 +0200 Subject: [PATCH] test(external): make the fold-collision guard test the real code (#745 follow-up) (#1234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The regression test added with #1165 re-implemented the claim ordering and the claim loop inside the test file and asserted against its own copy. It never required externalRelpathFold, so changing the real sort left it green — a guard against silently deleting a client's delivered edit that guarded nothing. The ordering is now a named, exported claimOrderFor() and the test drives it. Verified by sabotage: replacing the comparator with `return 0` fails the test, where before it passed. Three cases added while the seam existed: the managed row wins from BOTH input orders (the original bug was that the survivor was whichever came first, so one order proves nothing), the sort is stable for rows of the same kind, and it does not mutate the caller's array. No behaviour change — the comparator is byte-identical, only lifted out. Co-authored-by: Paul Nothaft --- .../services/lightroomRoundtrip.test.js | 65 ++++++++++++++----- backend/src/services/externalRelpathFold.js | 49 +++++++++----- 2 files changed, 80 insertions(+), 34 deletions(-) diff --git a/backend/__tests__/services/lightroomRoundtrip.test.js b/backend/__tests__/services/lightroomRoundtrip.test.js index d3eb5835..bea04467 100644 --- a/backend/__tests__/services/lightroomRoundtrip.test.js +++ b/backend/__tests__/services/lightroomRoundtrip.test.js @@ -222,34 +222,67 @@ describe('migration 193 backfill reaches watcher and external rows', () => { }); describe('externalRelpathFold — a delivered edit must survive a collision', () => { - it('claims managed rows first, so they win and the external row loses', () => { - // The fold DELETES collision losers, and the survivor used to be - // whichever row was claimed first. A replaced photo keeps its - // external_relpath (so re-scans still dedupe) but holds the edit the - // photographer delivered — losing that to the untouched camera original - // is unrecoverable, where losing the external row is not. - const placements = [ - [{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], - [{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], - ]; - const claimOrder = placements.slice().sort((a, b) => { - const aManaged = a[0].source_origin === 'managed' ? 0 : 1; - const bManaged = b[0].source_origin === 'managed' ? 0 : 1; - return aManaged - bManaged; - }); + // Drives the REAL claimOrderFor. The previous version of this test + // re-implemented the sort and the claim loop inline and asserted against its + // own copy, so it stayed green no matter what the service did — a guard + // against silently deleting a client's delivered edit that guarded nothing. + const { claimOrderFor } = require('../../src/services/externalRelpathFold'); + // The same claim loop the fold runs, fed by the real ordering. + const claim = (placements) => { const claimed = new Map(); const losers = new Map(); - for (const [row, chosen] of claimOrder) { + for (const [row, chosen] of claimOrderFor(placements)) { const next = chosen ? `${chosen}/${row.external_relpath}` : row.external_relpath; const winner = claimed.get(next); if (winner != null) { losers.set(row.id, winner); continue; } claimed.set(next, row.id); } + return { claimed, losers }; + }; + it('claims managed rows first, so they win and the external row loses', () => { + // The fold DELETES collision losers. A replaced photo keeps its + // external_relpath (so re-scans still dedupe) but holds the edit the + // photographer delivered — losing that is unrecoverable, where losing the + // external row is not: it is still on the share and a re-scan re-imports it. + const { claimed, losers } = claim([ + [{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], + [{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], + ]); expect([...losers.keys()]).toEqual([1]); expect([...claimed.values()]).toEqual([2]); }); + + it('wins regardless of the order the rows arrive in', () => { + // The bug was that the survivor was whichever row came first, so the + // managed row has to win from BOTH input orders or the fix is a coin flip. + const { losers } = claim([ + [{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], + [{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'], + ]); + expect([...losers.keys()]).toEqual([1]); + }); + + it('leaves rows of the same kind in their original order', () => { + // A stable sort matters: reordering external rows among themselves would + // change which one survives an external-vs-external collision for no reason. + const rows = [ + [{ id: 10, source_origin: 'external', external_relpath: 'a.jpg' }, ''], + [{ id: 11, source_origin: 'external', external_relpath: 'b.jpg' }, ''], + [{ id: 12, source_origin: null, external_relpath: 'c.jpg' }, ''], + ]; + expect(claimOrderFor(rows).map(([r]) => r.id)).toEqual([10, 11, 12]); + }); + + it('does not mutate the caller\'s array', () => { + const rows = [ + [{ id: 1, source_origin: 'external', external_relpath: 'a.jpg' }, ''], + [{ id: 2, source_origin: 'managed', external_relpath: 'b.jpg' }, ''], + ]; + claimOrderFor(rows); + expect(rows.map(([r]) => r.id)).toEqual([1, 2]); + }); }); describe('mergeMarks', () => { diff --git a/backend/src/services/externalRelpathFold.js b/backend/src/services/externalRelpathFold.js index d7eae2b4..6147bb73 100644 --- a/backend/src/services/externalRelpathFold.js +++ b/backend/src/services/externalRelpathFold.js @@ -125,6 +125,35 @@ async function rootUsable(root) { } } +/** + * The order rows claim their folded path in — managed rows first (#745). + * + * A replaced photo keeps its external_relpath so the folder re-scan can still + * dedupe on it, but its source_origin is now 'managed' and its file is the edit + * the photographer delivered. Collisions below DELETE the loser, and the + * survivor used to be whichever row happened to be claimed first — so a + * delivered edit could be destroyed in favour of the untouched camera original + * sitting next to it on the share. + * + * Managed rows claim first, which makes them the survivor in any collision. The + * external row that loses is the recoverable one: it is still on the share and + * a re-scan re-imports it. The edit is not. + * + * Exported so the regression test can drive THIS function rather than a copy of + * it — a test that re-implements the ordering passes no matter what this does. + * The sort is stable, so rows of the same kind keep their relative order. + * + * @param {Array<[object, string|null]>} placements [row, chosenBase] pairs + * @returns {Array<[object, string|null]>} the same pairs, managed rows first + */ +function claimOrderFor(placements) { + return placements.slice().sort((a, b) => { + const aManaged = a[0].source_origin === 'managed' ? 0 : 1; + const bManaged = b[0].source_origin === 'managed' ? 0 : 1; + return aManaged - bManaged; + }); +} + /** * @param {import('knex')} knex a knex instance, or a transaction from a caller * that is already inside one (the restore). @@ -228,23 +257,7 @@ async function foldExternalRelpaths(knex, log = () => {}) { const resolved = []; const losers = new Map(); - // A replaced photo keeps its external_relpath so the folder re-scan can - // still dedupe on it (#745), but its source_origin is now 'managed' and - // its file is the edit the photographer delivered. Collisions here DELETE - // the loser, and the survivor was whichever row happened to be claimed - // first — so a delivered edit could be destroyed in favour of the - // untouched camera original sitting next to it on the share. - // - // Managed rows claim first, which makes them the survivor in any - // collision. The external row that loses is the recoverable one: it is - // still on the share and a re-scan re-imports it. The edit is not. - const claimOrder = placements.slice().sort((a, b) => { - const aManaged = a[0].source_origin === 'managed' ? 0 : 1; - const bManaged = b[0].source_origin === 'managed' ? 0 : 1; - return aManaged - bManaged; - }); - - for (const [row, chosen] of claimOrder) { + for (const [row, chosen] of claimOrderFor(placements)) { const next = chosen ? `${chosen}/${row.external_relpath}` : row.external_relpath; const winner = claimed.get(next); if (winner != null) { losers.set(row.id, winner); collided++; continue; } @@ -318,4 +331,4 @@ async function foldExternalRelpaths(knex, log = () => {}) { } -module.exports = { foldExternalRelpaths, MARKER }; +module.exports = { foldExternalRelpaths, claimOrderFor, MARKER };