diff --git a/backend/__tests__/routes/adminThumbnails.previewColumns.test.js b/backend/__tests__/routes/adminThumbnails.previewColumns.test.js new file mode 100644 index 00000000..a139e7b3 --- /dev/null +++ b/backend/__tests__/routes/adminThumbnails.previewColumns.test.js @@ -0,0 +1,42 @@ +/** + * Source-inspection contract test for #1078. + * + * POST /api/admin/thumbnails/regenerate-previews hands its selected rows to + * ensurePreviewImage, which branches on `source_origin` (and then reads + * `external_relpath` / `filename`) to reach an external/reference photo on its + * media mount. When the select list omitted those columns, every external row + * looked managed, resolvePhotoStorageKey returned null, and the endpoint + * reported success while silently generating nothing for reference galleries. + */ +const fs = require('fs'); +const path = require('path'); + +describe('regenerate-previews selects the columns ensurePreviewImage branches on (#1078)', () => { + const src = fs.readFileSync( + path.join(__dirname, '..', '..', 'src', 'routes', 'adminThumbnails.js'), + 'utf8', + ); + + // The select feeding the regenerate-previews handler, from the route + // declaration to the end of that statement. + const selectStatement = (() => { + const routeIdx = src.indexOf('/regenerate-previews'); + expect(routeIdx).toBeGreaterThan(-1); + const selectIdx = src.indexOf('.select(', routeIdx); + expect(selectIdx).toBeGreaterThan(-1); + return src.slice(selectIdx, src.indexOf(';', selectIdx)); + })(); + + it.each(['source_origin', 'external_relpath', 'filename'])( + 'selects %s', + (column) => { + expect(selectStatement).toContain(`'${column}'`); + } + ); + + it('still selects the columns the managed path needs', () => { + for (const column of ['id', 'event_id', 'path', 'media_type', 'mime_type', 'preview_path']) { + expect(selectStatement).toContain(`'${column}'`); + } + }); +}); diff --git a/backend/__tests__/services/ensurePreviewImage.external.test.js b/backend/__tests__/services/ensurePreviewImage.external.test.js new file mode 100644 index 00000000..7252dd01 --- /dev/null +++ b/backend/__tests__/services/ensurePreviewImage.external.test.js @@ -0,0 +1,218 @@ +/** + * Regression tests for #1078 — ensurePreviewImage must generate previews for + * external/reference photos, not silently fall back to the full-size original. + * + * resolvePhotoStorageKey returns null for external photos by design, and that + * null used to be handed straight to withLocalCopy, which throws. The lightbox + * preview route caught the throw and redirected to the original, so a gallery + * whose photos all live on an external mount paid full size on every open — + * the exact cost the preview tier (#492) exists to avoid. + */ +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); +const sharp = require('sharp'); + +// Must be set before externalMediaService is first required: it caches the +// resolved root on first call, and the dir has to exist to win over the +// container default. +const EXTERNAL_ROOT = path.join(os.tmpdir(), `picpeak-ext-media-${process.pid}`); +process.env.EXTERNAL_MEDIA_ROOT = EXTERNAL_ROOT; + +jest.mock('../../src/database/db', () => { + const state = { event: null, updates: [] }; + const api = (table) => { + if (table === 'events') { + return { where: () => ({ first: async () => state.event }) }; + } + if (table === 'photos') { + return { + where: (criteria) => ({ + update: async (values) => { + state.updates.push({ criteria, values }); + return 1; + }, + }), + }; + } + throw new Error(`unexpected table in test: ${table}`); + }; + api.__state = state; + return { db: api }; +}); + +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); +const storageModule = require('../../src/services/storage'); +const { db } = require('../../src/database/db'); + +const EVENT = { + id: 7, + slug: 'nas-wedding', + source_mode: 'reference', + external_path: 'weddings/2026-08-smith', +}; + +async function writeSourceJpeg(absPath, { width = 2400, height = 1600 } = {}) { + await fs.mkdir(path.dirname(absPath), { recursive: true }); + const buf = Buffer.alloc(width * height * 3); + for (let i = 0; i < buf.length; i++) buf[i] = (i * 7) % 256; + await sharp(buf, { raw: { width, height, channels: 3 } }).jpeg({ quality: 90 }).toFile(absPath); +} + +describe('ensurePreviewImage — external/reference sources (#1078)', () => { + let storage; + let storageRoot; + let imageProcessor; + + beforeAll(async () => { + storageRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-preview-store-')); + storage = new LocalFsStorage({ root: storageRoot }); + await storage.init(); + storageModule.setStorageForTesting(storage); + + // Require AFTER the storage injection so the module sees it. + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + + await fs.mkdir(path.join(EXTERNAL_ROOT, EVENT.external_path), { recursive: true }); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(storageRoot, { recursive: true, force: true }).catch(() => {}); + await fs.rm(EXTERNAL_ROOT, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(() => { + db.__state.event = EVENT; + db.__state.updates = []; + }); + + it.each(['external', 'reference'])( + 'generates a downscaled preview for a %s photo off the media mount', + async (sourceOrigin) => { + const relpath = `${sourceOrigin}-shot.jpg`; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + + const photo = { + id: sourceOrigin === 'external' ? 101 : 102, + event_id: EVENT.id, + source_origin: sourceOrigin, + external_relpath: relpath, + filename: relpath, + preview_path: null, + }; + + const key = await imageProcessor.ensurePreviewImage(photo); + + // Per-photo basename so two events referencing the same NAS filename + // can't clobber each other's preview. + expect(key).toBe(`previews/preview_ext${photo.id}_${relpath}`); + expect(await storage.exists(key)).toBe(true); + + const meta = await sharp(storage.resolveLocalPath(key)).metadata(); + expect(meta.format).toBe('jpeg'); + // 2400x1600 capped at the 1920 long edge, aspect preserved. + expect(meta.width).toBe(1920); + expect(meta.height).toBe(1280); + + // The generated key is persisted so the next open short-circuits. + expect(db.__state.updates).toEqual([ + { criteria: { id: photo.id }, values: { preview_path: key } }, + ]); + } + ); + + it('short-circuits on an existing valid preview instead of regenerating', async () => { + const relpath = 'already-previewed.jpg'; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + const photo = { + id: 103, + event_id: EVENT.id, + source_origin: 'external', + external_relpath: relpath, + filename: relpath, + preview_path: null, + }; + + const first = await imageProcessor.ensurePreviewImage(photo); + db.__state.updates = []; + + const second = await imageProcessor.ensurePreviewImage({ ...photo, preview_path: first }); + expect(second).toBe(first); + expect(db.__state.updates).toEqual([]); + }); + + it('returns null (never throws) when the external source is missing', async () => { + const photo = { + id: 104, + event_id: EVENT.id, + source_origin: 'external', + external_relpath: 'not-on-the-mount.jpg', + filename: 'not-on-the-mount.jpg', + preview_path: null, + }; + + await expect(imageProcessor.ensurePreviewImage(photo)).resolves.toBeNull(); + expect(db.__state.updates).toEqual([]); + }); + + it('returns null (never throws) for a row with no source_origin in a reference event', async () => { + // Mode falls back to event.source_mode = 'reference', so + // resolvePhotoStorageKey yields null. That used to reach withLocalCopy and + // throw out of ensurePreviewImage instead of honouring null-on-failure. + const photo = { + id: 105, + event_id: EVENT.id, + source_origin: null, + external_relpath: null, + filename: 'orphan.jpg', + path: 'nas-wedding/individual/orphan.jpg', + preview_path: null, + }; + + await expect(imageProcessor.ensurePreviewImage(photo)).resolves.toBeNull(); + expect(db.__state.updates).toEqual([]); + }); + + it('branches on source_origin, so a row selected without it looks managed', async () => { + // Pins why the /regenerate-previews caller must select source_origin: + // an external row missing that column takes the managed path, where + // resolvePhotoStorageKey yields null and generation is skipped. + const relpath = 'column-starved.jpg'; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + const starved = { + id: 106, + event_id: EVENT.id, + external_relpath: relpath, + preview_path: null, + }; + + await expect(imageProcessor.ensurePreviewImage(starved)).resolves.toBeNull(); + await expect( + imageProcessor.ensurePreviewImage({ ...starved, source_origin: 'external', filename: relpath }) + ).resolves.toBe(`previews/preview_ext106_${relpath}`); + }); + + it('still routes managed photos through the storage backend', async () => { + const sourceKey = 'events/active/managed-event/individual/managed.jpg'; + const localSource = path.join(os.tmpdir(), `picpeak-managed-${process.pid}.jpg`); + await writeSourceJpeg(localSource, { width: 800, height: 600 }); + await storage.put(sourceKey, await fs.readFile(localSource), { contentType: 'image/jpeg' }); + await fs.rm(localSource, { force: true }); + + db.__state.event = { id: 8, slug: 'managed-event', source_mode: 'managed' }; + const photo = { + id: 201, + event_id: 8, + source_origin: 'managed', + path: 'managed-event/individual/managed.jpg', + filename: 'managed.jpg', + preview_path: null, + }; + + const key = await imageProcessor.ensurePreviewImage(photo); + expect(key).toBe('previews/preview_managed.jpg'); + expect(await storage.exists(key)).toBe(true); + }); +}); diff --git a/backend/src/routes/adminThumbnails.js b/backend/src/routes/adminThumbnails.js index 1e3b8c55..c7d1e789 100644 --- a/backend/src/routes/adminThumbnails.js +++ b/backend/src/routes/adminThumbnails.js @@ -201,7 +201,13 @@ router.post('/regenerate-previews', adminAuth, requirePermission('photos.edit'), try { const { eventId } = req.body; - let query = db('photos').select('id', 'event_id', 'path', 'media_type', 'mime_type', 'preview_path'); + // source_origin/external_relpath/filename are what ensurePreviewImage + // branches on for external/reference rows (#1078) — without them every + // external photo looks managed here and generation is skipped. + let query = db('photos').select( + 'id', 'event_id', 'path', 'media_type', 'mime_type', 'preview_path', + 'source_origin', 'external_relpath', 'filename' + ); if (eventId) query = query.where('event_id', eventId); // Skip videos — preview tier is image-only. query = query.where(function() { diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index e519678d..f87c3571 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -658,17 +658,27 @@ async function isPreviewValid(previewPath) { * Lazy-generate the preview image for a photo if missing or invalid. * Returns the storage key or null on failure (callers fall back to * the original URL so the lightbox never shows a broken image). + * + * Handles both managed photos (via the storage backend, possibly S3) and + * external/reference photos (#1078 — sourced from a local mount outside the + * managed storage tree). Externals used to have no branch here at all: + * resolvePhotoStorageKey returns null for them by design, that null reached + * withLocalCopy, and the throw put every lightbox open back on the full-size + * original — the exact cost the preview tier (#492) exists to avoid. */ async function ensurePreviewImage(photo) { - const { resolvePhotoStorageKey } = require('./photoResolver'); + const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); - let sourceKey; + let event; try { - const event = await db('events').where('id', photo.event_id).first(); - sourceKey = resolvePhotoStorageKey(event, photo); + event = await db('events').where('id', photo.event_id).first(); } catch (e) { const msg = (e && e.message) ? e.message : String(e); - logger.error(`Failed to resolve original key for preview (photo ${photo.id}): ${msg}`); + logger.error(`Failed to load event for preview (photo ${photo.id}): ${msg}`); + return null; + } + if (!event) { + logger.error(`ensurePreviewImage: event ${photo.event_id} not found for photo ${photo.id}`); return null; } @@ -678,14 +688,51 @@ async function ensurePreviewImage(photo) { logger.warn(`Invalid preview detected for photo ${photo.id}, regenerating…`); } - const newPreviewPath = await withLocalCopy(sourceKey, async (localPath) => { - const proc = await withProcessableImage(localPath, sourceKey); + const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; + + let newPreviewPath; + if (isExternal) { + // Mirrors ensureThumbnail's external branch: the source is a direct fs + // read off the mount, so no withLocalCopy. The per-photo outputBasename + // keeps two events that reference the same NAS basename from clobbering + // each other's preview. + let localPath; try { - return await generatePreviewImage(proc.path, { regenerate: true, outputBasename: proc.outputBasename }); - } finally { - await proc.cleanup(); + localPath = resolvePhotoFilePath(event, photo); + } catch (e) { + logger.error(`Failed to resolve external file for preview (photo ${photo.id}): ${e.message}`); + return null; } - }); + const sourceBasename = path.basename(photo.external_relpath || photo.filename || `photo-${photo.id}`); + const outputBasename = `ext${photo.id}_${sourceBasename}`; + logger.info(`Ensuring preview for external photo ${photo.id} from ${localPath}`); + newPreviewPath = await generatePreviewImage(localPath, { regenerate: true, outputBasename }); + } else { + let sourceKey; + try { + sourceKey = resolvePhotoStorageKey(event, photo); + } catch (e) { + const msg = (e && e.message) ? e.message : String(e); + logger.error(`Failed to resolve original key for preview (photo ${photo.id}): ${msg}`); + return null; + } + if (!sourceKey) { + // Reference-mode event holding a row with no source_origin: the mode + // falls back to the event's and resolvePhotoStorageKey returns null. + // Honour the documented null-on-failure contract instead of feeding + // null into withLocalCopy, which throws out of this function. + logger.warn(`No managed storage key for preview (photo ${photo.id}); skipping preview generation`); + return null; + } + newPreviewPath = await withLocalCopy(sourceKey, async (localPath) => { + const proc = await withProcessableImage(localPath, sourceKey); + try { + return await generatePreviewImage(proc.path, { regenerate: true, outputBasename: proc.outputBasename }); + } finally { + await proc.cleanup(); + } + }); + } if (newPreviewPath) { await db('photos').where({ id: photo.id }).update({ preview_path: newPreviewPath });