Establish the Minimal API endpoint structure (task 57) #16

Merged

rob merged 1 commit from feat/api-structure into main 2026-08-03 11:16:13 +00:00

Conversation 1 Commits 1 Files changed 4 +121

rob commented 2026-08-03 11:06:24 +00:00

Owner

Copy link

Feature-grouped MapGroup extension methods per CLAUDE.md, with GET /health as the worked example. Program.cs gets one line per feature and no route patterns.

15 tests pass; build, Release build and format all clean. Also run for real: /health → 200, POST /health → 405, /missing → 404.

Choices worth challenging

No aggregator method. Program.cs growing one line per feature reads as a table of contents; indirection with one entry buys nothing. One-file change if you disagree.

No contracts example. /health returns an empty 200, so no wire type was invented — a HealthResponse would be a shape task 63 redefines. ADR-0009's boundary is better demonstrated by the first real feature endpoint. This is the one acceptance-criteria-adjacent thing the example does not show.

A group rather than bare MapGet calls, because the group is where a per-feature authorisation policy, filter or rate limit is declared once and inherited.

Left room for

Versioning (62) — only the feature's own segment is written in a feature file, so /api/v1 becomes a parent group. Exception handling (58), CORS (61), rate limiting (123) attach in Program.cs. Health checks (63) replace the one method body. OpenAPI (60) — TypedResults already carries the response type.

UseHttpLogging untouched and still outermost. Adding /health does not make ADR-0015's logged-as-200 defect reachable — the route returns a constant and cannot throw.

Feature-grouped `MapGroup` extension methods per `CLAUDE.md`, with `GET /health` as the worked example. `Program.cs` gets one line per feature and no route patterns. 15 tests pass; build, Release build and format all clean. Also run for real: `/health` → 200, `POST /health` → 405, `/missing` → 404. ## Choices worth challenging **No aggregator method.** `Program.cs` growing one line per feature reads as a table of contents; indirection with one entry buys nothing. One-file change if you disagree. **No contracts example.** `/health` returns an empty 200, so no wire type was invented — a `HealthResponse` would be a shape task 63 redefines. ADR-0009's boundary is better demonstrated by the first real feature endpoint. This is the one acceptance-criteria-adjacent thing the example does not show. **A group rather than bare `MapGet` calls**, because the group is where a per-feature authorisation policy, filter or rate limit is declared once and inherited. ## Left room for Versioning (62) — only the feature's own segment is written in a feature file, so `/api/v1` becomes a parent group. Exception handling (58), CORS (61), rate limiting (123) attach in `Program.cs`. Health checks (63) replace the one method body. OpenAPI (60) — `TypedResults` already carries the response type. `UseHttpLogging` untouched and still outermost. Adding `/health` does **not** make ADR-0015's logged-as-200 defect reachable — the route returns a constant and cannot throw.

rob added 1 commit 2026-08-03 11:06:24 +00:00

Establish the Minimal API endpoint structure ... c3a8b4ef4f

Feature-grouped MapGroup extension methods, one folder per feature area,
with Program.cs holding one line per feature and no route patterns. The
group is the seam where a per-feature policy, filter or rate limit is
declared once and inherited by routes added later — stated in the doc
comment so it is not flattened into bare MapGet calls.

GET /health returns an empty 200. Liveness only: task 63 owns real health
checks and the doc comment says so. TypedResults rather than Results so
the response type is already metadata for task 60.

Deliberately no contracts example: /health returns no body, and inventing
a HealthResponse would be a shape task 63 redefines. The boundary is
better demonstrated by the first real feature endpoint.

No aggregator method — Program.cs growing one line per feature reads as a
table of contents, and indirection with one entry buys nothing.

UseHttpLogging is untouched and still outermost. Adding /health does not
make the logged-as-200 defect reachable: the route returns a constant and
cannot throw.

rob reviewed 2026-08-03 11:15:33 +00:00

rob left a comment

Copy link

Verdict: mergeable

Independent review, in a detached worktree at c3a8b4e. Nothing blocking; the notes below are all optional, and none of them needs to be actioned before merge.

What I ran

