From 6ca8baab23a659b1329dded05002722fd9eaddbc Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sat, 29 Aug 2026 12:22:50 +0200 Subject: [PATCH] fix(watcher): stop re-importing a photo whose file was replaced (#1226) (#1237) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existence check matched on filename OR path. replacePhoto regenerates both — a fresh generated filename and a fresh managed path — so a watched-folder photo that had its file replaced stopped matching either arm. The original is still sitting in the watched folder, so the next sweep imported it again and the gallery ended up holding the delivered edit AND the untouched original: the same duplicate shape external_relpath prevents for reference galleries. source_filename is now a third arm. It is the stable key here — written once at ingest by this same path and preserved across a replace by design. Rows predating migration 193 are covered by its backfill: COALESCE(original_filename, filename), and this path never wrote original_filename, so for watcher rows that resolves to the basename being compared. The query is lifted into an exported findExistingPhoto() so the test drives it rather than a copy — the thing under test IS the query, so a query-builder mock would only assert that knex was called the way the test expects. Predates the Lightroom round-trip and applies to the admin replace path too; it became reachable when #1165 brought watcher galleries into round-trip scope. Six tests against a real SQLite database. The load-bearing one fails without the change, verified by removing the arm and re-running; the other five pin what must not move — filename and path matching, the pre-193 backfill shape, a genuinely new file, and event scoping. Co-authored-by: Paul Nothaft --- .../services/fileWatcherReplaceDedupe.test.js | 171 ++++++++++++++++++ backend/src/services/fileWatcher.js | 48 ++++- 2 files changed, 210 insertions(+), 9 deletions(-) create mode 100644 backend/__tests__/services/fileWatcherReplaceDedupe.test.js diff --git a/backend/__tests__/services/fileWatcherReplaceDedupe.test.js b/backend/__tests__/services/fileWatcherReplaceDedupe.test.js new file mode 100644 index 00000000..c048302e --- /dev/null +++ b/backend/__tests__/services/fileWatcherReplaceDedupe.test.js @@ -0,0 +1,171 @@ +/** + * The watcher must not re-import a photo whose file was replaced (#1226). + * + * `replacePhoto` generates a fresh `filename` AND a fresh managed `path`, so a + * watched-folder photo that has been replaced — through the Lightroom + * round-trip (#745) or the admin replace — matched neither of the two arms the + * existence check used to have. The original file is still sitting in the + * watched folder, so the next sweep imported it again and the gallery ended up + * holding the delivered edit AND the untouched original. + * + * `source_filename` is the stable key: written once at ingest and preserved + * across a replace by design (migration 193). + * + * Runs against a real SQLite database rather than a query-builder mock — the + * thing under test IS the query, so a mock would only assert that knex was + * called the way the test expects. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-watcher-dedupe-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'watcher-dedupe-test-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); +const { findExistingPhoto } = require('../../src/services/fileWatcher'); + +const EVENT_SLUG = 'watcher-dedupe-event'; +const WATCHED_BASENAME = 'IMG_1234.JPG'; +const WATCHED_RELPATH = `${EVENT_SLUG}/individual/IMG_1234.JPG`; + +let db; +let cleanup; +let eventId; + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + const inserted = await db('events').insert({ + slug: EVENT_SLUG, + event_type: 'wedding', + event_name: 'Watcher Dedupe', + event_date: '2026-08-29', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: `/gallery/${EVENT_SLUG}/share`, + share_token: 'watcher-dedupe-share', + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + eventId = inserted[0]?.id ?? inserted[0]; +}, 120000); + +afterEach(async () => { + await db('photos').where({ event_id: eventId }).del(); +}); + +afterAll(async () => { + if (cleanup) await cleanup(); +}); + +async function addPhoto(row) { + const inserted = await db('photos').insert({ + event_id: eventId, + type: 'individual', + uploaded_at: new Date().toISOString(), + ...row, + }).returning('id'); + return inserted[0]?.id ?? inserted[0]; +} + +describe('fileWatcher existence check (#1226)', () => { + it('recognises a photo whose file was replaced, so it is not imported twice', async () => { + // Exactly the post-replace row: filename and path both regenerated by + // replacePhoto, source_filename still holding what the watcher imported. + const id = await addPhoto({ + filename: 'wedding_individual_1755892345.jpg', + source_filename: WATCHED_BASENAME, + path: 'events/active/wedding/individual/wedding_individual_1755892345.jpg', + }); + + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found).toBeTruthy(); + expect(found.id).toBe(id); + }); + + it('still recognises an ordinary watcher import by filename', async () => { + const id = await addPhoto({ + filename: WATCHED_BASENAME, + source_filename: WATCHED_BASENAME, + path: WATCHED_RELPATH, + }); + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found.id).toBe(id); + }); + + it('still recognises a row by path when the name differs', async () => { + const id = await addPhoto({ + filename: 'something-else.jpg', + source_filename: 'something-else.jpg', + path: WATCHED_RELPATH, + }); + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found.id).toBe(id); + }); + + it('covers rows predating migration 193, whose backfill set source_filename', async () => { + // The watcher never wrote original_filename, so COALESCE resolved to + // filename — the same basename this compares against. + const id = await addPhoto({ + filename: WATCHED_BASENAME, + original_filename: null, + source_filename: WATCHED_BASENAME, + path: 'events/active/wedding/individual/old-layout.jpg', + }); + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found.id).toBe(id); + }); + + it('does not match a genuinely new file', async () => { + await addPhoto({ + filename: 'IMG_9999.JPG', + source_filename: 'IMG_9999.JPG', + path: `${EVENT_SLUG}/individual/IMG_9999.JPG`, + }); + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found).toBeFalsy(); + }); + + it('is scoped to the event — the same basename elsewhere is not a match', async () => { + const other = await db('events').insert({ + slug: 'other-watcher-event', + event_type: 'wedding', + event_name: 'Other', + event_date: '2026-08-29', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: '/gallery/other-watcher-event/share', + share_token: 'other-watcher-share', + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + const otherId = other[0]?.id ?? other[0]; + await db('photos').insert({ + event_id: otherId, + type: 'individual', + uploaded_at: new Date().toISOString(), + filename: WATCHED_BASENAME, + source_filename: WATCHED_BASENAME, + path: `other-watcher-event/individual/${WATCHED_BASENAME}`, + }); + + const found = await findExistingPhoto(eventId, WATCHED_BASENAME, WATCHED_RELPATH); + expect(found).toBeFalsy(); + + await db('photos').where({ event_id: otherId }).del(); + await db('events').where({ id: otherId }).del(); + }); +}); diff --git a/backend/src/services/fileWatcher.js b/backend/src/services/fileWatcher.js index cd699da5..203c2a9c 100644 --- a/backend/src/services/fileWatcher.js +++ b/backend/src/services/fileWatcher.js @@ -63,6 +63,27 @@ function startFileWatcher() { logger.info('File watcher started'); } +/** + * Has this watched file already been imported into this event? + * + * Exported so the regression test drives this query rather than a copy of it. + * See the caller for why source_filename is one of the arms. + * + * @param {number} eventId + * @param {string} basename the file's basename on disk + * @param {string} relativePath the path the import would store + */ +async function findExistingPhoto(eventId, basename, relativePath) { + return db('photos') + .where({ event_id: eventId }) + .where(function() { + this.where('filename', basename) + .orWhere('source_filename', basename) + .orWhere('path', relativePath); + }) + .first(); +} + async function processNewPhoto(filePath) { const relativePath = path.relative(WATCH_PATH(), filePath); const pathParts = relativePath.split(path.sep); @@ -122,14 +143,23 @@ async function processNewPhoto(filePath) { } } - // Check if photo already exists (by filename or path, to handle replacements) - const existingPhoto = await db('photos') - .where({ event_id: event.id }) - .where(function() { - this.where('filename', path.basename(filePath)) - .orWhere('path', relativePath); - }) - .first(); + // Check if photo already exists. + // + // `source_filename` is in here, not just filename/path, because a REPLACEMENT + // changes both of those (#1226). replacePhoto generates a fresh filename and + // a fresh managed path, so a watched-folder photo that had its file replaced + // — through the Lightroom round-trip (#745) or the admin replace — stopped + // matching either arm, and the next sweep imported the untouched original a + // second time. The gallery then held the edit AND the original: the same + // duplicate shape external_relpath prevents for reference galleries. + // + // source_filename is the stable key: written once at ingest (below) and + // preserved across a replace by design. Rows predating migration 193 are + // covered too — its backfill sets source_filename from + // COALESCE(original_filename, filename), and this path never wrote + // original_filename, so for watcher rows that resolves to the basename this + // compares against. + const existingPhoto = await findExistingPhoto(event.id, path.basename(filePath), relativePath); if (!existingPhoto) { // Add to database @@ -192,4 +222,4 @@ async function removePhoto(filePath) { logger.info(`Removed photo: ${relativePath}`); } -module.exports = { startFileWatcher }; +module.exports = { startFileWatcher, findExistingPhoto };