refactor(donors): declarative route table [3/8] - #343
Merged
Conversation
No lambda is converted yet -- this only lands the package the conversions
build on, so it is a pure addition.
The six handlers each route with a chain of `if (normalizedPath === ...)`
statements that test two or three path spellings per route, because API
Gateway's {proxy+} forwards the full path (/projects/7) while the shared
dev-server strips the first segment (/7). Params come out of hand-rolled
`split('/')[2]` and regex tests, correctness depends on `if` ordering that
nothing enforces, and `json()` is defined six times over with `requireAuth`
three times.
@branch/lambda-http replaces that with a declarative route table:
- dispatch({ prefix, routes }) canonicalizes the path to the prefixed shape so
one table serves both callers, matches `:param` segments, and centralizes
OPTIONS preflight, /<prefix>/health, 404 and 500.
- json() with CORS headers, parseBody(), requireAuth() and a createAuthGuard()
factory that binds a service's db-scoped authenticateRequest.
- 28 unit tests, including route precedence and both path shapes.
The dispatch/match/response/types modules are recovered from the closed PR
#257; the auth and body helpers are new. No infrastructure change is needed --
{proxy+} and ANY already landed on main via PR #279.
CI: both workflows build lambda-http after lambda-auth (it consumes that
package's dist), a shared-http job runs its tests and is added to the
lambda-tests gate, and lambda-deploy triggers on shared/lambda-http/**.
Note: lambda-deploy still does not trigger on shared/lambda-auth/**, a
pre-existing gap left alone here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pure reorganization, no behaviour change:
- handler.ts is now a one-liner: dispatch(event, { prefix: 'users', routes }).
- routes.ts holds the ordered Route[] table, bracketed by the ROUTES-START/
ROUTES-END markers (moved here from handler.ts) in the same order as the
original if-chain: GET /users, GET /users/:userId, PATCH /users/:userId,
DELETE /users/:userId, POST /users.
- controllers/users.ts holds one RouteHandler per route, calling Kysely
directly (no services/ layer — this lambda is thin). Auth now goes through
createAuthGuard(authenticateRequest) from @branch/lambda-http instead of a
handler-local requireAuth/checkAuthorization pairing; @branch/lambda-auth's
checkAuthorization (which the shared requireAuth calls) is behaviourally
identical to the removed local copy for every level this lambda uses.
- Local json()/requireAuth() helpers deleted in favor of the @branch/lambda-http
exports. dev-server.ts, db.ts, auth.ts, validation-utils.ts, swagger-utils.ts
untouched.
- Added @branch/lambda-http as a dependency, regenerated package-lock.json.
- tsconfig.json now includes controllers/**/*.ts.
- Added a route-precedence unit test (literal /users/me vs /users/:userId).
Existing suites pass unmodified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the if-chain in handler.ts with a Route[] table dispatched via @branch/lambda-http's dispatch(). handler.ts is now a one-liner; route logic moved into controllers/donors.ts (GET/POST /donors, DELETE /donors/:id) and controllers/donations.ts (GET/POST /donors/donations, DELETE /donors/donations/:id), in the same order as the original if chain. No services/ layer added, per this lambda's existing shape. Local json() removed in favor of the shared one; auth stays manual (authenticateRequest + custom 401/403 messages) since this lambda's authorization messages don't match @branch/lambda-http's generic requireAuth reasons. ROUTES-START/END markers moved into routes.ts, now bracketing the route table entries. Added @branch/lambda-http as a dependency and regenerated package-lock.json. Added one test asserting GET /donors/donations reaches the donations controller rather than a donor-id route. No behavior change: same status codes, messages, and validation order. Verified via tsc --noEmit and jest (--runInBand to avoid DB contention with sibling lambda test runs): 51 passed, 1 pre-existing failure (health test requires a live dev-server on :3000, fails identically on main).
# Conflicts: # .github/workflows/lambda-deploy.yml # .github/workflows/lambda-tests.yml
# Conflicts: # apps/backend/lambdas/donors/handler.ts
The lambda declares @branch/lambda-http as a file: dependency, but the Dockerfile only copied and built shared/lambda-auth, so npm install inside the image resolved a path that was never copied and `make up` failed at build time. Mirrors the existing lambda-auth stage, placed after it: lambda-http resolves lambda-auth as file:../lambda-auth and consumes its dist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview deploys were failing at esbuild with "Could not resolve @branch/lambda-http" while lambda-tests and lambda-deploy were green. Three workflows each encoded their own copy of "build the shared packages a lambda depends on before packaging it", and adding @branch/lambda-http updated only two of them. preview-env.yml still built lambda-auth alone, so the lambda's npm ci installed a file: dependency whose dist had never been built and the bundle could not resolve the import. Replaces all of it with .github/actions/build-shared-packages, used by lambda-tests (test + shared-http), lambda-deploy (build) and preview-env (deploy). Build order lives in one place now: lambda-http declares lambda-auth as file:../lambda-auth and compiles against its dist, so it goes second. The next shared package added is the actual test of this: one edit instead of four, with no fourth copy left to forget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ctor/lambda-users
…nversion
The lambda-readme workflow regenerates every README and pushes the result, and
it ran the old CLI against a converted lambda: extractRoutesFromHandler parses
if-conditions out of handler.ts, which is now four lines, so it found no routes
and auto-committed a README with all five of users' endpoints deleted.
- extractRoutesFromHandler now prefers a sibling routes.ts and falls back to the
if-chain parse, so it reads converted and unconverted lambdas alike. That
matters inside this stack, where only some lambdas have been converted at any
given commit.
- collectRoutes lists health once, under the service prefix for a converted
lambda and bare otherwise, instead of hardcoding /health and duplicating a
spec entry that spells it the other way.
- users/openapi.yaml is normalized to match: paths carry the /users prefix and
servers is the bare host. It previously contradicted itself -- servers ended
in /users AND the /users path was prefixed, so Swagger built /users/users,
while /{userId} had no prefix at all.
The fuller CLI rebuild lands in 8/8; this is the subset needed for the README
workflow to stop rewriting these files as each lambda converts.
expenditures/README.md picks up a POST /expenditures row: pre-existing drift on
main that the workflow would have auto-committed anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lambda declares @branch/lambda-http as a file: dependency, but the Dockerfile only copied and built shared/lambda-auth, so npm install inside the image resolved a path that was never copied and `make up` failed at build time. Mirrors the existing lambda-auth stage, placed after it: lambda-http resolves lambda-auth as file:../lambda-auth and consumes its dist. README regenerated so the lambda-readme workflow has nothing to push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Stack 3/8. Base is #342, not
main. Pure reorganization of thedonorslambda; no behaviour change.handler.tsdrops from 349 lines to 4. The six routes, in the order theifchain tested them:/donorsdonors.getDonors/donors/donationsdonations.getDonations/donors/donationsdonations.createDonation/donorsdonors.createDonor/donors/:iddonors.deleteDonor/donors/donations/:iddonations.deleteDonationTwo controller files, split by resource:
controllers/donors.tsandcontrollers/donations.ts. Noservices/layer — this lambda is thin enough that one would be ceremony. The localjson()is deleted in favour of the shared one.Worth noting what the patterns replace:
DELETE /donors/:idwas/^\/[^\/]+$/.test(normalizedPath)plusnormalizedPath.split('/')[1], andDELETE /donors/donations/:idwasnormalizedPath.startsWith('/donations/') && normalizedPath.split('/').length === 3plussplit('/')[2]. Their relative order is preserved.Auth left inline, deliberately
This lambda does not adopt the shared
requireAuth/createAuthGuard. Every route ran an identical inlineisAuthenticatedcheck, but the follow-on authorization uses domain-specific messages —'Only admins can delete donors','You must be a member'— thatcheckAuthorization's generic reasons would have replaced. Keeping the original conditionals guarantees identical status codes and message strings. Consolidating those messages is a reasonable follow-up, but it is a behaviour change and does not belong in a refactor PR.Tests
51 passing, 1 pre-existing failure,
tsc --noEmitclean. Adds a precedence test thatGET /donors/donationsreaches the donations controller rather than being swallowed by/donors/:id.The failure is
test/donors.test.ts→Donor API with data › health test 🌞, which does a realfetch("http://localhost:3000/donors/health")and fails identically on unmodifiedmainwithout the donors container running. Not caused by this PR.tsconfig.json'sincludegainscontrollers/**/*.ts. Typechecking already reached the new files transitively through imports; this just makes it explicit and matches the other lambdas in the stack.Noticed, not fixed
donors/openapi.yamlis missing theGET /donationspath, which the handler implements and the tests cover. Pre-existing spec gap.donors/openapi.yamlthat closed PR fix: route lambda sub-paths via API Gateway proxy + shared dispatcher #257 claimed to fix is not present in the current file — either already fixed or it was never quite as described.Stack-wide note: unmatched paths now 404 instead of 401
Previously the top-level auth check ran before route matching, so an unauthenticated request to a nonexistent path got 401.
dispatchreturns 404 centrally without running auth, so an unauthenticated caller can now distinguish "route exists" (401) from "route doesn't" (404). Accepted deliberately — the route inventory is already in the repo'sopenapi.yamlfiles, and auth-before-routing is exactly what forces each lambda to hand-roll public-route exemptions. Flagged in every stack PR so it is a decision rather than an accident.🤖 Generated with Claude Code