• dotnet build — succeeded, 0 warnings (with TreatWarningsAsErrors).
• dotnet build -c Release — succeeded, 0 warnings.
• dotnet test — 15 passed, 0 failed (4 Api, 1 WebUI, 10 Infrastructure, Docker present).
• dotnet format --verify-no-changes — clean.
• Ran the API and hit it: GET /health → 200 with Content-Length: 0, POST /health → 405, GET /missing → 404. All three claims hold. Also GET /health/ → 200, HEAD /health → 405, and one HttpLogging record per request including the 405s and the 404.

Acceptance criteria

All three are met. /health returns 200 from a running process, not only from a test host. Routes live in Health/HealthEndpoints.cs behind an extension method and Program.cs contains no route pattern. The dependency direction is untouched and still correct (Api → Infrastructure → Domain, Api → Contracts), and the ticket's DI wiring already landed with AddPlaceMarkDatabase.

The missing contracts example is not a gap. The ticket never mentions contracts, and ADR-0009 decided that PlaceMark.Contracts ships empty and gains types alongside the endpoints that need them. A HealthResponse invented here would be a wire type with no consumer whose shape ticket #63 immediately redefines — it would demonstrate the boundary by violating the reason the boundary was drawn that way. The rule is stated in the new README section and in the dependency-rules section above it, which is as much as review can rely on given ADR-0009's own admission that nothing in the build enforces it.

No aggregator

Right call. One line per feature in Program.cs is the table of contents; an aggregator would be a second file to open to find out which features exist, and it buys nothing until the list is long enough to be unreadable — which at PlaceMark's feature count it will not be. It also stays reversible: the day it is wanted, it is one file and a sed.

Is the pattern unambiguous?

Yes, for the case it covers. The feature file states the rule twice over (in prose and by example), and the README sketch is close enough to compile after the ellipsis is filled in. A contributor adding PlacesEndpoints would get the namespace, the folder, the accessibility, the group and the prefix-ownership rule right without asking. Two small things it does not answer, both cheap to settle when the first real feature lands rather than now:

• Where per-feature service registration goes. Program.cs calls AddPlaceMarkDatabase directly, so the first feature needing a scoped service has to decide between another line in Program.cs and an AddPlacesFeature() beside MapPlacesEndpoints(). The endpoint half of the convention is settled; the registration half is not.
• Whether the returned builder is meant to be chained. The XML doc says "so feature groups can be chained", but Program.cs discards it on the only call site. Harmless, and conventional, but it is a claim nothing in the repository demonstrates.

Room for the follow-ups

I checked the forward-compatibility claims rather than taking them, by rebuilding the same group shape in a throwaway app and dumping the route patterns:

• Versioning (62) holds exactly as claimed. var v1 = app.MapGroup("/api/v1"); v1.MapHealthEndpoints(); yields /api/v1/health with no feature file changing, because RouteGroupBuilder is itself an IEndpointRouteBuilder.
• Health checks (63) fit the seam. MapHealthChecks("/") works inside the group and inside a nested version group; it also answers HEAD, which the current MapGet does not (HEAD /health is 405 today — irrelevant for a container probe, which GETs, but worth knowing if anything polls it with HEAD).
• OpenAPI (60) is unaffected by a detail worth recording. The registered pattern is literally /health/ — MapGroup joins with a separator, and MapGet("") produces the same — so the comment's "/health itself, not /health/" is true of matching but not of RoutePattern.RawText. I checked whether that leaks: with AddOpenApi/MapOpenApi, the document's path keys come out as /health and /api/v1/health. So the claim is true everywhere a consumer can see it, and no change is warranted.
• 58, 61 and 123 all attach in Program.cs or on a group without touching a feature file, as claimed.

One thing to remember rather than fix: when 62 lands, /health should stay outside the version group. A liveness URL that moves to /api/v2/health breaks every probe configured against it, and versioning an endpoint with no body to version buys nothing. The Program.cs comment currently reads as though a version prefix wraps all of these calls; that is the natural reading and it is the wrong outcome for this one feature.

Logging

UseHttpLogging is untouched and still first. The claim that /health cannot make ADR-0015's logged-as-200 defect reachable is correct: the delegate returns a constant TypedResults.Ok(), takes no parameters to bind and touches no service, so there is no user code in the request path that can throw. The 404 and 405 are produced by routing inside the logging middleware, so they are logged truthfully — I confirmed both in the console output.

