bermudalambandClaude Opus 5
f52976dec9
Three routes in admin.ts answered a miss with a success. PUT /items/:id ran an UPDATE that matched nothing, committed happily, selected nothing back and replied 200 with an empty body — a success the admin client could do nothing with, and no record anywhere that the item was not found. mark-sold and mark-available did the same. The create route beside them has always used requireRow for exactly this, which is why this reads as an oversight rather than a decision.
A garbage id was worse in a different direction. Number('abc') is NaN, the driver sends it to Postgres as the text "NaN", Postgres raises 22P02 for an integer column, and the catch turned that into a 500 — so a caller asking for an item that cannot exist was told the server broke. Both now answer 404, because from the caller's side "/items/abc" identifies no item in exactly the way "/items/999999" does.
readId is shared rather than repeated, and rejects zero, negatives and fractions as well as text: every id in this schema is a positive serial, so anything else identifies nothing.
mark-sold now notifies favouriters only after the row is known to exist, so nobody is told about a sale that did not happen.
The issue asked for the same shape to be checked across the other admin routes. It was: unpublish already looks the item up and 404s, and the tags and categories PUT routes both do an existence check before their UPDATE, so their rows[0] is guaranteed. items.ts already guards the public read. These three were the only ones lying about a miss.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
111 lines
3.4 KiB
TypeScript
111 lines
3.4 KiB
TypeScript
import request from 'supertest';
|
|
import app from '../../src/app';
|
|
import { pool } from '../../src/db';
|
|
import { resetDb, closeDb } from './setup/testDb';
|
|
|
|
beforeEach(async () => {
|
|
await resetDb();
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await pool.end();
|
|
await closeDb();
|
|
});
|
|
|
|
const PNG = Buffer.from(
|
|
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
|
|
'base64'
|
|
);
|
|
|
|
/**
|
|
* Nothing exists, so every id below is absent. 999999 is well-formed and
|
|
* missing; 'abc' is not a number at all. Before #207 the first answered 200
|
|
* with an empty body and the second answered 500, because the raw string
|
|
* reached Postgres and raised 22P02.
|
|
*/
|
|
const ABSENT = 999999;
|
|
|
|
describe('admin item routes for an id that does not exist', () => {
|
|
it('PUT answers 404 rather than 200 with an empty body', async () => {
|
|
const res = await request(app)
|
|
.put(`/api/admin/items/${ABSENT}`)
|
|
.field('name', 'renamed')
|
|
.field('price', '10.00');
|
|
|
|
expect(res.status).toBe(404);
|
|
expect(res.body).toEqual({ error: 'not found' });
|
|
});
|
|
|
|
it('mark-sold answers 404', async () => {
|
|
const res = await request(app).post(`/api/admin/items/${ABSENT}/mark-sold`);
|
|
expect(res.status).toBe(404);
|
|
});
|
|
|
|
it('mark-available answers 404', async () => {
|
|
const res = await request(app).post(`/api/admin/items/${ABSENT}/mark-available`);
|
|
expect(res.status).toBe(404);
|
|
});
|
|
|
|
// A garbage id used to reach Postgres and raise 22P02, which the catch turned
|
|
// into a 500. From the caller's side "/items/abc" identifies no item, exactly
|
|
// like "/items/999999" does.
|
|
it.each(['abc', '1.5', '-1', ''])('PUT answers 404 for the id %p', async (id) => {
|
|
const res = await request(app)
|
|
.put(`/api/admin/items/${id}`)
|
|
.field('name', 'renamed')
|
|
.field('price', '10.00');
|
|
|
|
expect(res.status).toBe(404);
|
|
});
|
|
|
|
it('mark-available answers 404 for a non-numeric id', async () => {
|
|
const res = await request(app).post('/api/admin/items/abc/mark-available');
|
|
expect(res.status).toBe(404);
|
|
});
|
|
});
|
|
|
|
describe('admin item routes for an id that does exist', () => {
|
|
async function makeItem(): Promise<number> {
|
|
const res = await request(app)
|
|
.post('/api/admin/items')
|
|
.field('name', 'a real item')
|
|
.field('price', '12.00')
|
|
.attach('images', PNG, 'a.png');
|
|
return res.body.id;
|
|
}
|
|
|
|
// The point of the change is to stop lying about misses, not to start
|
|
// refusing hits.
|
|
it('PUT still updates and answers with the item', async () => {
|
|
const id = await makeItem();
|
|
|
|
const res = await request(app)
|
|
.put(`/api/admin/items/${id}`)
|
|
.field('name', 'renamed')
|
|
.field('price', '15.00');
|
|
|
|
expect(res.status).toBe(200);
|
|
expect(res.body.name).toBe('renamed');
|
|
expect(res.body.price_cents).toBe(1500);
|
|
});
|
|
|
|
it('mark-available still publishes', async () => {
|
|
const id = await makeItem();
|
|
|
|
const res = await request(app).post(`/api/admin/items/${id}/mark-available`);
|
|
|
|
expect(res.status).toBe(200);
|
|
expect(res.body.status).toBe('available');
|
|
});
|
|
});
|
|
|
|
describe('a non-numeric id on every admin item route', () => {
|
|
// Each of these used to reach Postgres, raise 22P02 and surface as a 500.
|
|
it.each([
|
|
['mark-sold', '/api/admin/items/abc/mark-sold'],
|
|
['mark-available', '/api/admin/items/abc/mark-available']
|
|
])('%s answers 404', async (_name, path) => {
|
|
expect((await request(app).post(path)).status).toBe(404);
|
|
});
|
|
});
|