Feature/api surface - #12
Merged
Merged
Conversation
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.
Phase 9 — API surface (steps 44–52)
Ten commits: JWT auth with the three roles (44), tenant resolution middleware (45), centralized
ProblemDetails error mapping (46), Customers/Technicians/Jobs CRUD over HTTP (47), Schedule/Dispatch
over HTTP (48), Invoicing/Export over HTTP (49), the Sync controller (50), Attachment upload (50b),
the SignalR dispatch hub (51) — all committed and reviewed — and finalizing/publishing the contract
(52, pending review below), the step Document 3 itself puts last in Phase 9: every REST endpoint the
build plan named exists, the board is live, and now the document describing all of it is complete and
the package built from it is a real, versioned, installable release rather than a path a client
happens to sit beside.
780 tests pass (646 unit, 134 integration); zero warnings in Debug and Release under nullable +
analyzers + warnings-as-errors;
make gen-contractsbuilds and type-checks;make check-contractsand
make check-contracts-sampleboth pass against a clean regeneration.OpenApi/OpenApiSecurity(new — bearer scheme + per-operation requirement),OpenApi/OpenApiErrorResponses(new — shareddefault: ProblemDetailsresponse); every*Endpoints.csfile gains.Produces<T>()per route;Program.cswires both new transformersJobs/CreateJobResponse(new — replacesPOST /jobs's anonymous{ id })OpenApiSchemaNames(new — readsopenapi.json'scomponents/schemas);Program.csfilters the C#-sourced sweep through it;PackageManifestgets a real version, dropsprivate, addspublishConfig.accessMakefile:check-contracts-sample,publish-contracts(new targets);ci.yml: sample-client step added tocontract-drift, new tag-gatedpublish-contractsjob;tools/sample-client/(new — a throwaway consumer proving the package resolves through its ownexports)OpenApiSchemaNamesTests(new — the one piece of hand-written step-52 logic a unit test can reach); everything else verified by generating the real artifact and reading it, not by a test asserting a hand-maintained expectationDecisions a reviewer should weigh in on
Steps 44–51 (committed) — one line each; full reasoning in
DECISIONS.local.mdJWT auth,
TenantContextmadepublic,MapInboundClaims=false, centralizedResultHttpMapping+UnhandledExceptionHandler, no hard delete for Customer/Technician,AuthPolicies.AdminOrDispatcherintroduced and split for Technicians,
POST /jobs/{id}/assignanswers 200, boarddayresolves toUTC,
OptimizeDayValidator.MaxHorizoncaps a re-plan at 90 days, unassigned-job reasons re-deferredwith no owner,
AuthPolicies.AdminOnlyfor Invoicing/Export,GenerateInvoiceCommandreturnsInvoiceSummarynow, money crosses the wire as decimal dollars,GET /exportomits technicians,AuthUser.TechnicianId/techJWT claim, technician resolved inline rather than through a newambient port, pull's payloads typed while push's stay opaque, pull's missing horizon left with no
owner, attachment upload idempotent by client id with the server id being the storage key, no
cross-technician ownership check on an upload, a 25 MB cap with no content-type sniff,
IBoardNotifierhas two methods not three,
Invoiced/new-stop pushes declined,DispatchHub/SignalRBoardNotifierforced into
Apirather thanInfrastructure, hub connectionAdminOrDispatcher, JWT-over-query-stringscoped to
/hubs, a genuinePostgresFixturetenant bug found and fixed. Nothing here changed thisstep.
Step 52 (pending review)
A route needs the security scheme only if it carries
IAuthorizeDataand noIAllowAnonymous—not merely "isn't AllowAnonymous." The narrower check would have wrongly documented
GET /healthas requiring a token; nothing maps that route to a policy at all. Caught by generating the real
document and reading it, not by trusting the code.
Every operation gets a shared
default: ProblemDetailsresponse from one document transformer,not a per-route list of specific status codes. Which codes a route can actually answer with lives
in its handler's own
ErrorCategoryusage, which a transformer cannot see; a hand-maintained listwould drift from it. Registered once under
components/schemas/ProblemDetailsand referenced byevery operation rather than inlined twenty-eight times — caught and fixed by reading the generated
document, not assumed correct on the first attempt.
POST /sync/push/GET /sync/pulleach alsocarry an explicit bodyless
401, because both can answer one ahead of this shared entry (a missingtechclaim) that is not aProblemDetailsbody..Produces<T>()at every route, not a rewrite of every handler's return type to something theframework can infer from. Before this step the exported document had zero response schemas —
not even a correct status code, every route showed a blanket
200: OKregardless of whether itactually answers 200, 201, or 204 — because every handler returns the same non-generic
IResult.The alternative (
Results<Ok<T>, Created<T>, ...>typed unions everywhere) would touch everyendpoint delegate's signature for a step whose text is about description, not restructuring.
POST /jobsnow returns a namedCreateJobResponse(Guid Id)instead of an anonymous{ id }.Every sibling creation endpoint in this API already echoes a named record; this was the one
exception, and an anonymous type has no name
.Produces<T>()or an OpenAPI document can point at.Wire-compatible: both shapes camelCase to the identical
{"id": "..."}body, so no test needed tochange.
The Contracts C#/OpenAPI duplication (tracked since step 22) is fixed, not merely documented
again.
Contracts.CodeGennow readsopenapi.json'scomponents/schemasand excludes anynon-enum, non-static-class type already named there from the C#-sourced sweep — mechanical, not a
hand-maintained list.
contracts/src/index.tsgoes from 56 exported shapes to 14 (the sharedenums,
BoardEvents, the three SignalR payload types, the three opaque sync-payload types); enumsstay in both files deliberately, because a kept shape (
JobUpdated.status) can still reference one.The package is real and publish-ready — versioned, not
private,publishConfig.access: "public"— but nothing in this session has actually published it, and the CI job that would isgated on a tag and will fail cleanly at its one auth step until a repository admin adds a real
NPM_TOKEN. No npm account or registry claim exists anywhere in this repository to publishagainst; inventing one would be infrastructure no document asks for. Worth a reviewer's decision:
this repo's GitHub org is
opendispatchorg, notopendispatch— if the eventual registry is GitHubPackages rather than the public npm registry, the package's own scope would need to change to match.
The sample-client compile check (
tools/sample-client/,make check-contracts-sample) is asecond, separate node project from
tools/, resolving@opendispatch/contractsthrough its ownpackage.jsonvia a realnpm installrather than a relative path intocontracts/src. This iswhat
gen-contracts' own directtsc contracts/src/*.tsinvocation cannot catch — a brokenexportsentry or a missingtypesfield — and is exactly step 52's own "a sample client importcompiles against the package."
A pre-existing, unrelated
dotnet formatfailure inRepositoryTests.cs(nine stray spaces) wasfound and fixed while finishing this step. Confirmed present on the unmodified branch via
git stashbefore touching it; whitespace only, no test logic changed.Out-of-sequence follow-up (pending review): correlation id on ProblemDetails
Not a build-plan step — closing the one step-46 "no owner" gap this branch left behind, at the
user's request after step 52 landed.
Program.cs'sAddProblemDetails()now attachesextensions.traceIdto every ProblemDetails response viaCustomizeProblemDetails(one hook,covers
ResultHttpMapping.ToProblem's four categories andUnhandledExceptionHandleralike), andUseSerilogRequestLogging'sMessageTemplate— not just itsEnrichDiagnosticContext— now names{TraceId}explicitly, alongside the same addition toUnhandledExceptionHandlerLog's owntemplate.
Worth a reviewer's attention: the first attempt at the log-correlation half didn't work, and the
fix is why two things changed instead of one.
EnrichDiagnosticContextalone attachesTraceIdas a structured property, but
appsettings.json'sConsolesink has nooutputTemplate, andSerilog's default console rendering only prints a template's own named tokens — confirmed by
running the real host and reading actual console output, where enrichment alone produced nothing
visible. Naming
{TraceId}directly inRequestLoggingOptions.MessageTemplatefixed it, verifiedagain the same way: response body, the exception handler's own log line, and the request-summary
line all printed the identical id for one real request. Full reasoning in
DECISIONS.local.md's(reopened) step-46 entry.
Tests:
ErrorMappingFlowTests' two existing facts extended with atraceId-present assertioneach, rather than a new file — the same "Result-mapped path and raw-exception path, one flow
each" the file already covers. The log-line correlation itself stays a manual, empirical check
rather than an automated one: no in-memory Serilog sink exists anywhere in this suite, and adding
one to assert a substring in a log line is disproportionate to what this gap actually needed.
What is deliberately not here
step 47.
GET /invoices/{id}. Carried from step 49.GET /sync/pull; noITechnicianContext. Carried from step 50.download endpoint for attachment bytes. Carried from step 50b.
technician.moved; no board push for a job becoming Invoiced or a brand-new stopappearing; no coalescing of a reassignment's two
assignment.updatedpushes. Carried fromstep 51.
Development. Reconsidered this step and leftas is — see
DECISIONS.local.md. No owner if a hosted deployment wants it behind auth instead.@opendispatch/contractshas never actually been published. The package and the CI job areboth ready; the registry credential is not this session's to create. No owner until a repository
admin adds
secrets.NPM_TOKEN(and settles the npm-scope-vs-GitHub-org question above).left in the backend build plan — step 53 (seed data), 54 (observability), 55 (end-to-end test),
56 (README/demo) — is Phase 10.