create-a-container: MVC manifesto + phases 0-1 (test scaffolding, apikeys pilot migration)#405
Open
runleveldev wants to merge 9 commits into
Open
create-a-container: MVC manifesto + phases 0-1 (test scaffolding, apikeys pilot migration)#405runleveldev wants to merge 9 commits into
runleveldev wants to merge 9 commits into
Conversation
Defines the target architecture — resource-first folders (resources/<name>/ containing router, controller, service, repository, serializer, validator) — a complete new-route template built on the existing middlewares/api.js contract, and a strangler-fig migration playbook ordered from the apikeys pilot through the containers.js god routes. Normative for all new endpoints on this branch; the wire contract (response envelope, status codes, paths) is frozen throughout the migration.
Per docs/mvc-manifesto.md phase 0: - add zod (runtime) and jest + supertest (dev); npm test script - middlewares/validate.js: shared zod validation middleware producing ApiError(400, invalid_request) with per-field messages and parsed values on req.validated - jest.config.js + tests/setup-env.js: sqlite test database in tmpdir, env pinned before config/config.js loads - tests/helpers/db.js: sequelize.sync-based reset, baseline groups, user and API-key factories - tests/helpers/app.js: listen-less express harness mounting /api/v1 with in-memory sessions for supertest - unit tests for validate middleware (8 passing) Manifesto snippet updated to reference the real middleware instead of inlining it.
Manifesto §6 step 1 for the apikeys pilot: supertest integration tests against the legacy router covering auth (401s), list scoping/ordering/ shape, get (200/404 cross-user), create (201, one-time plaintext key, null description), and delete (204, cross-user 404). Must pass unchanged after the resource moves to resources/apikeys/.
Manifesto §6 steps 2-5 for the pilot resource. routers/api/v1/apikeys.js
(89 lines mixing auth lookup, queries, serialization, and HTTP) becomes
the six-file vertical slice:
- resources/apikeys/router.js wiring only (apiAuth, validate, ctrl)
- resources/apikeys/controller.js req/res translation, no model imports
- resources/apikeys/service.js owner resolution + not-found policy
- resources/apikeys/repository.js Sequelize queries (incl. the session-
uid User lookup, to move behind the
users service once that resource exists)
- resources/apikeys/serializer.js single serializer, no keyHash leakage
- resources/apikeys/validator.js zod schemas for body + :id param
Mount in routers/api/v1/index.js now points at the resource router.
Pinned integration tests pass unchanged (wire contract intact); adds
service unit tests with a mocked repository. Only deliberate behavior
delta: malformed (non-UUID) :id now 400 invalid_request instead of a
dialect-dependent 404/500, and description >255 chars now 400 instead
of a DB error.
…rtup
server.js previously built the express app inside main() and called
listen(), making the real app unreachable from tests — tests/helpers/
app.js had to rebuild a partial copy of the middleware stack, which
could silently drift from production.
- app.js: new buildApp({ sessionSecrets, rateLimit, accessLog })
containing the entire HTTP-visible stack (verbatim from server.js);
also unref()s the session store's expired-session sweeper so the
interval never keeps the process alive by itself (fixes jest hanging
on open handles)
- server.js: now only dotenv + DB-backed session secrets + listen()
- tests/helpers/app.js: consumes the real buildApp with a fixed secret,
rate limiting off (4xx assertions must not burn the limiter budget),
and access log off
Suite (23 tests) passes against the real app; boot smoke-tested:
/api/v1/health 200, unauthenticated /api/v1/apikeys 401.
Root Makefile gains a test target delegated to every component via the existing uniform loop: - create-a-container: installs devDependencies with the same lockfile-safe `npm install --no-package-lock` pattern as `dev`, then runs the jest suite - pull-config: no-op (plain bash, nothing to test) - mie-opensource-landing: no-op (the docs site is validated by build) `make test` from the repo root runs all three; only create-a-container currently executes anything (23 tests).
…ma race - jest.config.js: verbose: true so make test / npm test list every individual test, not just suite results - resetDb() must complete before buildApp(): the session store fires an unawaited CREATE TABLE for Sessions at construction, which raced resetDb's force-sync schema rebuild and could fail the API suite intermittently (seen once after a cold npm install). Order swapped in the apikeys suite and documented in the test helper.
…CSRF coverage
Two alerts fired on app.js (code newly moved from server.js):
- js/clear-text-cookie: replace the per-request cookie function with the
object form using secure: 'auto' — express-session derives the flag
from issecure(req, trustProxy), identical semantics to the previous
secure: req.secure, but statically analyzable
- js/missing-token-validation: mount csrfGuard app-wide after the rate
limiter. Behavior-preserving: the guard skips GET/HEAD/OPTIONS and
Bearer-only requests, and all state-changing routes live under
/api/v1 where the v1 router already applied it; this extends coverage
to the GET-only surfaces (templates, swagger, SPA, static) and
rejects mutations to unknown paths early (403 instead of 404)
Because the app-level guard runs before the v1 router, its failures
would have hit Express's default HTML error page; jsonErrorHandler is
now also mounted as the app's final error handler so CSRF failures keep
the frozen {"error":{"code":"csrf_invalid"}} envelope (verified by
live smoke test) and non-API errors render as JSON instead of stack
traces.
23/23 tests green.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lays down the first slice of the create-a-container MVC refactor by introducing a resource-first architecture contract (“MVC manifesto”), adding Jest/Supertest scaffolding, splitting app construction from server startup, and migrating the /api/v1/apikeys routes into a vertical resources/apikeys/ slice while keeping the API envelope stable.
Changes:
- Adds a normative MVC/resource layering guide and introduces shared zod-based request validation middleware.
- Adds server-side test scaffolding (Jest + Supertest) with SQLite-backed integration tests to pin
/api/v1/apikeysbehavior plus service unit tests. - Refactors startup by splitting
app.js(Express app construction) fromserver.js(migrations + secrets + listen) and migrates apikeys routes intoresources/apikeys/.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds repo-wide make test delegation across components. |
| pull-config/Makefile | Adds a no-op test target for uniform repo-wide test looping. |
| mie-opensource-landing/Makefile | Adds a no-op test target (docs validated by build) for uniform repo-wide test looping. |
| create-a-container/Makefile | Adds test target (jest) for the Manager component. |
| create-a-container/package.json | Adds npm test (jest) and test deps (jest, supertest, zod). |
| create-a-container/jest.config.js | Adds Jest config (node env, setupFiles, serialized workers). |
| create-a-container/tests/setup-env.js | Pins test env vars and per-worker sqlite storage location. |
| create-a-container/tests/helpers/db.js | Adds sqlite lifecycle helpers + factories for integration tests. |
| create-a-container/tests/helpers/app.js | Builds the real app stack for tests with test-friendly options and Bearer helper. |
| create-a-container/middlewares/validate.js | Adds shared zod validation → ApiError(400 invalid_request) mapping. |
| create-a-container/middlewares/tests/validate.test.js | Unit tests for validate middleware behavior. |
| create-a-container/docs/mvc-manifesto.md | Adds the normative architecture + migration playbook documentation. |
| create-a-container/app.js | New buildApp() that builds the full middleware/router stack and adds app-level CSRF + json error handling. |
| create-a-container/server.js | Simplifies startup: run migrations → fetch secrets → buildApp() → listen. |
| create-a-container/routers/api/v1/index.js | Switches /apikeys mount to the new resources/apikeys/router. |
| create-a-container/routers/api/v1/apikeys.js | Deletes legacy apikeys “god route” implementation. |
| create-a-container/resources/apikeys/router.js | New apikeys router (wiring-only) using shared validate middleware. |
| create-a-container/resources/apikeys/controller.js | New controller translating req/session to service calls + response helpers. |
| create-a-container/resources/apikeys/service.js | New service for apikey operations (ownership checks + create/delete). |
| create-a-container/resources/apikeys/repository.js | New repository encapsulating Sequelize queries. |
| create-a-container/resources/apikeys/serializer.js | New serializer enforcing non-disclosure of hashes/plaintext key. |
| create-a-container/resources/apikeys/validator.js | New zod schemas for body + :id param validation. |
| create-a-container/resources/apikeys/tests/apikeys.api.test.js | Integration tests pinning /api/v1/apikeys wire contract. |
| create-a-container/resources/apikeys/tests/service.test.js | Unit tests for apikeys service with mocked repository. |
- Makefile: ship app.js and resources/ in the packaged install — server.js requires ./app and the v1 router requires resources/apikeys/router, so an installed package would have failed at runtime. Also prune __tests__ directories from the staged tree (middlewares/ and resources/ now contain colocated tests). Verified via make install DESTDIR=<stage>: both files ship, zero __tests__ dirs staged. - validator: description accepts explicit JSON null (nullish) — the legacy router passed null through to storage; the schema was rejecting it with 400, a wire-contract regression. Pinned with a new integration test (24 total).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First installment of the create-a-container MVC refactor: the manager's route files have grown into "god routes" (
routers/api/v1/containers.jsis 860 lines; its POST handler alone is ~175 lines mixing validation, business logic, eight model writes, manual transactions, and error mapping). This PR establishes the target architecture, the test safety net, and proves the pattern end-to-end on the smallest resource.Before you balk at the diff size
The raw diff is ~6,400 insertions. 94% of that is not application code:
package-lock.json(~5,100), tests + helpers (~570), manifesto doc (~430), jest/Make configAnd within that +374 of application code:
server.jsmain()relocated verbatim intoapp.js(see the matching −95 in server.js)middlewares/validate.js(zod validation → standard error envelope)resources/apikeys/replacing the 89-line route fileunref(),secure: 'auto'cookie, app-levelcsrfGuard+jsonErrorHandlerNet: roughly +65 lines of new production behavior, plus +65 net for the apikeys migration — the growth there is the cost of layer boundaries (imports/exports/docblocks across six files instead of one) and is front-loaded: the shared scaffolding amortizes across every subsequent resource, and the big files (
containers.js) already contain duplicated serializers and inline query builders that these layers absorb.Suggested review order: the manifesto, then
resources/apikeys/against the deletedrouters/api/v1/apikeys.js, thenapp.jsside-by-side with the oldserver.js, then the tests.The architecture
Read first:
create-a-container/docs/mvc-manifesto.md— normative for humans and agents. It defines:resources/<name>/containingrouter.js,controller.js,service.js,repository.js,serializer.js,validator.js, with strict downward-only dependencies. Layers that outgrow one file become folders (service/index.js+ focused modules).{ data }/{ error: { code, message, fields? } }), status codes, and paths — the React client andopenapi.v1.yamlmust not notice the refactor. Built on the existingmiddlewares/api.jscontract (ApiError,asyncHandler,jsonErrorHandler), not a replacement for it.apikeys) to largest (containers).What's in this PR
f7848d2f4d4f7962middlewares/validate.js; sqlite test DB; user/API-key factories0078dd19/api/v1/apikeyswire contract before touching it42e1b6a5routers/api/v1/apikeys.js(89 lines) → six-file slice inresources/apikeys/; router is now pure wiring; +5 service unit tests1068d51fapp.js(buildApp) fromserver.js(migrations + secrets + listen) so tests exercise the real production middleware stack, not a hand-rolled copy893dbfd4make testadded to the repo Makefile standard (root loop + all three components)168da7297ad1d19dsecure: 'auto'session cookie (object form, statically analyzable); app-widecsrfGuard+ app-leveljsonErrorHandlerRebased onto current
main(#373 package enhancements): the mergedserver.jskeeps the new umzugrunMigrationsstartup step and the fatal-error exit handler on the server.js side of the app/server split, andpackage.jsonfoldsumzug+ thesequelize-clidevDependencies move together with the new test dependencies.Behavior notes
The pinned integration tests pass unchanged across the migration. Deliberate error-path changes:
:id→400 invalid_request(previously 404 on sqlite / would be 500 on postgres);description> 255 chars →400(previously dialect-dependent DB error)jsonErrorHandler)csrfGuard; all real state-changing routes live under/api/v1where the guard already applied)CodeQL status
js/clear-text-cookieis fixed (secure: 'auto').js/missing-token-validationremains open as a false positive: CodeQL does not modelcsrf-sync(only csurf/tiny-csrf/lusca/fastify), and the synchronizer-token check runs inside the library where the analyzer doesn't look. The protection is real —csrfGuardwrapscsrf-sync'scsrfSynchronisedProtectionand now covers every handler app-wide. Will be dismissed manually.Conflict review
Checked all open PRs before starting: #394, #365, #318, #296 touch
containers.js/nodes.js/models — none touchapikeys,middlewares/,server.js, or the Makefiles. The playbook sequencesnodesafter #394 andcontainerslast partly for this reason.Testing
make testfrom the repo root: 23 tests (10 pinned integration via supertest against the real app + sqlite, 5 service unit, 8 validate middleware), verified over 5+ consecutive runs and re-run after the rebase/api/v1/health200, unauthenticated/api/v1/apikeys401, CSRF failure returns the exact{"error":{"code":"csrf_invalid"}}envelopeNext (separate PRs)
Phase 2:
settings,groups,external-domainsper the playbook. A CI workflow invokingmake testwould make the pinning tests guard those migrations — happy to add it here or separately.