From 75afedaef00f3439f6cb2844c12876df1bf68906 Mon Sep 17 00:00:00 2001 From: root Date: Sun, 31 May 2026 16:37:06 +1000 Subject: [PATCH] feat(api): error + validate + pagination plumbing Add lib/api/{errors,validate,pagination,index}.js: typed ApiError subclasses, errorMiddleware, zod-backed validate(), parsePagination with caps, and a mountApi() that owns /api routing + 404 + error tail. server.js delegates /api to mountApi and drops the inline /api/spaces smoke (returns in Task 2). Co-Authored-By: Claude Opus 4.7 --- lib/api/errors.js | 48 ++++++++++++++++++++++++ lib/api/index.js | 16 ++++++++ lib/api/pagination.js | 14 +++++++ lib/api/validate.js | 15 ++++++++ server.js | 9 +---- tests/api/errors.test.js | 64 ++++++++++++++++++++++++++++++++ tests/api/helpers.js | 14 +++++++ tests/api/validate.test.js | 75 ++++++++++++++++++++++++++++++++++++++ tests/server.test.js | 14 +++---- 9 files changed, 254 insertions(+), 15 deletions(-) create mode 100644 lib/api/errors.js create mode 100644 lib/api/index.js create mode 100644 lib/api/pagination.js create mode 100644 lib/api/validate.js create mode 100644 tests/api/errors.test.js create mode 100644 tests/api/helpers.js create mode 100644 tests/api/validate.test.js diff --git a/lib/api/errors.js b/lib/api/errors.js new file mode 100644 index 0000000..4aa2600 --- /dev/null +++ b/lib/api/errors.js @@ -0,0 +1,48 @@ +import { log } from '../log.js'; + +export class ApiError extends Error { + constructor(code, message, status, details) { + super(message); + this.code = code; + this.status = status; + this.details = details; + } +} + +export class NotFoundError extends ApiError { + constructor(message = 'not found', details) { + super('not_found', message, 404, details); + } +} + +export class ValidationError extends ApiError { + constructor(message = 'validation failed', details) { + super('validation_failed', message, 400, details); + } +} + +export class ConflictError extends ApiError { + constructor(message = 'conflict', details) { + super('conflict', message, 409, details); + } +} + +export class ForbiddenError extends ApiError { + constructor(message = 'forbidden', details) { + super('forbidden', message, 403, details); + } +} + +export function asyncWrap(fn) { + return (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next); +} + +export function errorMiddleware(err, _req, res, _next) { + if (err instanceof ApiError) { + const body = { error: { code: err.code, message: err.message } }; + if (err.details !== undefined) body.error.details = err.details; + return res.status(err.status).json(body); + } + log.error({ err }, 'unhandled'); + res.status(500).json({ error: { code: 'internal', message: err.message } }); +} diff --git a/lib/api/index.js b/lib/api/index.js new file mode 100644 index 0000000..828842c --- /dev/null +++ b/lib/api/index.js @@ -0,0 +1,16 @@ +import { Router } from 'express'; +import { ownerOnly } from '../auth/owner.js'; +import { errorMiddleware, NotFoundError } from './errors.js'; + +export function mountApi(app) { + const api = Router(); + api.use(ownerOnly); + + // route modules registered here as Plan 2 progresses + + api.use((_req, _res, next) => next(new NotFoundError('route not found'))); + + api.use(errorMiddleware); + app.use('/api', api); + return api; +} diff --git a/lib/api/pagination.js b/lib/api/pagination.js new file mode 100644 index 0000000..6302dc1 --- /dev/null +++ b/lib/api/pagination.js @@ -0,0 +1,14 @@ +import { ValidationError } from './errors.js'; + +export function parsePagination(req, { defaultLimit = 50, max = 200 } = {}) { + const q = req.query || {}; + const limit = q.limit === undefined ? defaultLimit : Number(q.limit); + const offset = q.offset === undefined ? 0 : Number(q.offset); + if (!Number.isFinite(limit) || limit < 1 || limit > max) { + throw new ValidationError(`limit must be 1..${max}`, { limit: q.limit }); + } + if (!Number.isFinite(offset) || offset < 0) { + throw new ValidationError('offset must be >= 0', { offset: q.offset }); + } + return { limit, offset }; +} diff --git a/lib/api/validate.js b/lib/api/validate.js new file mode 100644 index 0000000..bf81445 --- /dev/null +++ b/lib/api/validate.js @@ -0,0 +1,15 @@ +import { ValidationError } from './errors.js'; + +export function validate({ body, params, query } = {}) { + return (req, _res, next) => { + try { + if (body) req.body = body.parse(req.body); + if (params) req.params = params.parse(req.params); + if (query) req.validatedQuery = query.parse(req.query); + next(); + } catch (e) { + if (e?.issues) return next(new ValidationError('validation failed', e.issues)); + next(e); + } + }; +} diff --git a/server.js b/server.js index 66cb371..14cc9e4 100644 --- a/server.js +++ b/server.js @@ -1,9 +1,8 @@ import 'dotenv/config'; import express from 'express'; import { pool } from './lib/db/pool.js'; -import { ownerOnly } from './lib/auth/owner.js'; import { log } from './lib/log.js'; -import * as spaces from './lib/db/repos/spaces.js'; +import { mountApi } from './lib/api/index.js'; const VERSION = '2.0.0-alpha.1'; @@ -22,11 +21,7 @@ export function createApp() { res.json({ ok: true, db_ok, version: VERSION }); }); - app.use('/api', ownerOnly); - - app.get('/api/spaces', async (_req, res) => { - res.json(await spaces.list()); - }); + mountApi(app); app.use((_req, res) => res.status(404).json({ error: { code: 'not_found' } })); diff --git a/tests/api/errors.test.js b/tests/api/errors.test.js new file mode 100644 index 0000000..f166fcc --- /dev/null +++ b/tests/api/errors.test.js @@ -0,0 +1,64 @@ +import { describe, it, expect } from 'vitest'; +import express from 'express'; +import request from 'supertest'; +import { + errorMiddleware, + NotFoundError, ValidationError, ConflictError, ForbiddenError, + asyncWrap, ApiError +} from '../../lib/api/errors.js'; + +function buildApp(handler) { + const app = express(); + app.use(express.json()); + app.get('/test', asyncWrap(handler)); + app.use(errorMiddleware); + return app; +} + +describe('api/errors', () => { + it('NotFoundError → 404 with code "not_found"', async () => { + const res = await request(buildApp(() => { throw new NotFoundError('missing'); })) + .get('/test'); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: { code: 'not_found', message: 'missing' } }); + }); + + it('ValidationError → 400 with details', async () => { + const res = await request(buildApp(() => { throw new ValidationError('bad', [{ path: 'name' }]); })) + .get('/test'); + expect(res.status).toBe(400); + expect(res.body.error.code).toBe('validation_failed'); + expect(res.body.error.details).toEqual([{ path: 'name' }]); + }); + + it('ConflictError → 409', async () => { + const res = await request(buildApp(() => { throw new ConflictError(); })).get('/test'); + expect(res.status).toBe(409); + expect(res.body.error.code).toBe('conflict'); + }); + + it('ForbiddenError → 403', async () => { + const res = await request(buildApp(() => { throw new ForbiddenError(); })).get('/test'); + expect(res.status).toBe(403); + expect(res.body.error.code).toBe('forbidden'); + }); + + it('unknown error → 500 with generic shape', async () => { + const res = await request(buildApp(() => { throw new Error('boom'); })).get('/test'); + expect(res.status).toBe(500); + expect(res.body.error.code).toBe('internal'); + }); + + it('asyncWrap forwards rejected promises', async () => { + const res = await request(buildApp(async () => { throw new NotFoundError(); })) + .get('/test'); + expect(res.status).toBe(404); + }); + + it('ApiError exposes code, status, and details', () => { + const e = new ApiError('x', 'y', 418, { a: 1 }); + expect(e.code).toBe('x'); + expect(e.status).toBe(418); + expect(e.details).toEqual({ a: 1 }); + }); +}); diff --git a/tests/api/helpers.js b/tests/api/helpers.js new file mode 100644 index 0000000..0448297 --- /dev/null +++ b/tests/api/helpers.js @@ -0,0 +1,14 @@ +import { createApp } from '../../server.js'; +import { resetDb } from '../helpers/db.js'; +import { migrateUp } from '../../lib/db/migrate.js'; + +const OWNER_TOKEN = 'test-token'; + +export async function setup() { + await resetDb(); + await migrateUp(); + process.env.OWNER_TOKEN = OWNER_TOKEN; + const app = createApp(); + const ownerHeaders = { Authorization: `Bearer ${OWNER_TOKEN}` }; + return { app, ownerHeaders, OWNER_TOKEN }; +} diff --git a/tests/api/validate.test.js b/tests/api/validate.test.js new file mode 100644 index 0000000..e006bc5 --- /dev/null +++ b/tests/api/validate.test.js @@ -0,0 +1,75 @@ +import { describe, it, expect } from 'vitest'; +import express from 'express'; +import request from 'supertest'; +import { z } from 'zod'; +import { validate } from '../../lib/api/validate.js'; +import { errorMiddleware } from '../../lib/api/errors.js'; +import { parsePagination } from '../../lib/api/pagination.js'; + +function buildApp() { + const app = express(); + app.use(express.json()); + + app.post('/body', + validate({ body: z.object({ name: z.string().min(1) }) }), + (req, res) => res.json(req.body) + ); + + app.get('/params/:id', + validate({ params: z.object({ id: z.string().uuid() }) }), + (req, res) => res.json(req.params) + ); + + app.get('/page', (req, res) => { + try { + const p = parsePagination(req); + res.json(p); + } catch (e) { errorMiddleware(e, req, res, () => {}); } + }); + + app.use(errorMiddleware); + return app; +} + +describe('api/validate', () => { + it('body schema parses and replaces req.body', async () => { + const res = await request(buildApp()).post('/body').send({ name: 'home' }); + expect(res.status).toBe(200); + expect(res.body).toEqual({ name: 'home' }); + }); + + it('body schema failure → 400 with zod issues in details', async () => { + const res = await request(buildApp()).post('/body').send({ name: '' }); + expect(res.status).toBe(400); + expect(res.body.error.code).toBe('validation_failed'); + expect(Array.isArray(res.body.error.details)).toBe(true); + expect(res.body.error.details.length).toBeGreaterThan(0); + }); + + it('params schema failure → 400', async () => { + const res = await request(buildApp()).get('/params/not-a-uuid'); + expect(res.status).toBe(400); + }); +}); + +describe('api/pagination', () => { + it('defaults to limit=50 offset=0', async () => { + const res = await request(buildApp()).get('/page'); + expect(res.body).toEqual({ limit: 50, offset: 0 }); + }); + + it('honors ?limit&offset', async () => { + const res = await request(buildApp()).get('/page?limit=10&offset=5'); + expect(res.body).toEqual({ limit: 10, offset: 5 }); + }); + + it('rejects negative offset', async () => { + const res = await request(buildApp()).get('/page?offset=-1'); + expect(res.status).toBe(400); + }); + + it('rejects limit above max', async () => { + const res = await request(buildApp()).get('/page?limit=999'); + expect(res.status).toBe(400); + }); +}); diff --git a/tests/server.test.js b/tests/server.test.js index 649c724..f293718 100644 --- a/tests/server.test.js +++ b/tests/server.test.js @@ -25,16 +25,14 @@ describe('server', () => { expect(res.status).toBe(401); }); - it('GET /api/spaces with token returns 200 and empty array', async () => { - const res = await request(app) - .get('/api/spaces') - .set('Authorization', 'Bearer test-token'); - expect(res.status).toBe(200); - expect(res.body).toEqual([]); + it('unknown /api route returns 404 with structured error', async () => { + const res = await request(app).get('/api/nope').set('Authorization', 'Bearer test-token'); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: { code: 'not_found', message: 'route not found' } }); }); - it('unknown route returns 404', async () => { - const res = await request(app).get('/api/nope').set('Authorization', 'Bearer test-token'); + it('unknown non-api route returns 404', async () => { + const res = await request(app).get('/missing'); expect(res.status).toBe(404); }); });