* fix(preview): generate lightbox previews for external/reference photos (#1078) ensurePreviewImage() resolved its source only via resolvePhotoStorageKey(), which returns null for external/reference photos by design — those live on a media mount outside the managed storage tree. The null went straight into withLocalCopy(), which throws ("LocalFsStorage: invalid relative path: null"), so the preview route fell back to redirecting at the full-size original. A gallery whose photos are all external got no benefit from the preview tier (#492) at all: guests paid 5-12 MB on every lightbox open, with nothing surfaced in the admin UI. Add the external branch ensureThumbnail() has had since #423: resolve via resolvePhotoFilePath() and feed the mount path to generatePreviewImage() directly, with an ext<id>_ output basename so two events referencing the same NAS filename can't clobber each other's preview. Also close the adjacent hole that made the failure a throw rather than the documented null: a row with no source_origin in a reference-mode event takes its mode from the event, so resolvePhotoStorageKey returns null for it too. Return null instead of handing that to withLocalCopy. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(preview): select the columns the external branch needs on bulk regenerate POST /api/admin/thumbnails/regenerate-previews selected only id, event_id, path, media_type, mime_type and preview_path, so photo.source_origin was undefined by the time ensurePreviewImage branched on it. Every external row in a reference gallery took the managed path, resolvePhotoStorageKey returned null for it, and the endpoint reported success while generating nothing. Add source_origin, external_relpath and filename to the select, plus a source-inspection test pinning the caller contract and a service-level test showing a column-starved row is indistinguishable from a managed one. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * style(test): single-quote the source-inspection needles Matches the repo eslint quotes rule (no avoidEscape) by dropping the nested quotes from the search strings rather than escaping them. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
This commit is contained in:
@@ -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}'`);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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() {
|
||||
|
||||
@@ -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 });
|
||||
|
||||
Reference in New Issue
Block a user