diff --git a/apps/clubs/src/clubs-db.ts b/apps/clubs/src/clubs-db.ts index d18c05c..dddddb5 100644 --- a/apps/clubs/src/clubs-db.ts +++ b/apps/clubs/src/clubs-db.ts @@ -14,7 +14,7 @@ * can build the tables directly. */ -import { getSavedImagesByNames, placeholderSavedImage } from '@repo/domain' +import { getSavedImagesByIds } from '@repo/domain' import type { SavedImage } from '@repo/domain' @@ -130,11 +130,12 @@ interface StoredClub extends Club { CreatedAt: string CustomTags?: string[] /** - * The club's gallery images, by slot (the client PUTs to - * `/additionalimage/{index}`). Positional, so a cleared middle slot stays as an - * empty string rather than shifting the images after it. + * The club's gallery, by slot (the client PUTs to `/additionalimage/{index}`). + * Entries are `SavedImage` ids — image names are R2 keys and change when an image + * is re-uploaded, so the id is what stays true — with `0` for an empty slot. + * Positional, so clearing a middle slot doesn't shift the images after it. */ - AdditionalImages?: string[] + AdditionalImages?: number[] } /** How many gallery images a club has room for (slots 0..2). */ @@ -674,44 +675,48 @@ function dedupeTags(tags: string[]): string[] { } /** - * A club's gallery images (stored on the blob). Trailing empty slots are trimmed, so - * a club with nothing set reads as `[]` while a club with only slot 1 filled still - * reports `['', 'name.jpg']` — the index a client PUT to is the index it reads back. + * A club's gallery slots as stored (image ids, `0` for empty). Trailing empty slots + * are trimmed, so a club with nothing set reads as `[]` while a club with only slot 1 + * filled still reports `[0, 42]` — the index a client PUT to is the index it reads + * back. */ -export async function getClubAdditionalImages(db: D1Database, clubId: number): Promise { +export async function getClubAdditionalImages(db: D1Database, clubId: number): Promise { const row = await db .prepare('SELECT data FROM club WHERE club_id = ?1') .bind(clubId) .first() const images = row === null ? [] : ((JSON.parse(row.data) as StoredClub).AdditionalImages ?? []) let end = images.length - while (end > 0 && images[end - 1] === '') end-- + while (end > 0 && images[end - 1] === 0) end-- return images.slice(0, end) } /** * A club's gallery as the client reads it: the image record behind each filled slot, - * in slot order. Empty slots are left out (the records carry no index, so a hole - * would just be a blank image), and a name whose metadata row is missing falls back - * to a placeholder record so the picture still renders. + * in slot order. Empty slots are left out — the records carry no index, so a hole + * would just be a blank image — and so are ids whose image has since been deleted, + * which is the point of storing ids: the gallery follows the image rather than a + * filename that may now belong to nothing. */ export async function getClubGallery(db: D1Database, clubId: number): Promise { - const names = (await getClubAdditionalImages(db, clubId)).filter((n) => n !== '') - if (names.length === 0) return [] - const records = await getSavedImagesByNames(db, names) - return names.map((name) => records.get(name) ?? placeholderSavedImage(name)) + const ids = (await getClubAdditionalImages(db, clubId)).filter((id) => id !== 0) + if (ids.length === 0) return [] + const byId = await getSavedImagesByIds(db, ids) + return ids + .map((id) => byId.get(id)) + .filter((image): image is SavedImage => image !== undefined) } /** - * Set (or clear, with an empty `imageName`) one of a club's gallery image slots. - * Returns null when the club doesn't exist. The slot must be in range — callers - * validate the index before getting here. + * Set (or clear, with a null `imageId`) one of a club's gallery image slots. Returns + * null when the club doesn't exist. The slot must be in range, and the image must + * exist — callers validate both before getting here. */ export async function setClubAdditionalImage( db: D1Database, clubId: number, index: number, - imageName: string + imageId: number | null ): Promise { const row = await db .prepare('SELECT data FROM club WHERE club_id = ?1') @@ -721,10 +726,10 @@ export async function setClubAdditionalImage( const stored = JSON.parse(row.data) as StoredClub // Pad rather than assign past the end: a sparse array would serialize its holes as - // nulls, and the client expects strings in every slot it reads. + // nulls, and every slot the client reads should be an id (0 when empty). const images = [...(stored.AdditionalImages ?? [])] - while (images.length <= index) images.push('') - images[index] = imageName + while (images.length <= index) images.push(0) + images[index] = imageId ?? 0 const updated: StoredClub = { ...stored, AdditionalImages: images } await db diff --git a/apps/clubs/src/clubs.app.ts b/apps/clubs/src/clubs.app.ts index ac0a802..8a37bc9 100644 --- a/apps/clubs/src/clubs.app.ts +++ b/apps/clubs/src/clubs.app.ts @@ -1,6 +1,7 @@ import { Hono } from 'hono' import { useWorkersLogger } from 'workers-tagged-logger' +import { getSavedImageByName } from '@repo/domain' import { withNotFound, withOnError } from '@repo/hono-helpers' import { validateAndGetAccountId } from '@repo/jwt' @@ -576,9 +577,13 @@ const app = new Hono() // One of the club's gallery images, by slot (`/additionalimage/{index}`, 0-based — // the client PUTs the first image to 0, the second to 1). Takes the same - // `imageName` the `storage` worker handed back; an empty one clears that slot. - // Co-owner or above, like the main image. The slots are positional, so clearing - // one doesn't shift the others; they come back on `value.AdditionalImages`. + // `imageName` the `storage` worker handed back (or an `imageId` directly); an empty + // one clears that slot. Co-owner or above, like the main image. The slots are + // positional, so clearing one doesn't shift the others; they come back on + // `value.AdditionalImages` as whole image records. + // + // The name is resolved to the image's id and *that* is what's stored: names are R2 + // keys, so a re-upload changes them, while the id keeps pointing at the image. .put('/club/:clubId{[0-9]+}/additionalimage/:index{[0-9]+}', async (c) => { const id = await authedId(c) if (id === null) return c.body(null, 401) @@ -598,10 +603,26 @@ const app = new Hono() } const body = (await c.req.parseBody().catch(() => ({}))) as Record - const key = Object.keys(body).find((k) => k.toLowerCase() === 'imagename') - const imageName = typeof body[key ?? ''] === 'string' ? (body[key ?? ''] as string).trim() : '' + const field = (name: string): string => { + const key = Object.keys(body).find((k) => k.toLowerCase() === name) + return typeof body[key ?? ''] === 'string' ? (body[key ?? ''] as string).trim() : '' + } + const rawId = field('imageid') + const imageName = field('imagename') - const updated = await setClubAdditionalImage(c.env.DB, clubId, index, imageName) + // Neither field (or an empty one) clears the slot; otherwise resolve to an id and + // refuse an image we don't know, rather than storing a dangling reference. + let imageId: number | null = null + if (rawId !== '') { + imageId = Number.parseInt(rawId, 10) + if (Number.isNaN(imageId)) return clubError(c, 'Invalid imageId.') + } else if (imageName !== '') { + const image = await getSavedImageByName(c.env.DB, imageName) + if (image === null) return clubError(c, 'No such image.') + imageId = image.Id + } + + const updated = await setClubAdditionalImage(c.env.DB, clubId, index, imageId) if (updated === null) return c.notFound() return c.json({ error: '', diff --git a/apps/clubs/src/test/integration/api.test.ts b/apps/clubs/src/test/integration/api.test.ts index 87e39a7..3059071 100644 --- a/apps/clubs/src/test/integration/api.test.ts +++ b/apps/clubs/src/test/integration/api.test.ts @@ -585,7 +585,7 @@ describe('clubs endpoints', () => { const first = 'sharecamera/2026-07-21/e37fc41f-005e-4216-8f1e-a37dca953981.jpg' const insertImage = env.DB.prepare('INSERT OR IGNORE INTO image (data) VALUES (?1)') await env.DB.batch( - [first, 'b.jpg', 'c.jpg'].map((ImageName, i) => + [first, 'b.jpg', 'c.jpg', 'legacy.jpg'].map((ImageName, i) => insertImage.bind( JSON.stringify({ Id: 500 + i, @@ -641,23 +641,63 @@ describe('clubs endpoints', () => { const second = (await (await setImage(1, 'b.jpg')).json()) as Details expect(names(second.value.AdditionalImages)).toEqual([first, 'b.jpg', 'c.jpg']) - // Re-PUTting a slot replaces just that image; an empty name clears it. A name with - // no image row still renders, as a placeholder record. - const replaced = (await (await setImage(0, 'a2.jpg')).json()) as Details - expect(names(replaced.value.AdditionalImages)).toEqual(['a2.jpg', 'b.jpg', 'c.jpg']) - expect(replaced.value.AdditionalImages[0]).toMatchObject({ Id: 0, ImageName: 'a2.jpg' }) + // What's stored is the image's id, not its name: renaming the image (a re-upload + // gets a new R2 key) leaves the gallery pointing at the same picture. + await env.DB.prepare("UPDATE image SET data = json_set(data, '$.ImageName', ?2) WHERE id = ?1") + .bind(501, 'b-renamed.jpg') + .run() + const renamed = (await ( + await exports.default.fetch(`${ORIGIN}/club/${clubId}/details`) + ).json()) as { AdditionalImages: Image[] } + expect(names(renamed.AdditionalImages)).toEqual([first, 'b-renamed.jpg', 'c.jpg']) + + // Re-PUTting a slot replaces just that image; an empty name clears it. + const replaced = (await (await setImage(0, 'c.jpg')).json()) as Details + expect(names(replaced.value.AdditionalImages)).toEqual(['c.jpg', 'b-renamed.jpg', 'c.jpg']) const cleared = (await (await setImage(1, '')).json()) as Details - expect(names(cleared.value.AdditionalImages)).toEqual(['a2.jpg', 'c.jpg']) + expect(names(cleared.value.AdditionalImages)).toEqual(['c.jpg', 'c.jpg']) + + // An image name nothing was uploaded under is refused rather than stored as a + // dangling reference, and a deleted image drops out of the gallery. + expect((await setImage(1, 'never-uploaded.jpg')).status).toBe(400) + await env.DB.prepare('DELETE FROM image WHERE id = ?1').bind(502).run() + const afterDelete = (await ( + await exports.default.fetch(`${ORIGIN}/club/${clubId}/details`) + ).json()) as { AdditionalImages: Image[] } + expect(afterDelete.AdditionalImages).toEqual([]) + // Put one back so the rest of the test has a gallery to read. + await setImage(0, first) // They're on the club's details payload, for everyone reading the club. const details = (await ( await exports.default.fetch(`${ORIGIN}/club/${clubId}/details`) ).json()) as { AdditionalImages: Image[] } - expect(names(details.AdditionalImages)).toEqual(['a2.jpg', 'c.jpg']) + expect(names(details.AdditionalImages)).toEqual([first]) + + // A club stored before ids (its slots hold image names) still resolves, and the + // next PUT to that slot rewrites it as an id. + await env.DB.prepare( + "UPDATE club SET data = json_set(data, '$.AdditionalImages', json(?2)) WHERE club_id = ?1" + ) + .bind(clubId, JSON.stringify(['legacy.jpg'])) + .run() + const legacy = (await ( + await exports.default.fetch(`${ORIGIN}/club/${clubId}/details`) + ).json()) as { AdditionalImages: Image[] } + expect(legacy.AdditionalImages).toEqual([ + expect.objectContaining({ Id: 503, ImageName: 'legacy.jpg' }), + ]) + await setImage(0, first) + const row = await env.DB.prepare('SELECT data FROM club WHERE club_id = ?1') + .bind(clubId) + .first<{ data: string }>() + expect((JSON.parse(row!.data) as { AdditionalImages: unknown[] }).AdditionalImages).toEqual([ + 500, + ]) // There are only three slots, and only co-owners may set them. - expect((await setImage(3, 'd.jpg')).status).toBe(400) - expect((await setImage(0, 'hijack.jpg', '7101')).status).toBe(403) + expect((await setImage(3, first)).status).toBe(400) + expect((await setImage(0, first, '7101')).status).toBe(403) expect( ( await exports.default.fetch(`${ORIGIN}/club/${clubId}/additionalimage/0`, { diff --git a/packages/domain/src/images-db.ts b/packages/domain/src/images-db.ts index 4adbc83..6e884eb 100644 --- a/packages/domain/src/images-db.ts +++ b/packages/domain/src/images-db.ts @@ -30,45 +30,37 @@ const placeholders = (n: number): string => Array.from({ length: n }, (_, i) => `?${i + 1}`).join(',') /** - * Look up image records by name (the R2 key), returned keyed by ImageName. One query - * for the whole set; names with no record are simply absent from the map. + * Look up image records by id, returned keyed by Id. Ids with no record (the image + * was deleted since) are simply absent from the map. */ -export async function getSavedImagesByNames( +export async function getSavedImagesByIds( db: D1Database, - names: string[] -): Promise> { - if (names.length === 0) return new Map() + ids: number[] +): Promise> { + if (ids.length === 0) return new Map() const { results } = await db - .prepare(`SELECT data FROM image WHERE image_name IN (${placeholders(names.length)})`) - .bind(...names) + .prepare(`SELECT data FROM image WHERE id IN (${placeholders(ids.length)})`) + .bind(...ids) .all<{ data: string }>() return new Map( results.map((r) => { const image = JSON.parse(r.data) as SavedImage - return [image.ImageName, image] + return [image.Id, image] }) ) } /** - * A minimal SavedImage for an image name with no metadata row — enough for the client - * to render the picture. Uploads normally write a row first, so this only covers a - * name that was set directly (or whose row was since deleted). + * Look up a single image record by name (the R2 key), or null. Only for turning a + * name a client sent into the image's id — store the id, never the name. */ -export function placeholderSavedImage(imageName: string): SavedImage { - return { - Id: 0, - Type: 1, - Accessibility: 1, - AccessibilityLocked: false, - ImageName: imageName, - Description: null, - PlayerId: 0, - TaggedPlayerIds: [], - RoomId: null, - PlayerEventId: null, - CreatedAt: new Date(0).toISOString(), - CheerCount: 0, - CommentCount: 0, - } +export async function getSavedImageByName( + db: D1Database, + name: string +): Promise { + const row = await db + .prepare('SELECT data FROM image WHERE image_name = ?1') + .bind(name) + .first<{ data: string }>() + return row ? (JSON.parse(row.data) as SavedImage) : null }