Validate requests with data annotations (task 59) #21
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/request-validation"
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?
Validation rules are data-annotation attributes on the contracts, read by our own endpoint filter, so failures flow through
ApiExceptionHandlerand there is one 400 shape.System.ComponentModel.Annotationsis in the base framework, soPlaceMark.Contractsstays a leaf (ADR-0009) and the front end can enforce the same rules.errorskeys are the wire names, resolved through the sameJsonSerializerOptionsthe request was bound with. Recorded in ADR-0025.Microsoft.Extensions.Validationwas built and rejected on three measured grounds: its filter runs outside application filters, so failures cannot reach our handler; itsAccept: text/htmlbody dropsstatusandtraceId, against ADR-0022; and its generator discovers only public types, so aninternalrequest record silently accepted invalid input.Program.csgains three lines:RouteHandlerOptions.ThrowOnBadRequestin every environment,ApiExceptionHandlerclaimingBadHttpRequestExceptionwith a 4xx, andRequestCharsetMiddleware. The option defaults toIsDevelopment(), so the same malformed body previously produced different answers per environment, and with the option on but no handler branch the middleware answers 500 rather than reading the exception's status.An unresolvable charset is refused before routing, by middleware, not by interpreting an exception afterwards. An earlier revision did the latter and review found it masked faults: on a bodyless endpoint the header is never inspected, so the endpoint ran, faulted, and the branch then read the header — turning every
InvalidOperationExceptioninto a 400 and erasing it from the error log.RefusalStatusCodeno longer takes anHttpContextat all, so it cannot consult the request even by accident.The log names fields, never values:
JsonException.Pathis logged rather than the message, which quotes the offending literal verbatim.Needs a decision:
MapGroup; a group that omits it is silently unvalidated. Making it global needsapp.MapGroup("").Validator.TryValidateObjectis shallow — a nested contract would be waved through. v1's contracts are flat; the method carries the warning.A second 400 shape the ADR does not admit.
POST {"name":"ok","latitude":"nope"}returns 400 with no body, noContent-Typeand notraceId— minimal-API binding short-circuits before any filter, sinceRouteHandlerOptions.ThrowOnBadRequestis false. ADR-0025's "one 400, one place that writes it" is therefore false for the commonest malformed request. Either setThrowOnBadRequestso it reachesApiExceptionHandler, or amend the ADR and raise a follow-up.errorskeys are camel-cased CLR names, not what the caller sent. With[JsonPropertyName("place_name")]a caller sendingplace_namegets"name"back.RequestValidator's own remark admits this; the ADR and PR body assert the opposite. Resolve the wire name through the serialiser options and cover it with a renamed-property test.Nits: an
IValidatableObjectcross-field failure lands under key""; a refusal logs no field names.The flagged gaps (per-group opt-in, shallow
TryValidateObject,__EndpointRouteBuilder) are acceptable as recorded.Verdict: changes required
Two things the record claimed were not true of the code. A body the framework cannot bind — {"latitude":"nope"} — was answered with a bare 400: no body, no Content-Type, no traceId, and nothing in the log but the request line. Minimal API binding short-circuits before any endpoint filter, so ADR-0025's "one 400, one place that writes it" was false for the commonest malformed request there is. Worse, RouteHandlerOptions.ThrowOnBadRequest defaults to IsDevelopment(), so the shape differed between environments — the defect ADR-0022 exists to prevent. Program.cs now sets it in every environment, and ApiExceptionHandler claims BadHttpRequestException alongside ApiException: the exception's own status, and a constant detail. The framework's message names the parameter and the type behind it, so it goes to the log rather than to the caller, and the exception joins ApiException on SuppressDiagnosticsCallback — a caller sending an unreadable body is not the server failing, and could otherwise fill the log with Error records at will. An unsupported Content-Type is still a bare 415, measured: the media-type check short-circuits without throwing, so there is nothing for the handler to claim. It is recorded under what this does not cover rather than glossed, with a follow-up written up on ticket #59. The errors keys were camel-cased CLR names, not the names the caller sent: a contract with [JsonPropertyName("place_name")] answered a caller who sent place_name with "name", which is unmatchable. The wire name is now asked of the serialiser — JsonSerializerOptions.GetTypeInfo, from the same options the request was bound with — so the naming policy, JsonPropertyName and any custom resolver all reach the answer by one route. The example contract renames a property so the pipeline tests exercise it end to end. Also, from the review's non-blocking notes: a cross-field failure from IValidatableObject is keyed by the empty string, which is where ASP.NET Core's own model state puts a message about the request rather than one of its fields, and it now has a test; and a refusal logs the field names at Information, so a support question about someone else's 400 is answerable from the records. The names come from the contract, not from the request, so nothing the caller sent is written down.Actioned at
21b9dca. Both findings fixed rather than caveated, both nits fixed, description corrected. Detail is in ADR-0025; the remaining 415 boundary is task 138.413does not come out of the handler. Under real Kestrel withMaxRequestBodySizeexceeded — content-length and chunked alike — the answer is a bare413withContent-Length: 0, noContent-Type, notraceIdand noApiExceptionHandlerrecord; a probe middleware insideUseExceptionHandlersees no exception at all, because the binder sets the status and short-circuits exactly as the 415 does. SoApiExceptionHandler.Describe's "413 for one that is too large — so its choice is kept" and ADR-0025's "413for one too large" are both false, and 413 is a third bare-status shape the record does not name while claiming the exceptions are named rather than glossed. Drop the claim from both, and put 413 on task 138 beside the 415.The binder's reason does not reach the log. The record carries only
Failed to read parameter "ExampleRequest request" from the request body as JSON.TheJsonExceptioninner — the part that saysPath: $.latitude— is dropped, and ADR-0022 suppresses the middleware's Error record, so nothing anywhere says which field failed. That is precisely the support question the record uses to justify logging field names on the validation path, and the response withholds on the promise the log has it. Pass the exception toLogUnreadableRequest; its inner message names the JSON path, not the value.A
BadHttpRequestExceptioncarrying a 5xx — public constructor, so application code can produce one — is answered500withdetail: "The request could not be read."and logged only atInformationwith no stack trace. Gate both the branch and the suppression predicate on a 4xx status so a 5xx falls through to the generic 500 and is logged as a fault.Nit:
ToWireName's remark says a member the contract marksJsonIgnore"is reported as the rule named it" — it is not; such properties are still inJsonTypeInfo.Properties, so[JsonIgnore] string SecretValuekeys assecretValue. The?? memberNamefallback fires only for a name that is not a property at all, and nothing tests it.Verdict: changes required
Actioned at
7e040e9. All three fixed and the nit corrected; the 413 measurement reproduced independently under Kestrel, and 413 is now recorded beside the 415 against task 138.A fourth case the narrowed claim still misses, and it is a caller-triggerable 500.
Content-Type: application/json; charset=zzzon/tests/validatedthrowsInvalidOperationException: Unable to read the request as JSON because the request content type charset 'zzz' is not a known encodingout of the sameReadFromJsonAsync.ApiExceptionHandlerdeclines it, so one malformed header token from an unauthenticated client is answered500and logged atErrorwith a stack trace — the log-flooding vector the suppression rationale two paragraphs above disowns. It reaches the application as an exception and is not written byApiExceptionHandler, so "every failure that reaches the application as an exception is written byApiExceptionHandler" is still false. Name it beside the 415 and the 413, and put it on #138.The inner
JsonExceptioncarries payload text, not only the path.{"place_name":"ok","latitude": nope}logs'nope}' is an invalid JSON literal, and a body ofnotjson-SECRETVALUElogs that token in full —Utf8JsonReaderquotes the offending literal verbatim whenever it startsn,torf, and the first byte otherwise. That falsifies "nothing the caller sent is written down" in the consequence this commit extended, and cuts againstProgram.cs's stated reason for keeping bodies out of the log. Log(unreadableRequest.InnerException as JsonException)?.Path, which is the thing the record actually asks for, rather than the whole exception.Verdict: changes required
Actioned at
ccfaf14. The charset case is claimed and answered 400 rather than ticketed — matched on the request's unresolvable charset, not on the exception type — and the record now logsJsonException.Pathinstead of the exception, so no payload text reaches the log.The charset branch masks application faults, and the safety argument does not hold. "No application code has had the chance to fail" is only true of an endpoint that binds a JSON body; one that does not never inspects the charset, so the header sits inert until the branch reads it after the fault. Measured on
POST /tests/leavemapped as() => Array.Empty<int>().Single()— a bodyless write endpoint, the shape of "leave this group" or "accept invitation", faulting the way Dapper and LINQ fault:Content-Type: application/json→500,Errorwith the exception.Content-Type: application/json; charset=zzz→400 "The request could not be read.",Information, noErrorrecord at all.One header token on any request carrying a body turns every
InvalidOperationExceptionin the API into a client refusal and deletes it from the error log — the inverse of the vector this branch was added to close.Send_InvalidOperationExceptionFromAnOrdinaryRequest_IsStillAFaultpasses only because it sends noContent-Type. Reject the unresolvable charset before routing — a few lines of middleware, or the existing filter — so nothing has to be inferred from an exception after the fact and no fault can be reached by the branch at all.The closing claim is still overstated: a caller whose
Acceptrules out JSON gets the status and an empty body, whichPost_UnbindableRequestFromAClientRefusingJson_RespondsWith400AndAnEmptyBodyasserts. "Answered with a Problem Details body" needs that carve-out, which ADR-0022 already names."Nothing that could be a value reaches the log by this route" is unconditional, but
JsonException.Pathputs a caller-chosen dictionary key in it —$.tags.attackerKey, measured on aDictionary<string, double>property. No v1 contract has one; the sentence needs the qualifier rather than the absolute.Verdict: changes required
The charset branch added last round masks application faults, and the argument that justified it was wrong. "The charset is read before a byte of the body" is a property of endpoints that bind a JSON body, not of requests. An endpoint taking no body — leave a group, accept an invitation — never inspects Content-Type, so the header sits inert, the endpoint runs and can fault on its own, and the branch then reads the header after the fact. Measured on a bodyless endpoint failing the way a Single() over an empty result set does: application/json → 500, Error with the exception application/json; charset=zzz → 400 "could not be read", Information, no error record at all So one header token on any request turned every InvalidOperationException in the API into a client refusal and erased it from the error log, which is the inverse of the vector the branch was added to close. The previous test missed it only because it sent no Content-Type. The fix is structural. Any decision taken after the exception has to guess whether the framework or the application threw it, and InvalidOperationException carries nothing to guess with, so the guess is removed rather than improved. RequestCharsetMiddleware refuses the header before routing, throwing ValidationFailedException like any other refusal, and RefusalStatusCode has no InvalidOperationException arm and no HttpContext parameter — it cannot consult the request even by accident. Refusing on the way in also answers better than the branch could: a detail naming the header the caller can fix, rather than the constant "The request could not be read." Two tests hold it: the bodyless endpoint is not reached when the charset is unreadable, asserted through the endpoint rather than the status code because a fault dressed as a refusal looks identical from outside; and the same endpoint on an ordinary request still faults, answers 500 and is logged as an error. Two overstatements in the ADR go with it. "Answered with a Problem Details body" now carries ADR-0022's carve-out, since a caller whose Accept rules out JSON gets the status and an empty body. "Nothing that could be a value reaches the log" is now qualified by the contracts v1 has: a Dictionary<string, T> property would put a caller-chosen key in JsonException.Path.Actioned at
9892029(mainmerged in, ADRs back in numeric order). Reproduced the masking on a bodyless endpoint, then removed the guess rather than improving it:RequestCharsetMiddlewarerefuses the header before routing, andRefusalStatusCodeno longer takes anHttpContextat all. Both ADR overstatements corrected.Nothing to act on.
Verdict: mergeable