Side effect worth a thought at #63 rather than here: every probe now writes a request record, and ADR-0015 has no sampling and one sink. A monitor polling every few seconds turns the console into health noise, which is a real cost for logs that record says are meant to be read.

Tests

Conventions are followed: <ClassUnderTest>Tests, <MethodName>_<Scenario>_<ExpectedResult>, Shouldly through the global using, no ?. in front of a Should…, AAA separated by blank lines, and the class sits in a Health/ folder mirroring the feature. The single test asserts the acceptance criterion and nothing more, which is right — 404 and 405 are framework behaviour and testing them would pin someone else's contract.

One nit: unlike RequestLoggingTests, this factory does not pin the environment, so it runs under whatever ASPNETCORE_ENVIRONMENT the machine has — a different middleware pipeline (the developer exception page) on a developer's box than in CI, for no benefit. Its comment also asserts an isolation property (TEST-NET-1, "a request that reached PostgreSQL would hang") that depends on UseSetting outranking user secrets. It does — I measured it, UseSetting beats appsettings.Development.json for the same key — so the comment is true, but it is true by a precedence rule the test does not state and does not need. webHost.UseEnvironment(Environments.Production) would make both points moot and match the file next door.

Not required, but considered

No ADR. Correct by ADR-0001's own test: CLAUDE.md already mandates feature-grouped MapGroup extension methods, so the architecturally significant part was decided before this branch; what is left — no aggregator, a group for a single route — is cheap to reverse and now documented where a contributor will actually meet it.

