diff --git a/backend/__tests__/integration/adminArchives.restoreCategories.test.js b/backend/__tests__/integration/adminArchives.restoreCategories.test.js new file mode 100644 index 00000000..723efc9f --- /dev/null +++ b/backend/__tests__/integration/adminArchives.restoreCategories.test.js @@ -0,0 +1,477 @@ +/** + * Restoring an archive must put the photos back into their categories. + * + * The archive writer already persists `category_name` per photo in + * `photos_manifest.json` — that is why the manifest exists, and the comment + * above it says so: "(and category linkage) can't be derived from the + * extracted files alone". The restore route then read only + * `original_filename` from it and kept deriving the category from the ZIP's + * first path segment. + * + * Archives store photos exactly as they sit on disk, so an event whose photos + * live in the gallery root produces a FLAT zip. `path.dirname()` is '.' for + * every entry, no category is resolved, and every restored photo lands with + * `category_id = null` — silently, with a 200 response. + * + * These pin the manifest as the source of truth, with the directory as the + * fallback that keeps foldered and legacy archives working. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const express = require('express'); +const request = require('supertest'); + +describe('archive restore restores categories (flat archives included)', () => { + let tmpDir; let db; let cleanup; let app; let storagePath; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-restore-cat-')); + storagePath = path.join(tmpDir, 'storage'); + process.env.NODE_ENV = 'test'; + process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'test.db'); + process.env.STORAGE_PATH = storagePath; + await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true }); + await fs.promises.mkdir(path.join(storagePath, 'archives'), { recursive: true }); + + jest.resetModules(); + jest.doMock('../../src/middleware/auth', () => ({ + adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); }, + })); + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + })); + jest.doMock('../../src/middleware/ownership', () => ({ + requireEventOwnership: (_req, _res, next) => next(), + })); + + ({ db, cleanup } = await require('./helpers/crmDb').bootCrmDb()); + // bootCrmDb points STORAGE_PATH at its own tmp dir; follow it rather than + // fighting it, so the archives the tests write are where the route looks. + storagePath = process.env.STORAGE_PATH; + await fs.promises.mkdir(path.join(storagePath, 'archives'), { recursive: true }); + + app = express(); + app.use(express.json()); + app.use('/admin/archives', require('../../src/routes/adminArchives')); + }, 180000); + + afterAll(async () => { + if (cleanup) await cleanup(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + await db('photos').del(); + await db('photo_categories').del(); + await db('events').del(); + }); + + /** A one-pixel JPEG is enough; the route only stats the extracted file. */ + const PIXEL = Buffer.from( + '/9j/4AAQSkZJRgABAQEAYABgAAD/2wBDAAgGBgcGBQgHBwcJCQgKDBQNDAsLDBkSEw8UHRofHh0a' + + 'HBwgJC4nICIsIxwcKDcpLDAxNDQ0Hyc5PTgyPC4zNDL/wAALCAABAAEBAREA/8QAFAABAAAAAAAA' + + 'AAAAAAAAAAAACf/EABQQAQAAAAAAAAAAAAAAAAAAAAD/2gAIAQEAAD8AKp//2Q==', + 'base64', + ); + + async function writeArchive(name, entries) { + // Required lazily: the suite calls jest.resetModules() in beforeAll, and + // archiver's readable-stream copy does not survive being split across the + // two module registries. + const archiver = require('archiver'); + const archivePath = path.join(storagePath, 'archives', name); + await new Promise((resolve, reject) => { + const output = fs.createWriteStream(archivePath); + const zip = archiver('zip', { zlib: { level: 0 } }); + output.on('close', resolve); + zip.on('error', reject); + zip.pipe(output); + for (const [entryName, buffer] of Object.entries(entries)) { + zip.append(buffer, { name: entryName }); + } + zip.finalize(); + }); + return path.join('archives', name); + } + + async function seedArchivedEvent(archiveRelPath, slug) { + const [row] = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: slug, + event_date: '2026-06-27', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `${slug}-share`, + expires_at: new Date().toISOString(), + is_archived: 1, // sqlite stores booleans as 0/1, see utils/dbCompat + archive_path: archiveRelPath, + }).returning('id'); + return typeof row === 'object' ? row.id : row; + } + + const categoryOf = async (filename) => { + const photo = await db('photos').where('filename', filename).first(); + if (!photo || !photo.category_id) return null; + const category = await db('photo_categories').where('id', photo.category_id).first(); + return category ? category.name : null; + }; + + it('takes the category from the manifest when the archive is flat', async () => { + // Exactly the shape a gallery-root event archives to: no directories. + const manifest = JSON.stringify([ + { filename: 'a.jpg', original_filename: 'DSC_0001.jpg', category_name: 'Polterabend' }, + { filename: 'b.jpg', original_filename: 'DSC_0002.jpg', category_name: 'Ceremony' }, + ]); + const archiveRelPath = await writeArchive('flat.zip', { + 'a.jpg': PIXEL, + 'b.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(manifest, 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'flat-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + // The whole bug: both of these used to be null. + expect(await categoryOf('a.jpg')).toBe('Polterabend'); + expect(await categoryOf('b.jpg')).toBe('Ceremony'); + }); + + it('reuses an existing category row instead of creating a duplicate', async () => { + const archiveRelPath = await writeArchive('reuse.zip', { + 'c.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'c.jpg', original_filename: 'DSC_0003.jpg', category_name: 'Party' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'reuse-event'); + await db('photo_categories').insert({ + event_id: eventId, name: 'Party', slug: 'party', created_at: new Date(), + }); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('c.jpg')).toBe('Party'); + const rows = await db('photo_categories').where({ event_id: eventId, name: 'Party' }); + expect(rows).toHaveLength(1); + }); + + it('still falls back to the directory for legacy archives with no manifest', async () => { + // No manifest at all — the shape every archive had before the manifest + // landed. The directory is the only signal left, and it must keep working. + // + // `individual/` is what a REAL archive contains: entry names are the + // storage key minus `events/active/{slug}`, and that layout is + // `individual/` / `collages/`. Categories have never been directories, so + // the fallback invents a category with that name — not useful, but better + // than losing every category, and this pins what actually happens rather + // than a category-shaped folder no archive produces. + const archiveRelPath = await writeArchive('foldered.zip', { + 'individual/d.jpg': PIXEL, + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'foldered-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('d.jpg')).toBe('individual'); + }); + + it('reuses a GLOBAL category instead of cloning it into the event', async () => { + // Seeded categories (Ceremony, Reception, ...) have event_id NULL. An + // event-only lookup misses them, so the restore used to create a second + // "Ceremony" — and because is_global defaults to TRUE, that duplicate then + // appeared in every other event's category list. + const [g] = await db('photo_categories').insert({ + event_id: null, name: 'Ceremony', slug: 'ceremony', is_global: true, created_at: new Date(), + }).returning('id'); + const globalId = typeof g === 'object' ? g.id : g; + + const archiveRelPath = await writeArchive('global.zip', { + 'individual/gl.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'gl.jpg', original_filename: 'DSC_1.jpg', category_name: 'Ceremony' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'global-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + const photo = await db('photos').where('filename', 'gl.jpg').first(); + expect(photo.category_id).toBe(globalId); + // No clone, global or otherwise. + const all = await db('photo_categories').where('name', 'Ceremony'); + expect(all).toHaveLength(1); + }); + + it('does not create a GLOBAL category when it has to invent one', async () => { + // is_global defaults to true on this column, so an unqualified insert would + // leak a restore's category name into every gallery on the instance. + const archiveRelPath = await writeArchive('newcat.zip', { + 'individual/nc.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'nc.jpg', original_filename: 'DSC_2.jpg', category_name: 'Polterabend' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'newcat-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + const created = await db('photo_categories').where('name', 'Polterabend').first(); + expect(created.event_id).toBe(eventId); + expect(created.is_global === false || created.is_global === 0).toBe(true); + }); + + it('matches the manifest when the ZIP was written with original filenames', async () => { + // With general_use_original_filenames_for_downloads on at archive time, + // archiveService names entries after the ORIGINAL filename while the + // manifest stays keyed by photos.filename. Looking up the extracted + // basename missed every entry, so categories were lost on exactly those + // archives. + const archiveRelPath = await writeArchive('original-names.zip', { + 'individual/DSC_4242.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'stored_9f8e7d.jpg', original_filename: 'DSC_4242.jpg', category_name: 'Drohne' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'original-names-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('DSC_4242.jpg')).toBe('Drohne'); + }); + + it('prefers the event-scoped category when a global shares its name', async () => { + // The category API permits both. A single OR-lookup with .first() returned + // whichever the engine chose, so a photo could be reassigned to the global + // row and lose event-local settings such as allow_downloads. + const archiveRelPath = await writeArchive('collide.zip', { + 'individual/co.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'co.jpg', original_filename: 'DSC_3.jpg', category_name: 'Reception' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'collide-event'); + + await db('photo_categories').insert({ + event_id: null, name: 'Reception', slug: 'reception-global', is_global: true, created_at: new Date(), + }); + const [own] = await db('photo_categories').insert({ + event_id: eventId, name: 'Reception', slug: 'reception-own', is_global: false, created_at: new Date(), + }).returning('id'); + const ownId = typeof own === 'object' ? own.id : own; + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + const photo = await db('photos').where('filename', 'co.jpg').first(); + expect(photo.category_id).toBe(ownId); + }); + + it('matches a sanitized original filename, as the ZIP would have written it', async () => { + // archiveService runs original names through sanitizeForZipEntry() before + // writing the entry, so the emitted name differs from the manifest column. + const archiveRelPath = await writeArchive('sanitized.zip', { + 'individual/od_dr_DSC_5.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'stored_abc.jpg', original_filename: 'od/dr/DSC_5.jpg', category_name: 'Strand' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'sanitized-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('od_dr_DSC_5.jpg')).toBe('Strand'); + }); + + it('ignores a legacy event-owned row when falling back to globals', async () => { + // The bug fixed here left rows behind on upgraded instances: event-owned + // AND is_global true, because the column defaults true. Matching on the + // flag alone would let one event's leftover be adopted by another event's + // restore, tying photos to a category that vanishes with someone else's + // gallery. + const otherEventId = await seedArchivedEvent('archives/none.zip', 'legacy-owner-event'); + await db('photo_categories').insert({ + event_id: otherEventId, name: 'Sunset', slug: 'sunset-legacy', + is_global: true, created_at: new Date(), + }); + + const archiveRelPath = await writeArchive('legacy-global.zip', { + 'individual/lg.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'lg.jpg', original_filename: 'DSC_6.jpg', category_name: 'Sunset' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'legacy-global-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + const photo = await db('photos').where('filename', 'lg.jpg').first(); + const cat = await db('photo_categories').where('id', photo.category_id).first(); + // Its own row, not the other event's leftover. + expect(cat.event_id).toBe(eventId); + }); + + it('drops an ambiguous original-name alias rather than guessing', async () => { + // Two photos in different ZIP folders can share an original basename; + // archiveService treats the paths as distinct and suffixes neither. Both + // would collapse onto one alias, and whichever won would hand the other + // photo someone else's category. + const archiveRelPath = await writeArchive('ambiguous.zip', { + 'individual/SHARED.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'a_stored.jpg', original_filename: 'SHARED.jpg', category_name: 'Alpha' }, + { filename: 'b_stored.jpg', original_filename: 'SHARED.jpg', category_name: 'Beta' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'ambiguous-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + // Falls back to the directory rather than picking Alpha or Beta at random. + expect(await categoryOf('SHARED.jpg')).toBe('individual'); + for (const name of ['Alpha', 'Beta']) { + expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy(); + } + }); + + it('honours a manifest that says UNCATEGORIZED, instead of inventing one from the directory', async () => { + // The case the manifest-first change was for. A real archive puts every + // photo under `individual/`, so a photo the manifest records as having no + // category used to come back filed under a category called "individual" — + // the manifest being authoritative for "category X" but not for "none". + const manifest = JSON.stringify([ + { filename: 'u.jpg', original_filename: 'DSC_7000.jpg', category_name: null }, + ]); + const archiveRelPath = await writeArchive('uncategorized.zip', { + 'individual/u.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(manifest, 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'uncategorized-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('u.jpg')).toBeNull(); + // And no junk category row was created as a side effect. + const rows = await db('photo_categories').where({ event_id: eventId }); + expect(rows).toHaveLength(0); + }); + + it('drops a canonical filename that two photos claim, rather than guessing', async () => { + // photos.filename is not unique within an event: s3AutoImporter takes + // path.basename(entry.key) and dedupes by path, so two imported files in + // different subfolders both land as IMG_1234.jpg. Both ZIP entries reduce + // to the same basename at restore, so keeping the last row seen would give + // one photo the other's category. + const archiveRelPath = await writeArchive('dup-canonical.zip', { + 'individual/IMG_1234.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'IMG_1234.jpg', original_filename: 'a.jpg', category_name: 'Alpha' }, + { filename: 'IMG_1234.jpg', original_filename: 'b.jpg', category_name: 'Beta' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'dup-canonical-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await categoryOf('IMG_1234.jpg')).toBe('individual'); + for (const name of ['Alpha', 'Beta']) { + expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy(); + } + }); + + it("drops a name that one row owns canonically and another claims as an alias", async () => { + // Undecidable: with original-filename archiving ON the ZIP entry under + // this name is the ALIAS owner's file, with it OFF it is the canonical + // owner's, and the manifest does not record which mode was used. The + // point of the two-pass split is that this now resolves the same way + // every run — the archive query has no ORDER BY, so it used to be a coin + // flip between dropping the name and overwriting it. + const archiveRelPath = await writeArchive('alias-vs-canonical.zip', { + 'individual/CANON.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'CANON.jpg', original_filename: 'unrelated.jpg', category_name: 'Canonical' }, + { filename: 'other_stored.jpg', original_filename: 'CANON.jpg', category_name: 'Aliased' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'alias-vs-canonical-event'); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + // Falls back to the directory rather than guessing either row. + expect(await categoryOf('CANON.jpg')).toBe('individual'); + for (const name of ['Canonical', 'Aliased']) { + expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy(); + } + }); + + it('picks the lowest id and warns when two categories share a name', async () => { + // Allowed: two event-scoped categories with the same display name and + // different slugs. .first() used to pick either, so a re-run could move + // photos between them and inherit the wrong allow_downloads. + const archiveRelPath = await writeArchive('dupe-category.zip', { + 'individual/DUPE.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'DUPE.jpg', original_filename: 'DUPE.jpg', category_name: 'Ceremony' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'dupe-category-event'); + + const [first] = await db('photo_categories').insert({ + name: 'Ceremony', slug: 'ceremony-a', is_global: 0, event_id: eventId, + }).returning('id'); + await db('photo_categories').insert({ + name: 'Ceremony', slug: 'ceremony-b', is_global: 0, event_id: eventId, + }); + const firstId = typeof first === 'object' ? first.id : first; + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + // Stable, not arbitrary: the same run twice lands on the same row. + const photo = await db('photos').where({ event_id: eventId, filename: 'DUPE.jpg' }).first(); + expect(photo.category_id).toBe(firstId); + // And no third "Ceremony" was invented. + expect((await db('photo_categories').where({ event_id: eventId, name: 'Ceremony' })).length) + .toBe(2); + }); + + it('does not invent a category for a photo row that already exists', async () => { + // archiveEvent retains photo rows, so a restore can skip every insert. + // Resolving categories before that check created one from the stale + // manifest name that nothing then used — renaming a category while its + // event was archived left the old name behind as an empty duplicate. + const archiveRelPath = await writeArchive('existing-rows.zip', { + 'individual/KEPT.jpg': PIXEL, + 'photos_manifest.json': Buffer.from(JSON.stringify([ + { filename: 'KEPT.jpg', original_filename: 'KEPT.jpg', category_name: 'OldName' }, + ]), 'utf8'), + }); + const eventId = await seedArchivedEvent(archiveRelPath, 'existing-rows-event'); + await db('photos').insert({ + event_id: eventId, filename: 'KEPT.jpg', path: 'whatever/KEPT.jpg', type: 'jpg', + uploaded_at: new Date().toISOString(), + }); + + const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({}); + expect(res.status).toBe(200); + + expect(await db('photo_categories').where({ event_id: eventId, name: 'OldName' }).first()) + .toBeFalsy(); + }); + +}); diff --git a/backend/src/routes/adminArchives.js b/backend/src/routes/adminArchives.js index fcebccfa..ebdf81c0 100644 --- a/backend/src/routes/adminArchives.js +++ b/backend/src/routes/adminArchives.js @@ -10,6 +10,7 @@ const archiver = require('archiver'); const StreamZip = require('node-stream-zip'); const { requireEventOwnership } = require('../middleware/ownership'); const { assertZipEntriesWithin } = require('../utils/safePath'); +const { sanitizeForZipEntry } = require('../utils/filenameSanitizer'); const logger = require('../utils/logger'); const { getPagination } = require('../utils/routeHelpers'); const router = express.Router(); @@ -204,15 +205,111 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re // manifest the archive process writes. Older archives have no manifest; // we fall back to filename for those. const manifestByFilename = new Map(); + // Aliases that more than one manifest row claims — see the loop below. + const ambiguousAliases = new Set(); try { const manifestRaw = await fs.readFile( path.join(eventDir, 'photos_manifest.json'), 'utf8', ); const parsed = JSON.parse(manifestRaw); if (Array.isArray(parsed)) { - for (const m of parsed) { - if (m && m.filename) manifestByFilename.set(m.filename, m); + // Two passes, and the order is the point. Canonical photos.filename + // keys are claimed first and never yielded afterwards; aliases only + // fill names no canonical row wanted. Interleaving them made the + // result depend on manifest iteration order — the query has no + // ORDER BY — and could delete a canonical key because some OTHER + // row's original_filename happened to collide with it. + const rows = parsed.filter((m) => m && m.filename); + + // photos.filename is not unique within an event: s3AutoImporter + // takes path.basename(entry.key) and dedupes by path, so two + // imported files in different subfolders both land as `IMG_1234.jpg` + // with different `path` values. At restore both ZIP entries reduce + // to the same basename, so whichever row won the key would hand the + // other photo someone else's category. Contested names are dropped + // rather than guessed. + const contestedFilenames = new Set(); + for (const m of rows) { + const held = manifestByFilename.get(m.filename); + if (held && held !== m) { + contestedFilenames.add(m.filename); + continue; + } + manifestByFilename.set(m.filename, m); } + for (const name of contestedFilenames) manifestByFilename.delete(name); + if (contestedFilenames.size) { + logger.warn( + `Photos manifest: ${contestedFilenames.size} filename(s) claimed by more than one photo; ` + + 'those fall back to the directory for their category.' + ); + } + + // Every canonical name, contested ones included — an alias must not + // claim a name that a canonical row wanted and lost, either. + const canonicalNames = new Set(rows.map((m) => m.filename)); + + for (const m of rows) { + // Also index by original_filename. When + // general_use_original_filenames_for_downloads was on at archive + // time, archiveService names each ZIP entry after the ORIGINAL + // filename, while the manifest stays keyed by the internal + // photos.filename — so a lookup by the extracted basename misses + // every entry and the restore silently loses categories on exactly + // those archives. Never overwrite a real filename key: that one is + // authoritative if both happen to collide. + // Index the name as the ZIP would have EMITTED it, not the raw + // column: archiveService runs original names through + // sanitizeForZipEntry() before writing the entry, so an original + // with a slash or a control byte lands under a different name than + // the manifest records. Index both, so either spelling resolves. + // + // Still not total: uniquifyZipNames() appends `_1` when two photos + // in one event share an original name, and that suffix cannot be + // reconstructed from the manifest. Those few fall through to the + // directory, exactly as they did before this fix — no worse, just + // not better. Closing that needs the emitted name recorded at + // archive time, which is a writer change and a new archive format. + for (const alias of [m.original_filename, sanitizeForZipEntry(m.original_filename)]) { + if (!alias) continue; + // An alias colliding with someone else's canonical name is + // genuinely undecidable, so it is dropped rather than resolved + // either way. Which photo the ZIP emitted under that name + // depends on whether original-filename archiving was on at + // archive time, and the manifest does not record that: with it + // ON the entry is the ALIAS owner's file, with it OFF it is the + // canonical owner's. Preferring either one silently mislabels + // the other half of the time. + // + // What the two-pass split buys is that this is now decided the + // same way every run — the archive query has no ORDER BY, so + // interleaving the passes previously made it a coin flip + // between dropping the name and overwriting it. + if (canonicalNames.has(alias)) { + if (manifestByFilename.get(alias) !== m) ambiguousAliases.add(alias); + continue; + } + if (manifestByFilename.has(alias)) { + // Two rows want the same alias — e.g. `individual/IMG.jpg` and + // `collages/IMG.jpg`, which archiveService treats as distinct + // paths and does not suffix, but which collapse to one basename + // here. Whichever won would give the other photo someone else's + // category. Drop the alias so both fall through to the + // directory instead: an unresolved category is recoverable, a + // confidently wrong one is not. + if (manifestByFilename.get(alias) !== m) ambiguousAliases.add(alias); + continue; + } + manifestByFilename.set(alias, m); + } + } + } + for (const alias of ambiguousAliases) manifestByFilename.delete(alias); + if (ambiguousAliases.size) { + logger.warn( + `Photos manifest: ${ambiguousAliases.size} original-filename alias(es) claimed by more than one ` + + 'photo; those fall back to the directory for their category.' + ); } logger.info(`Loaded photos manifest: ${manifestByFilename.size} entries`); } catch (e) { @@ -226,9 +323,84 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re // Get list of extracted files to update database const extractedPhotos = []; - // First, collect all category information from the ZIP structure + // Category name -> id, resolved once per name for the whole restore. const categoriesMap = new Map(); + // Find-or-create the category by name, among the ones this event can see. + const resolveCategoryId = async (categoryName) => { + if (!categoryName) return null; + if (categoriesMap.has(categoryName)) return categoriesMap.get(categoryName); + + // Globals count as existing. A photo filed under the seeded "Ceremony" + // has event_id NULL on its category row, so an event-only lookup misses + // it and creates a second "Ceremony" — and since is_global defaults to + // TRUE, that duplicate then shows up in every other event's category + // list. Same visibility rule the photo routes use: own rows or global. + // Two queries, not one with an OR: an event-scoped category and a + // global one may share a name, and a single .first() would return + // whichever the engine felt like — silently reassigning a photo to the + // global row and losing event-local settings like allow_downloads. + // The event's own row is the more specific answer, so it wins. + // + // The global arm requires event_id IS NULL, not just is_global. The + // bug fixed here left legacy rows behind on upgraded instances — + // event-owned AND is_global true, because the column defaults true — + // and matching on the flag alone would let one event's leftover row be + // adopted by another event's restore, tying photos to a category that + // vanishes with someone else's gallery. + // Two event-scoped categories CAN share a display name when their + // slugs differ, and .first() would then pick one arbitrarily — both + // manifest names collapse onto a single id and half the photos + // inherit the wrong per-category settings (allow_downloads above all). + // Resolving that properly needs a stable category identifier in the + // manifest, which is a writer change and an archive-format bump, and + // could not help any archive already written. So: surface it instead + // of fixing it blind. If this never fires in real logs, the format + // change is not worth making; if it does, this is the evidence for it. + const ownRows = await db('photo_categories') + .where({ event_id: archive.id, name: categoryName }) + .select('id'); + if (ownRows.length > 1) { + logger.warn( + `Photos manifest: category name "${categoryName}" matches ${ownRows.length} rows in event ` + + `${archive.id}; picking the lowest id. Photos from the other row(s) will inherit its settings.` + ); + } + + const existingCategory = + // Lowest id, not engine order — an arbitrary-but-stable choice beats + // a nondeterministic one, so a re-run lands the same way. + (ownRows.length + ? await db('photo_categories') + .where('id', Math.min(...ownRows.map((r) => r.id))) + .first() + : null) + || await db('photo_categories') + .where('name', categoryName) + .whereNull('event_id') + .where('is_global', formatBoolean(true)) + .first(); + + if (existingCategory) { + categoriesMap.set(categoryName, existingCategory.id); + } else { + const insertResult = await db('photo_categories').insert({ + event_id: archive.id, + name: categoryName, + slug: slugify(categoryName), + // Explicit: the column defaults to true, and a restore inventing a + // GLOBAL category would leak this event's naming into every other + // gallery. Anything created here belongs to this event alone. + is_global: formatBoolean(false), + created_at: new Date() + }).returning('id'); + + categoriesMap.set(categoryName, insertResult[0]?.id || insertResult[0]); + } + + return categoriesMap.get(categoryName); + }; + for (const entry of entries) { if (!entry.isDirectory && entry.name.match(/\.(jpg|jpeg|png|gif|webp)$/i)) { const filename = path.basename(entry.name); @@ -239,48 +411,47 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re // Check if file was extracted successfully const stats = await fs.stat(actualFilePath); - // Determine category from directory structure - let categoryId = null; - if (dirPath && dirPath !== '.') { - // Get the first level directory as category - const categoryName = dirPath.split(path.sep)[0]; - - if (!categoriesMap.has(categoryName)) { - // Check if this category exists in the database - const existingCategory = await db('photo_categories') - .where('event_id', archive.id) - .where('name', categoryName) - .first(); - - if (existingCategory) { - categoriesMap.set(categoryName, existingCategory.id); - } else { - // Create the category if it doesn't exist - const insertResult = await db('photo_categories').insert({ - event_id: archive.id, - name: categoryName, - slug: slugify(categoryName), - created_at: new Date() - }).returning('id'); - - const newCategoryId = insertResult[0]?.id || insertResult[0]; - categoriesMap.set(categoryName, newCategoryId); - } - } - - categoryId = categoriesMap.get(categoryName); - } - + const manifestEntry = manifestByFilename.get(filename); + + // The manifest is the only faithful source for the category, and + // it is authoritative INCLUDING when it says "none". A manifest + // entry with a null category_name means the photo was genuinely + // uncategorized, so falling through to the directory would + // contradict the very record being restored from. + // + // That matters because the directory is not a category. Archive + // entry names are the storage key minus `events/active/{slug}`, + // and that layout is `individual/{filename}` / `collages/…` — + // categories have never been directories there. Reading the first + // path segment on a real archive therefore invents categories + // literally named "individual" and "collages". + // + // So the fallback is confined to photos with NO manifest entry at + // all: archives written before the manifest existed, where the + // directory is the only signal left and inventing those two names + // is still better than losing every category. // Check if photo already exists in database const existingPhoto = await db('photos') .where('event_id', archive.id) .where('filename', filename) .first(); - + if (!existingPhoto) { + // Resolved HERE, not above: resolveCategoryId find-or-CREATES, + // and archiveEvent retains photo rows. Resolving before this + // check meant restoring an archive whose rows still exist + // created a category from the stale manifest name that nothing + // then used — so renaming a category while its event was + // archived left the old name behind as an empty duplicate. + let categoryId = null; + if (manifestEntry) { + categoryId = await resolveCategoryId(manifestEntry.category_name); + } else if (dirPath && dirPath !== '.') { + categoryId = await resolveCategoryId(dirPath.split(path.sep)[0]); + } + // Store relative path from storage root const relativePath = path.relative(storagePath, actualFilePath); - const manifestEntry = manifestByFilename.get(filename); extractedPhotos.push({ event_id: archive.id, filename: filename,