perf(lambdas): stop paying for avoidable database round trips

[nourshoreibah]

What

Removes database round trips from the lambda controllers, and changes two query
shapes. No behavioural change beyond the two 404-ordering notes called out below.

Round trips

Endpoint	Before	After	How
GET /donors (paginated)	2	1	Promise.all count + page
GET /donors/donations (paginated)	2	1	Promise.all count + page
GET /users (paginated)	2	1	Promise.all count + page
GET /reports (paginated)	2	1	Promise.all count + page
GET /expenditures (paginated)	2	1	Promise.all count + page
POST /donors/donations	3	2	donor-exists and project-exists checks in one Promise.all, then the insert
GET /expenditures/{id}	3	1	LEFT JOIN users + JOIN projects folded into the row read
PATCH /expenditures/{id}	3	2	.returningAll() instead of a post-write re-read (the pre-write read stays — the RBAC resourceOf check needs it)
PATCH /expenditures/{id}/status	3	1	UPDATE ... RETURNING settles the 404 and the body
PATCH /users/{userId}	3	1	single UPDATE ... RETURNING, existence folded into the affected row
POST /reports (fetchReportData)	4	2	project read first (it is the 404), then Promise.all of members + donations + expenditures
GET /projects/{id}/overview	2 waves	1 wave	project read folded into the existing Promise.all, now four queries

"Waves" for the overview row because the three project-scoped queries were
already parallel; the project read was a serial hop in front of them.

Query shapes

1. IN (...) to a single array parameter — done. projectScopeIds returns a
number[] whose length varies per caller, so where('project_id','in',scope)
emitted in ($1, ..., $n) and Postgres re-planned per distinct cardinality.
Now sql\project_id = ANY(${ids})`, one bound array, one plan. Changed at all three call sites: projects/controllers/projects.ts, donors/controllers/donations.ts, expenditures/services/scope.ts`.

projectScopeIds itself is untouched, so its contract holds exactly: null for
an unrestricted caller (filter skipped), and the MATCHES_NOTHING (-1)
sentinel — never an empty array — for a caller who is a member of nothing.
Verified against a real database on all three branches:

scope null   -> select * from "branch"."projects" order by "project_id" asc            -> [1,2,3,4]
scope [3,7,9]-> ... where project_id = ANY($1) ...  params [[3,7,9]]                   -> [3]
scope [-1]   -> ... where project_id = ANY($1) ...  params [[-1]]                      -> []

The empty-scope caller still sees nothing, and = ANY('{-1}') is valid SQL
where IN () would not have been.

2. UNION rewrite of the OR disjunction — SKIPPED. Deliberately. Three
reasons:

• applyExpenditureScope is the single function that puts the same predicate on
  both the count and the page. A UNION of two arms is a page-shaped
  construction; the count would have to be built by different code, which is
  exactly the divergence the pagination rule exists to prevent.
• Correct OFFSET pagination over a UNION needs limit + offset rows from
  each arm before the merge, and UNION's dedup can then drop the count below
  limit while more rows exist — short pages, the bug class this predicate is
  meant to avoid.
• The two arms overlap heavily in practice (your own expenses are usually on
  your own projects), so the sort it would avoid is over a set that is close to
  the one arm alone.

A correct slow query beats a fast wrong one, and the sort is addressable with
(project_id, spent_on DESC) / (entered_by, spent_on DESC) indexes without
touching the predicate. Left for a PR that can own the index change too.

Pagination counts

Confirmed: every count still carries the page's own predicate. getDonations
builds one inScope expression and applies it to both queries;
countExpenditures and queryExpenditures both go through
applyExpenditureScope and both take the same projectId; listReports applies
the same projectId filter to both. Nothing counts a row its page would hide.

Two ordering changes worth a reviewer's eye

• PATCH /users/{userId}: the existence SELECT is gone, so a malformed body
  aimed at a missing id now answers 400 rather than 404. That is the precedence
  every other validated route here already has. Two tests asserted the old
  order and are updated — users.test.ts's "patch user 404 test" was sending an
  immutable email field, so it was asserting a 404 over what is really a 400;
  its body now contains only the field it means to patch.
• PATCH /expenditures/{id}/status: the pre-write read is dropped too, not just
  the post-write one. There is no record-level gate on this route
  (expense:review on the route settles it), so nothing needed the row in hand;
  an UPDATE matching no row is a no-op and returns nothing, which is the same
  404 with the same message. This is one step past the brief — easy to revert to
  3-to-2 if you would rather keep the read.