Verdict: mergeable Independent review, in a detached worktree at `c3a8b4e`. Nothing blocking; the notes below are all optional, and none of them needs to be actioned before merge. ## What I ran - `dotnet build` — succeeded, 0 warnings (with `TreatWarningsAsErrors`). - `dotnet build -c Release` — succeeded, 0 warnings. - `dotnet test` — 15 passed, 0 failed (4 Api, 1 WebUI, 10 Infrastructure, Docker present). - `dotnet format --verify-no-changes` — clean. - Ran the API and hit it: `GET /health` → 200 with `Content-Length: 0`, `POST /health` → 405, `GET /missing` → 404. All three claims hold. Also `GET /health/` → 200, `HEAD /health` → 405, and one `HttpLogging` record per request including the 405s and the 404. ## Acceptance criteria All three are met. `/health` returns 200 from a running process, not only from a test host. Routes live in `Health/HealthEndpoints.cs` behind an extension method and `Program.cs` contains no route pattern. The dependency direction is untouched and still correct (`Api → Infrastructure → Domain`, `Api → Contracts`), and the ticket's DI wiring already landed with `AddPlaceMarkDatabase`. **The missing contracts example is not a gap.** The ticket never mentions contracts, and ADR-0009 decided that `PlaceMark.Contracts` ships empty and gains types alongside the endpoints that need them. A `HealthResponse` invented here would be a wire type with no consumer whose shape ticket #63 immediately redefines — it would demonstrate the boundary by violating the reason the boundary was drawn that way. The rule is stated in the new README section and in the dependency-rules section above it, which is as much as review can rely on given ADR-0009's own admission that nothing in the build enforces it. ## No aggregator Right call. One line per feature in `Program.cs` is the table of contents; an aggregator would be a second file to open to find out which features exist, and it buys nothing until the list is long enough to be unreadable — which at PlaceMark's feature count it will not be. It also stays reversible: the day it is wanted, it is one file and a `sed`. ## Is the pattern unambiguous? Yes, for the case it covers. The feature file states the rule twice over (in prose and by example), and the README sketch is close enough to compile after the ellipsis is filled in. A contributor adding `PlacesEndpoints` would get the namespace, the folder, the accessibility, the group and the prefix-ownership rule right without asking. Two small things it does not answer, both cheap to settle when the first real feature lands rather than now: - **Where per-feature service registration goes.** `Program.cs` calls `AddPlaceMarkDatabase` directly, so the first feature needing a scoped service has to decide between another line in `Program.cs` and an `AddPlacesFeature()` beside `MapPlacesEndpoints()`. The endpoint half of the convention is settled; the registration half is not. - **Whether the returned builder is meant to be chained.** The XML doc says "so feature groups can be chained", but `Program.cs` discards it on the only call site. Harmless, and conventional, but it is a claim nothing in the repository demonstrates. ## Room for the follow-ups I checked the forward-compatibility claims rather than taking them, by rebuilding the same group shape in a throwaway app and dumping the route patterns: - **Versioning (62) holds exactly as claimed.** `var v1 = app.MapGroup("/api/v1"); v1.MapHealthEndpoints();` yields `/api/v1/health` with no feature file changing, because `RouteGroupBuilder` is itself an `IEndpointRouteBuilder`. - **Health checks (63) fit the seam.** `MapHealthChecks("/")` works inside the group and inside a nested version group; it also answers `HEAD`, which the current `MapGet` does not (`HEAD /health` is 405 today — irrelevant for a container probe, which GETs, but worth knowing if anything polls it with `HEAD`). - **OpenAPI (60) is unaffected by a detail worth recording.** The registered pattern is literally `/health/` — `MapGroup` joins with a separator, and `MapGet("")` produces the same — so the comment's "`/health` itself, not `/health/`" is true of matching but not of `RoutePattern.RawText`. I checked whether that leaks: with `AddOpenApi`/`MapOpenApi`, the document's path keys come out as `/health` and `/api/v1/health`. So the claim is true everywhere a consumer can see it, and no change is warranted. - 58, 61 and 123 all attach in `Program.cs` or on a group without touching a feature file, as claimed. One thing to remember rather than fix: **when 62 lands, `/health` should stay outside the version group.** A liveness URL that moves to `/api/v2/health` breaks every probe configured against it, and versioning an endpoint with no body to version buys nothing. The `Program.cs` comment currently reads as though a version prefix wraps *all* of these calls; that is the natural reading and it is the wrong outcome for this one feature. ## Logging `UseHttpLogging` is untouched and still first. The claim that `/health` cannot make ADR-0015's logged-as-200 defect reachable is correct: the delegate returns a constant `TypedResults.Ok()`, takes no parameters to bind and touches no service, so there is no user code in the request path that can throw. The 404 and 405 are produced by routing *inside* the logging middleware, so they are logged truthfully — I confirmed both in the console output. Side effect worth a thought at #63 rather than here: every probe now writes a request record, and ADR-0015 has no sampling and one sink. A monitor polling every few seconds turns the console into health noise, which is a real cost for logs that record says are meant to be *read*. ## Tests Conventions are followed: `<ClassUnderTest>Tests`, `<MethodName>_<Scenario>_<ExpectedResult>`, Shouldly through the global using, no `?.` in front of a `Should…`, AAA separated by blank lines, and the class sits in a `Health/` folder mirroring the feature. The single test asserts the acceptance criterion and nothing more, which is right — 404 and 405 are framework behaviour and testing them would pin someone else's contract. One nit: unlike `RequestLoggingTests`, this factory does not pin the environment, so it runs under whatever `ASPNETCORE_ENVIRONMENT` the machine has — a different middleware pipeline (the developer exception page) on a developer's box than in CI, for no benefit. Its comment also asserts an isolation property (TEST-NET-1, "a request that reached PostgreSQL would hang") that depends on `UseSetting` outranking user secrets. It does — I measured it, `UseSetting` beats `appsettings.Development.json` for the same key — so the comment is true, but it is true by a precedence rule the test does not state and does not need. `webHost.UseEnvironment(Environments.Production)` would make both points moot and match the file next door. ## Not required, but considered No ADR. Correct by ADR-0001's own test: `CLAUDE.md` already mandates feature-grouped `MapGroup` extension methods, so the architecturally significant part was decided before this branch; what is left — no aggregator, a group for a single route — is cheap to reverse and now documented where a contributor will actually meet it.

rob merged commit df1308dc33 into main 2026-08-03 11:16:13 +00:00

rob deleted branch feat/api-structure 2026-08-03 11:16:13 +00:00

rob referenced this pull request from a commit 2026-08-03 11:16:14 +00:00

Establish the Minimal API endpoint structure (#16)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!16

No description provided.