From 8d12cb2f2da69a627c1e3ecff45bcdcce04e1985 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:30:09 -0500 Subject: [PATCH] feat(intake): swap a photo for a cut-out, keeping the original (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds backend/src/intake/backgroundRemoval.ts, the shared module the drafting worker and the admin endpoints both call so a cut-out obtained either way is undoable the same way. cutoutPathFor is pure and writes a new file beside the original rather than overwriting it, which is what keeps the original restorable and makes the JPEG-to-PNG change free. removeImageBackground only points the row at the new file after it is already on disk, and is idempotent via the original_image_path IS NOT NULL check — load-bearing twice, since it also stops a second pass from recording the cut-out as the original and losing the real one for good. restoreImageOriginal swaps the paths back and deliberately leaves the cut-out file on disk. Extends the Task 1 integration test file with a stub sidecar bound to an ephemeral port and covers the no-op-on-repeat case plus three failure modes (500, non-image body, unreachable), asserting the row is left untouched in every failure case. Co-Authored-By: Claude Opus 5 --- backend/src/intake/backgroundRemoval.ts | 139 +++++++++++++ .../backgroundRemoval.integration.test.ts | 187 ++++++++++++++++++ backend/tests/unit/backgroundRemoval.test.ts | 22 +++ 3 files changed, 348 insertions(+) create mode 100644 backend/src/intake/backgroundRemoval.ts create mode 100644 backend/tests/unit/backgroundRemoval.test.ts diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts new file mode 100644 index 0000000..23eb313 --- /dev/null +++ b/backend/src/intake/backgroundRemoval.ts @@ -0,0 +1,139 @@ +import { promises as fs } from 'fs'; +import path from 'path'; +import { pool } from '../db'; +import { typeForExtension } from '../uploadTypes'; +import { isRembgConfigured, removeBackground } from './rembgClient'; + +/** + * Swapping a photo for a cut-out of itself, and swapping it back. + * + * One module rather than two, because the worker's path and the admin's path + * must produce identical results: a cut-out obtained either way has to be + * undoable the same way. A near-copy that drifted would mean a photo the + * Restore button could not restore. + * + * Nothing here deletes anything. The original file stays on disk and so does + * every cut-out ever made, because the submitter's photos are often the only + * copy of an item no longer in their hands — the same rule Discard follows in + * the review queue. + */ + +interface ImageRow { + image_path: string; + original_image_path: string | null; +} + +/** + * The path a cut-out of `imagePath` is written to. + * + * Pure, so the naming rule can be checked without a database or a sidecar. + * Always `.png` because the result is transparent, and the storefront's dark + * theme would show a flat white background as a bright box behind every + * product. + */ +export function cutoutPathFor(imagePath: string): string { + const base = path.basename(imagePath, path.extname(imagePath)); + return `/uploads/${base}-cutout.png`; +} + +function uploadsDir(): string { + return process.env.UPLOADS_DIR ?? ''; +} + +/** + * Replaces one image with a cut-out, keeping the original. + * + * Idempotent by way of the `original_image_path IS NOT NULL` check rather than + * a separate flag. That guard is load-bearing twice over: it makes a repeat + * call a no-op, and it stops a second pass from recording the *cut-out* as the + * original and losing the real one for good. + * + * Throws on every failure. Nothing is written to the row unless the file is + * already on disk, so a caller that catches and moves on leaves the photo + * exactly as it was. + */ +export async function removeImageBackground(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + const row = rows[0]; + if (!row) { + throw new Error(`no image ${imageId}`); + } + if (row.original_image_path !== null) { + // Already cut out. Doing it again would overwrite the record of where the + // real original went. + return; + } + + // basename only: image_path is stored as '/uploads/' and the directory + // it lives in is a server constant. Same rule readPhotos follows in the + // drafting worker. + const sourceName = path.basename(row.image_path); + const mediaType = typeForExtension(path.extname(sourceName)); + if (mediaType === null) { + throw new Error(`cannot read ${sourceName}: unrecognised extension`); + } + + const cutout = await removeBackground( + await fs.readFile(path.join(uploadsDir(), sourceName)), + mediaType + ); + + const cutoutPath = cutoutPathFor(row.image_path); + await fs.writeFile(path.join(uploadsDir(), path.basename(cutoutPath)), cutout); + + // The row is pointed at the new file only after the file exists. The other + // order would leave a window in which the storefront rendered a broken image. + // + // `original_image_path = image_path` reads the pre-update value, which is how + // Postgres evaluates an UPDATE's right-hand side — so this records where the + // photo came from in the same statement that moves it. + await pool.query( + `UPDATE item_images + SET image_path = $2, original_image_path = image_path + WHERE id = $1 AND original_image_path IS NULL`, + [imageId, cutoutPath] + ); +} + +/** + * Puts the original back. + * + * The cut-out file is left on disk deliberately. Removing a background is + * exactly the operation that produces an occasional bad result on an unusual + * object, so somebody restoring one is quite likely to try again — and this + * module deletes nothing in any case. + */ +export async function restoreImageOriginal(imageId: number): Promise { + const { rowCount } = await pool.query( + `UPDATE item_images + SET image_path = original_image_path, original_image_path = NULL + WHERE id = $1 AND original_image_path IS NOT NULL`, + [imageId] + ); + if (rowCount === 0) { + throw new Error(`image ${imageId} has no original to restore`); + } +} + +/** + * Every photo of one item, in order. + * + * Sequential rather than parallel: the sidecar is assumed to handle one + * request at a time, and the worker it runs inside is not in a hurry. A + * failure on one photo stops the rest, and the caller logs it — the item keeps + * whatever was already done, and nothing is left half-written. + */ +export async function removeBackgroundsForItem(itemId: number): Promise { + if (!isRembgConfigured()) return; + + const { rows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + for (const row of rows) { + await removeImageBackground(row.id); + } +} diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index 4233c07..0832617 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -1,5 +1,11 @@ +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; import { pool } from '../../src/db'; import { resetDb, closeDb } from './setup/testDb'; +import { removeImageBackground, restoreImageOriginal } from '../../src/intake/backgroundRemoval'; beforeEach(async () => { await resetDb(); @@ -52,3 +58,184 @@ describe('the background-removal columns', () => { expect(rows[0]?.original_image_path).toBeNull(); }); }); + +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +interface ImagePaths { + image_path: string; + original_image_path: string | null; +} + +let uploads = ''; +let stub: http.Server | null = null; + +/** + * A real uploads directory and a stub sidecar. + * + * A temporary directory rather than the configured one, because these tests + * write files and a suite that leaves rubbish in a developer's uploads volume + * is a suite people stop running. + * + * No test here contacts the real sidecar. It takes forty seconds to start, and + * a suite that depends on that is broken by construction. + */ +async function startStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'bgremoval-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + // Drain the body before answering, or the client sees a reset rather than + // the status this test is about. + req.on('data', () => undefined); + req.on('end', () => handler(req, res)); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +} + +function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); +} + +afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +async function seedWithFile(): Promise<{ itemId: number; imageId: number; name: string }> { + const name = 'original.jpg'; + const seeded = await seedSubmission(`/uploads/${name}`); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + return { ...seeded, name }; +} + +async function pathsOf(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + return rows[0]!; +} + +describe('removing one photo’s background', () => { + it('points the row at the cut-out and records where the original went', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original-cutout.png'); + expect(paths.original_image_path).toBe('/uploads/original.jpg'); + }); + + // The original is the only copy of an item that may no longer be in the + // sender's hands. Nothing in this module is allowed to remove it. + it('leaves the original file on disk', async () => { + await startStub(answerWithPng); + const { imageId, name } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.access(path.join(uploads, name))).resolves.toBeUndefined(); + }); + + it('writes the cut-out where the row now says it is', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.readFile(path.join(uploads, 'original-cutout.png'))).resolves.toEqual( + PNG_BYTES + ); + }); + + // The guard that stops a second pass recording the cut-out as the original + // and losing the real one for good. + it('is a no-op on a photo that has already been cut out', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + await removeImageBackground(imageId); + + expect((await pathsOf(imageId)).original_image_path).toBe('/uploads/original.jpg'); + }); +}); + +describe('when the sidecar will not answer', () => { + it('leaves the row untouched on a 500', async () => { + await startStub((_req, res) => { + res.writeHead(500); + res.end('boom'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/500/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it returns something that is not an image', async () => { + await startStub((_req, res) => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end('gateway error'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/not a PNG/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it is unreachable', async () => { + await startStub(answerWithPng); + // Closed before the call, so the connection is refused rather than hung. + // The URL stays set, which is the case worth modelling: configured, and + // not there. + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); +}); + +describe('putting the original back', () => { + it('swaps the paths back and clears the record', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + await removeImageBackground(imageId); + + await restoreImageOriginal(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('refuses a photo that was never cut out, rather than blanking its path', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await expect(restoreImageOriginal(imageId)).rejects.toThrow(/no original/); + + expect((await pathsOf(imageId)).image_path).toBe('/uploads/original.jpg'); + }); +}); diff --git a/backend/tests/unit/backgroundRemoval.test.ts b/backend/tests/unit/backgroundRemoval.test.ts new file mode 100644 index 0000000..23bf41b --- /dev/null +++ b/backend/tests/unit/backgroundRemoval.test.ts @@ -0,0 +1,22 @@ +import { cutoutPathFor } from '../../src/intake/backgroundRemoval'; + +describe('where a cut-out is written', () => { + // A new file rather than a rewrite of the original, which is what makes the + // original restorable at all — and what makes the JPEG-to-PNG change free, + // since no existing path is renamed. + it('sits beside the original with a -cutout suffix and a .png extension', () => { + expect(cutoutPathFor('/uploads/abc-123.jpg')).toBe('/uploads/abc-123-cutout.png'); + }); + + // image_path is a contract, not a string: #103 made the stored value the + // path uploadUrl joins an origin onto. + it('keeps the /uploads/ prefix', () => { + expect(cutoutPathFor('/uploads/x.webp')).toBe('/uploads/x-cutout.png'); + }); + + // The extension is replaced rather than appended, so a second pass cannot + // produce `.png.png`. + it('replaces the extension rather than appending to it', () => { + expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png'); + }); +});