[AsParameters]: reject unparseable values in collection query parameters (GH-3398)#3402
Closed
jeremydmiller wants to merge 2 commits into
Closed
[AsParameters]: reject unparseable values in collection query parameters (GH-3398)#3402jeremydmiller wants to merge 2 commits into
jeremydmiller wants to merge 2 commits into
Conversation
A header-identified saga cannot work over a gRPC hop: the saga-id envelope header is not propagated across the call, so no id can be resolved. That is unchanged and is not what this fixes. What it fixes is the diagnostic. The caller used to get StatusCode.Internal with "Could not determine a valid saga state id for Envelope ..." -- true, but it names no cause and no remedy, and the remedy is not guessable, because the reason the id is missing is a property of the transport rather than of the message. It now maps to StatusCode.InvalidArgument with a detail that says the identity must be on the message body, why (the header does not cross the hop), how to supply it ([SagaIdentity] or a matching property), and where to read more. InvalidArgument rather than Internal is deliberate per AIP-193: the request cannot succeed as sent and retrying will not change that -- it is a caller-side contract problem, not a transient server fault. The mapping lives in WolverineGrpcExceptionMapper, which only runs at the gRPC boundary, so the core exception is untouched and non-gRPC users see no change. Message-identified sagas over gRPC already work and are unaffected (covered by #3386). Per @erikshafer's analysis on the issue, full three-point saga-id propagation is deliberately NOT taken: HTTP does not exercise the header path either, so "works like HTTP" is already satisfied, and the complexity is not warranted until someone shows a concrete need. Flips the #3386 characterization test, and updates the gRPC + Sagas docs page. Full Wolverine.Grpc.Tests 273/273. Full wolverine.slnx Release build clean. Closes #3385. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ers (GH-3398) GH-3372 made a present-but-unparseable *scalar* query string value short circuit with a 400 ProblemDetails under the opt-in WolverineHttpOptions.RejectUnparseableQueryValues. Collections were left as a noted follow-up, and they are the worse failure mode: an unparseable element was silently dropped, taking an optional filter collection to null (or to a partial list), so a filtering endpoint answered 200 with an *unfiltered* result set while the caller believed the results were filtered. FluentValidation cannot compensate because it only ever sees null. ParsedArrayQueryStringValue (T[]) and ParsedCollectionQueryStringValue (List<T>/IList<T>/IReadOnlyList<T>/IEnumerable<T>) now take the same rejectUnparseableValue flag as ReadHttpFrame and emit a shared rejection block: a present but unparseable element writes the 400 ProblemDetails naming the parameter and exits. Binding is all-or-nothing -- one bad element rejects the whole collection rather than silently keeping the parseable ones. An omitted parameter still keeps its initializer in both modes, string collections are untouched, and the flag remains opt-in (default false) until the 7.0 flip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Superseded by a clean branch — this one was accidentally cut on top of the GH-3385 branch in a shared working tree, so its diff carried three unrelated gRPC files. Same commit, rebased onto main. Reopening; no review time lost. |
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.
Fixes #3398. Completes the job started in #3372 (shipped in 6.17.1/6.17.2), which explicitly left collection query binding as a follow-up.
Why this one is worse than a plain 400
An unparseable element of a multi-valued query string parameter was silently dropped by the generated binder. On a filtering endpoint the bound collection is typically an optional predicate:
So
?colours=Purple(typo) did not fail the request — it silently removed the filter and returned an unfiltered200while the caller believed the results were filtered. That is a silently-wrong-answer failure mode, not a loud one, and it is unrecoverable downstream:nullis indistinguishable from "parameter omitted", so FluentValidation'sRuleForEachhas nothing to iterate andNotEmpty().When(x => x.Colours is not null)never fires. Applies toint[]/Guid[]/long[]as well as enums.The fix
ParsedArrayQueryStringValue(T[]) andParsedCollectionQueryStringValue(List<T>,IList<T>,IReadOnlyList<T>,IEnumerable<T>) now accept the samerejectUnparseableValueflagReadHttpFramegot in #3372, wired fromWolverineHttpOptions.RejectUnparseableQueryValuesthroughHttpChain. Both emit a sharedStrictQueryBinding.WriteRejectionBlock: an element that is present but failsTryParsewrites a 400 ProblemDetails naming the parameter (via the existingHttpHandler.WriteQueryValueParsingProblem) and exits the generated method.Semantics match the scalar case exactly:
?Colours=Red&Colours=Blue200—[Red, Blue]200—[Red, Blue]200— keeps initializer200— keeps initializer?Colours=Purple200—null400ProblemDetails namingColours?Colours=Red&Colours=Purple200—[Red]400ProblemDetails namingColoursThe default is not changed; the flag stays opt-in through 6.x and flips to strict in 7.0. String collections are untouched (nothing to parse). Form/route/header binding untouched.
Decision: all-or-nothing binding
?colours=Red&colours=Purplerejects the whole binding rather than binding[Red]and droppingPurple. ASP.NET Core minimal APIs reject the request when any element of a bound collection parameter fails to parse (the binding failure is recorded and the pipeline returns 400) rather than yielding a partial collection, so this is also the minimal-API-compatible answer that #3372 set out to match. Independently it is the only defensible option: a partially-bound collection is exactly the silently-wrong-answer this issue is about — the caller asked to filter on two colours and would get results filtered on one, with a200and no signal. Silently nulling the whole collection is worse still. If the caller genuinely wants lenient element dropping, that is what the (default) flag-off behavior gives them.Red → green evidence
Tests added to
src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs(strict + lenient hosts already in that fixture): enum array,int[],Guid[],List<int>, a directint[]method argument, the mixed valid+invalid case, valid-values-bind, omitted-parameter-keeps-initializer, and two flag-off lenience tests.Before the fix — exactly the six strict-mode assertions fail, and the lenient/omitted expectations already pass (proving the behavior change is confined to the flag):
After:
Full
Wolverine.Http.Testssuite on net9.0: 844 passed, 0 failed, 10 skipped.dotnet build wolverine.slnx -c Release: 0 warnings, 0 errors.Docs:
docs/guide/http/as-parameters.mdgains a "Collections" section under Strict Query String Binding with the behavior matrix and the all-or-nothing rationale;docs/guide/http/querystring.mdnotes collection coverage.Thanks to @outofrange-consulting for the report (and for the scalar case in #3372).
🤖 Generated with Claude Code