Enforce ADR-0009 rule 3: entities never exposed over the wire #79
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/entities-off-the-wire-test"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes Vikunja task #140, the fifth rule ADR-0030 deferred.
EndpointHandlersfinds every mapped handler by reading which method aMap(Get|Post|Put|Delete|Patch)call names as source text, then reflects on that method's compiled signature — the oneProjectReferencethis test project carries (ADR-0070).EntitiesOffTheWireTestsunwraps return type and parameters through arrays/generics and fails on any leaf declared inPlaceMark.Domain, exempting interface-typed parameters (proven necessary: the first run flaggedIPasswordHasher, a legitimate DI service).Verified by introducing a real violation in
PlaceEndpoints.GetPlaceById(returningOk<Place>), confirming the test failed naming exactly that endpoint and type, then reverting — cleangit diffbefore this PR.Also updates
CLAUDE.md,README.mdand ADR-0030's status metadata, since entities-off-the-wire stops being a review-only rule.The guard's limits are stated in its own doc comments and in ADR-0070's Consequences: it cannot see through an
IResult-typed return (three handlers today), anobject/dynamicreturn, or an anonymous type.Verdict: changes needed
Reproduced the author's proof independently: changed
GroupEndpoints.GetGroupto returnOk<PlaceMark.Domain.Groups.Group>— the test failed naming exactly that endpoint and type, reverted, clean tree. The reflection unwrap also already exercises real nested-generic shapes correctly (HealthEndpoints.GetReadinessisTask<Results<Ok, StatusCodeHttpResult>>and passes cleanly today).The "fails loudly rather than silently" claim is false for the commonest evasion — a lambda handler — and it's asserted in three places (
EndpointHandlers.csremarks,EntitiesOffTheWireTests.csremarks, ADR-0070 Consequences). I added a scratch file undersrc/PlaceMark.Api/Groups/following the project's own namespace/class convention, with one route mapped asscratch.MapGet("/{groupId}", async (Guid groupId, GroupRepository groups, CancellationToken ct) => Results.Ok(await groups.FindByIdAsync(groupId, ct)))— a domainGroupstraight onto the wire. All 34 tests passed, no exception, no violation reported.The reason:
HandlersDeclaredInonly throws whenMappedHandlerReferencefinds at least one bare-method-group match in a file but then fails to resolve the class or method. If a file's only.Map*calls are lambdas, the regex matches zero handlers,handlerNames.Length == 0, and the method returns[]before ever reaching the code path that would throw. Nothing distinguishes "this file maps nothing" from "this file maps something my regex can't parse."EndpointHandlers_Discovery_FindsAKnownHandlerdoesn't catch this either — it only provesGetPlaceByIdspecifically still resolves, not that a new lambda-shaped file elsewhere gets flagged.This is exactly the shape of prior overclaim this project has already had to walk back twice, and a lambda handler is arguably the single most idiomatic way to write a small minimal-API endpoint — not an exotic evasion. A future feature file written that way would be silently invisible to this guard while the docs assert the opposite.
Suggested fix: broaden
MappedHandlerReference(or add a second, looser pattern) to detect any.Map(Get|Post|Put|Delete|Patch)(call regardless of whether the second argument parses as a bare identifier, and throw inHandlersDeclaredInwhen such a call is found but no handler name could be extracted from it. That would make the existing "throws rather than skips" claim true instead of aspirational. At minimum, if that's out of scope for this PR, the three "fails loudly" claims need correcting to state the actual gap (a lambda-only file is silently invisible, full stop) rather than asserting a guarantee the code doesn't provide.Everything else checks out:
Results<T1,T2>,Task<>wrapping all unwrap correctly to leaves.ISomething<Place>) still yieldsPlaceas a leaf and gets flagged. Return-side interfaces are never exempted.ProjectReferencetoPlaceMark.Apiisn't checked by any existing rule (ProjectReferenceTests/ExternalReferenceTestsonly look atDomain/Contracts/Infrastructure/WebUI), so it's not a self-exemption hole — it's simply out of scope for those rules, which is correct.PlaceMark.Contracts.csprojis untouched and still has zero references (Contracts_ExternalReferences_AreNone/Contracts_ProjectReferences_AreNoneboth pass).README.mdandCLAUDE.mdno longer call this review-only, and both accurately name theIResult/object/dynamic/anonymous-type blind spots — just not the lambda-discovery one, per above.Fixed in
779f97a.EndpointHandlersnow reads eachMap...(call's own argument list (tracking nested()/[]/{}, skipping literals) and throws — naming the file and line — when the handler argument isn't a bare method group. Comments are blanked first (not stripped, so line numbers stay accurate), which was needed because the stricter check otherwise tripped on aMap...call quoted inside a//comment inPlaceEndpoints.cs.Re-ran your reproduction (lambda on
GroupEndpoints.GetGroupreturning a domainGroup): now fails with'src/PlaceMark.Api/Groups/GroupEndpoints.cs', line 98: ... handler argument is not a bare method group. Reverted after confirming, cleangit diff.Added
EndpointHandlersTests.csas a permanent regression test (reflects onResolveHandlerNamedirectly against synthetic source text, no fixture file needed) so this doesn't require a live endpoint to go wrong again to catch it.All three descriptions (
EndpointHandlers.cs,EntitiesOffTheWireTests.cs, ADR-0070) now say the same thing, and ADR-0070's Consequences records the correction and why it was wrong the first time.Verdict: changes needed
Re-reviewed at
779f97a. Reproduced my lambda case: it now throws, naming file and line correctly (ScratchLambdaEndpoints.cs, line 14, "not a bare method group"), and the whole suite is loud rather than silent. Also confirmed a comment containing an unbalanced bracket doesn't upset the depth tracking, and aMap...call split across lines still resolves and still catches a real violation. Good fix, correctly targeted.Attacking the new parser found one real gap and one doc inconsistency, both narrower than the original finding but the same category of problem.
Raw string literals (
"""...""") in aMap...call's argument list are misparsed, and this can throw on legitimate code with a misleading diagnosis.EndOfLiteralonly understands ordinary and verbatim (@"...") strings — its own doc comment says so explicitly and doesn't mention raw strings at all. A raw-string route pattern with no embedded quote happens to survive by accident (the triple-quote delimiter gets misread as a 2-char pseudo-literal immediately followed by a second literal that, by coincidence, still spans the real content). Add a literal"inside that raw string — a legitimate use case, since avoiding escaping is the entire reason to reach for one — and the boundary tracking breaks: I got a genuine bare-method-group handler (GetScratchGroup) rejected with "this reader could not check the endpoint... because its handler argument is not a bare method group — most likely a lambda", handler argument reported as''. That's exactly the "parser that throws on legitimate code, blocking the build for the wrong reason" the brief called out. No raw string literal appears in anyMap...call in the codebase today, so nothing is broken now, but the failure mode is real and the diagnosis is actively misleading. Suggest either extendingEndOfLiteralto recognise"""delimiters, or — cheaper — having it detect a raw string opener and throw a distinct, honest "raw string literals aren't handled here" error rather than falling through to the generic "not a bare method group" message.ADR-0070's Consequences omits the comment-blanking fix entirely and contains one now-false claim. Searched the ADR text for "comment" or "blank" — zero matches. The section that records "this was not the first version of that claim, and the correction is worth recording" covers only the lambda-silently-skipped bug; the second bug this same commit fixed (stripping vs. blanking comments, caught via the
PlaceEndpoints.csline describingplaces.MapGet("", ...)in prose) isn't mentioned anywhere in the ADR, despite the ADR's own stated purpose being to record exactly this kind of correction honestly. Separately, the same section's last sentence — "Nothing here regression-tests the lambda case as a permanent unit test, deliberately" — is contradicted byEndpointHandlersTests.ResolveHandlerName_LambdaHandler_ThrowsNamingTheFileAndLine, a permanent[Fact]added in this same commit that does exactly that (via synthetic source and reflection into the private method, sidestepping the fixture-file objection the ADR gives — but the sentence as written claims no such test exists at all, not "no fixture-file-based test exists"). Either narrow the sentence to what it actually argues against, or drop it now that the test exists.Minor, not blocking: the delegate-variable-handler case (
_handlerFieldpassed instead of a method group) does throw — confirmed — but viaMethodDeclaredBy's generic "compiled assembly and the working tree have diverged — rebuild PlaceMark.Api" message, which is the wrong diagnosis for this shape (rebuilding won't help).EndpointHandlersTestsalready documents this honestly in its own remarks, so it's a message-quality nit rather than a coverage gap.Everything else — split-across-lines, chained calls after the
Map...call, nested parens/literals in the pattern, comment-quoted false Map references — resolved or failed exactly as documented.Fixed in
a83f8d2.1. Raw string literals.
EndOfLiteralnow detects an opening run of 3+", and (via a newEndOfRawStringLiteral) terminates at the first run of at least that many quotes, consuming exactly the opening count — no escape processing, single- and multi-line both handled by the same scan since newlines are just ordinary characters to it. AddedQuoteRunLengthas a shared helper.Confirmed failing first: disabled the new branch, ran the two new
EndpointHandlersTestscases (single-line and multi-line raw string route pattern with an embedded quote, bare method group handler) — both threwInvalidOperationException: ... no matching ')' this reader could find(the embedded quote pairs with the opening run and leaves a literal scanning for a closing quote that never comes). Re-enabled the branch, both pass,GetGroupresolves correctly.2. ADR-0070. Corrected: Decision section now describes comment-blanking (previously undocumented) and states plainly that this is a hand-rolled partial C# lexer, names its complete scope (line/block comments, char/ordinary/verbatim/raw string literals, bracket nesting), and names the accepted limit (interpolation-hole contents). Consequences now records the raw-string correction alongside the lambda one, and the test count is corrected (13 supporting tests total, precisely enumerated — "nothing regression-tests the lambda case permanently" is gone;
EndpointHandlersTests.cshas 7 tests reflecting on the parser directly against synthetic text).Also fixed a naming bug I found while in there: a test named
..._ThrowsNamingTheFileAndLinewhose body actually assertedShould.NotThrow.On Roslyn: considered it and added an Alternatives-considered entry in ADR-0070 rather than switching. My judgement: the two corrections (comments, raw strings) closed the complete set of C# lexical forms that can hide a paren/comma/comment marker — nothing else introduces a new one at the language version this repo targets, and the accepted limit (interpolation holes) doesn't apply to any route pattern in this API. So I don't think the cost now outweighs the benefit; I'd revisit if a third lexical gap turns up, which is what the ADR says explicitly. Flagging this as my call, not a unilateral one — happy to switch if you'd rather not carry the hand-rolled scan at all.
Full CI sequence re-run clean: solution build (Release), full
dotnet test,dotnet format --verify-no-changes.Verdict: changes needed
Re-reviewed at
a83f8d2. Raw-string fix reproduced (disabling the raw-string branch fails both new raw-string tests with the exact "no matching ')'" symptom; re-enabling passes). ADR-0070's Consequences now covers both corrections honestly, and the "thirteen tests" count checks out (6 inEntitiesOffTheWireTests+ 7 inEndpointHandlersTests). The lambda-naming defect is fixed and no sibling test shares it — the two otherThrows/StillThrows-named tests genuinely assert throwing, the oneWithoutThrowing-named test genuinely doesn't.Priority 1, interpolation holes — the stated limit is real but understates its own danger, and doesn't degrade the way it's described.
EndOfLiteralreacts to any"it meets, regardless of whether it's inside a hole. Two distinct outcomes, both reproduced against a liveMap...call returning a domainGroupdirectly:value ?? "default") happens to resolve correctly and catches the violation. Not by design: the doc comment frames "a quote inside{ }" as uniformly misaligning the scan, but a balanced pair of embedded quotes toggles the literal-tracking state twice and lands back in sync by coincidence, the same shape of accident the raw-string bug was.{'"'}, acharliteral for a double-quote — valid, compiling C#) does not degrade gracefully.EndOfLiteralruns away hunting for a phantom closing quote through the rest of the file. In my single-call fixture it exhausted the file and threw"...has no matching ')' this reader could find"on entirely valid code — a wrong diagnosis for a build that isn't broken. In a real endpoints file, which always has more source after any oneMap...call, that runaway scan is just as free to resynchronise on some unrelated later quote and hand back a)that isn't the real one, producing a garbage argument list — I didn't pin an exact case where that silently resolves to a wrong-but-valid-looking handler name rather than throwing, but the mechanism plainly reaches that state, so it can't be ruled out the way the doc comment's "would misalign the scan" implies. This is worth being honest about rather than leaving as a single soft sentence.Priority 2 — the "complete lexical surface" claim is still false, twice more, neither shape related to interpolation:
Map...call inside a false#ifblock (symbol undefined) is found and resolved anyway — the reader has no concept of conditional compilation. Since the handler is compiled out,MethodDeclaredBythrows "the compiled assembly and the working tree have diverged — rebuild PlaceMark.Api", which is false and points at the wrong fix. Reproduced directly.GetGroupforGetGroup, valid and compiles to the same method) failsBareMethodGroup's^[A-Za-z_]\w*$and throws "not a bare method group... most likely a lambda" — also false. Minor on its own (nobody writes handler names this way), but it's a second independent gap found in the same pass./*inside a//line,//inside a quoted string) resolved correctly — no issue there.ADR-0070's own Alternatives section sets the bar for revisiting Roslyn at "a third lexical gap ... found" from here. This review pass alone found three (odd-quote interpolation,
#if-excluded code, Unicode escapes), on top of the two already spent on comments and raw strings. That bar has already been passed by the ADR's own criterion, before this PR has even merged — worth weighing against "no route pattern in this API is interpolated today" before treating this as closed.rob referenced this pull request2026-08-06 04:48:09 +00:00
Replaced the hand-rolled scanner with Roslyn in
4833e18.Confirmed all four gaps against the old scanner before replacing it:
$"{Escape('"')}") — threwno matching ')', file exhausted.value ?? "default") — passed by luck, as you said; pinned as a regression test regardless.#if-excludedMap...call (reproduced live onGroupEndpoints.GetGroup, reverted after) — resolved anyway, threw the false "rebuild PlaceMark.Api" diagnosis.GetGroup) — rejected as "not a bare method group".EndpointHandlers.csnow parses each file withCSharpSyntaxTree.ParseText, walksInvocationExpressionSyntaxnodes for the fiveMap*names, and requires the handler argument to be anIdentifierNameSyntax—Identifier.ValueTextgives the Unicode-decoded name for free.EndOfLiteral,StripComments, the bracket/comma scanners and both regexes are deleted, not left beside the new code.#ifhandling is explicit: parses withpreprocessorSymbols: ["TRACE"]— Release's symbol, matching what CI builds and ships — stated as a decision in both the class doc comment and ADR-0070, not inherited as a default. Verified both directions (#if TRACEfound,#if PLACEMARK_NEVER_DEFINEDexcluded) as permanent tests, using the same_parseOptionsinstance via reflection so they prove the real decision rather than a copy of it.Dependency:
Microsoft.CodeAnalysis.CSharp5.6.0, current perdotnet package search, added viadotnet add package, restores and builds clean against the pinned SDK (10.0.100).ADR-0070 rewritten (renamed to
...-with-roslyn.md, all cross-references updated) with a History section recording the three-round arc honestly, and your point about the dependency cost being paid on an instalment plan (three rounds of review time and false-positive risk) rather than once up front, in Consequences.Tests:
EntitiesOffTheWireTestsunchanged (7 tests).EndpointHandlersTestsnow has 12 — every prior case kept, plus the four new ones, plus the#iftrue/false pair.Full CI sequence re-run clean: solution build (Release), full
dotnet test,dotnet format --verify-no-changes. Live end-to-end reproduction re-verified against the new implementation (GetPlaceById→Ok<Place>, failed correctly, reverted, clean diff).Verdict: mergeable
Re-reviewed at
4833e18. All prior findings addressed:EndOfLiteral,StripComments, bracket-tracking and the regex-basedMapCallOpener/BareMethodGroupare gone outright, not left beside the parser (verified by grep — only doc-comment prose mentions "hand-rolled" now).#if-excludedMap...call, and Unicode-escaped identifier all resolve/exclude correctly, backed by permanent tests (EndpointHandlersTests, 12 cases). MutatedResolveHandlerNameto useIdentifier.Textinstead ofIdentifier.ValueText, and the parse options to dropTRACE— both mutations correctly failed the relevant new test, confirming they discriminate rather than passing vacuously.TRACE-only parsing matches what actually ships:Directory.Build.props/PlaceMark.Api.csprojdefine noDefineConstants, so SDK defaults apply — Release (what CI builds and tests) definesTRACEand notDEBUG. A#if DEBUG-gated handler is correctly invisible to both the compiled artefact and this reader; ADR-0070's Consequences states this plainly rather than implying a false completeness.Microsoft.CodeAnalysis.CSharpand the newProjectReferencetoPlaceMark.Apiare confined toPlaceMark.Architecture.Tests.csproj— no shipping project references either.ExternalReferenceTests/ProjectReferenceTestsdon't checkPlaceMark.Apior the test project itself, which is correctly out of scope, not a hole....-by-reflection.md→...-with-roslyn.md) is consistently cross-referenced inREADME.md,CLAUDE.md,docs/adr/README.mdand ADR-0030 — no stale link to the old filename anywhere.CLAUDE.md's "entities-off-the-wire … is still a review rule" line is updated and now accurately names the enforcement mechanism.dotnet build --configuration Release) andPlaceMark.Architecture.Tests(46/46) pass clean under the pinned SDK (10.0.100);dotnet format --verify-no-changesis clean; CI run #384 on this head commit succeeded.Nothing further to add over the three prior rounds.
Verdict: mergeable
Re-reviewed at
4833e18. All prior findings addressed:EndOfLiteral,StripComments, bracket-tracking and the regex-basedMapCallOpener/BareMethodGroupare gone outright, not left beside the parser (verified by grep — only doc-comment prose mentions "hand-rolled" now).#if-excludedMap...call, and Unicode-escaped identifier all resolve/exclude correctly, backed by permanent tests (EndpointHandlersTests, 12 cases). MutatedResolveHandlerNameto useIdentifier.Textinstead ofIdentifier.ValueText, and the parse options to dropTRACE— both mutations correctly failed the relevant new test, confirming they discriminate rather than passing vacuously.TRACE-only parsing matches what actually ships:Directory.Build.props/PlaceMark.Api.csprojdefine noDefineConstants, so SDK defaults apply — Release (what CI builds and tests) definesTRACEand notDEBUG. A#if DEBUG-gated handler is correctly invisible to both the compiled artefact and this reader; ADR-0070's Consequences states this plainly rather than implying a false completeness.Microsoft.CodeAnalysis.CSharpand the newProjectReferencetoPlaceMark.Apiare confined toPlaceMark.Architecture.Tests.csproj— no shipping project references either.ExternalReferenceTests/ProjectReferenceTestsdon't checkPlaceMark.Apior the test project itself, which is correctly out of scope, not a hole....-by-reflection.md→...-with-roslyn.md) is consistently cross-referenced inREADME.md,CLAUDE.md,docs/adr/README.mdand ADR-0030 — no stale link to the old filename anywhere.CLAUDE.md's "entities-off-the-wire … is still a review rule" line is updated and now accurately names the enforcement mechanism.dotnet build --configuration Release) andPlaceMark.Architecture.Tests(46/46) pass clean under the pinned SDK (10.0.100);dotnet format --verify-no-changesis clean; CI run #384 on this head commit succeeded.Nothing further to add over the three prior rounds.