GET /projects/{id}/overview now issues the three scoped queries even when the
project does not exist. They return empty and the 404 is unchanged; the trade is
a little wasted work on a 404 for one less round trip on every success.

Verification

Built shared/rbac, shared/types, shared/lambda-auth, shared/lambda-http
from this branch and pointed each lambda at them. npx tsc --noEmit clean in all
five touched lambdas. Tests run against a throwaway postgres:16-alpine, not the
shared local instance.

Lambda	Result
donors	59 passed, 1 failed (health test fetches localhost:3000; pre-existing, needs a dev server)
users	53 passed, 0 failed
reports	112 passed, 0 failed
expenditures	128 passed, 0 failed
projects	115 passed, 0 failed

467 of 468 passing; the one failure is the known health check.

Out of scope

Untouched: shared/lambda-auth/src/authenticate.ts and rbac.ts, the
unbounded-response / default-limit work, selectAll() narrowing, and
dashboard.ts's getOverview Node-side aggregation.

🤖 Generated with Claude Code

[nourshoreibah]

Merged main in and resolved the conflict

This branch was cut from ddf0988; main has since gained #353, #358, #360, #361, #362, #363, #364, #365. One real conflict, in apps/backend/lambdas/users/controllers/users.ts, between #353 (profile page) and this PR's patchUser rewrite.

Resolution: kept both sides. #353's additions to the response — await resolveProfileImage(...) and created_at — are preserved, and the ! non-null assertions it needed are dropped, because this PR's if (!updatedUser) return json(404, ...) already narrows the type.

const updatedUser = await db
  .updateTable('branch.users')
  .set(updates)
  .where('user_id', '=', Number(userId))
  .returningAll()
  .executeTakeFirst();

if (!updatedUser) return json(404, { message: 'User not found' });

return json(200, { ok: true, /* ... */ body: {
  email: updatedUser.email,
  name: updatedUser.name,
  isAdmin: updatedUser.is_admin,
  profileImage: await resolveProfileImage(updatedUser.profile_image),
  created_at: updatedUser.created_at,
} });

The part worth reviewing

Git also produced a clean but semantically wrong auto-merge that I had to undo. #353 edited the region around patchUser's existence check while this PR deleted that check, so git happily kept both: the // make sure user exists pre-read came back and the UPDATE ... RETURNING stayed, with a redundant re-read after it. That compiles and passes a naive read, but it silently reverts this endpoint from 3 round trips to 1 back to 3 — the exact win the PR exists for. The committed resolution has the pre-read and the post-read gone, as intended.

Test fix in the follow-up commit

user.photos.unit.test.ts (added in #353) mocks updateTable().set().where().execute() plus a re-read. With the single-statement rewrite the chain ends in returningAll().executeTakeFirst(), so the old mock returned undefined and threw — surfacing as 500 instead of 200 in the two tests that PATCH a valid profileImage. The mock now matches the chain the code actually uses.

Verification

• npx tsc --noEmit in lambdas/users — exit 0.
• npx jest on user.unit.test.ts, user.photos.unit.test.ts, user.privilege-escalation.test.ts — 47/47 pass (2 failing before the mock fix).
• CI on ca38d74: test (apps/backend/lambdas/users/) pass, all other jobs pass, no failures.

Note I ran unit suites only, not e2e: every e2e fixture pool except donors' hardcodes port 5432 rather than reading DB_PORT, so running them would have truncated the shared local Postgres other work is using. That is a real (small) bug worth its own PR, and CI is unaffected since it gets a fresh database per lambda.

[github-actions]

🌿 ⏳ Creating preview environment… (logs)

[github-actions]

🌿 Preview environment — ready ✅

Open: https://d3nmtjoh6ir9ym.cloudfront.net/pr-366/
API: https://17ojiue68g.execute-api.us-east-2.amazonaws.com/prod

Shared RDS + Cognito (prod data); DB migrations are not applied here — if this PR adds a migration, endpoints using the new columns will fail until it merges. New commits update this environment in place — a note is posted here on each update. Remove the test-environment label or close the PR to tear it down.

[github-actions]

🌿 Test environment updated in place ✅ — Click here to open. _{updated for 2e6d9ec · logs}

[github-actions]

🌿 Preview environment torn down 🧹 — the stack for this PR has been destroyed.