diff --git a/docs/guide/grpc/sagas.md b/docs/guide/grpc/sagas.md index c077d0eb9..b26fca378 100644 --- a/docs/guide/grpc/sagas.md +++ b/docs/guide/grpc/sagas.md @@ -176,13 +176,24 @@ Wolverine has two ways to resolve which saga state a message belongs to: 2. **Header-identified** — the message carries no identity member, and Wolverine falls back to the envelope's `saga-id` header. -**Only message-identified sagas are supported over a gRPC service hop today.** The `saga-id` -envelope header does not cross the hop — neither the client nor server propagation interceptor -carries it, so a header-identified saga invoked through a gRPC service fails with an -`IndeterminateSagaStateIdException` (surfaced to the caller as a `StatusCode.Internal` -`RpcException`). This is tracked in -[GH-3385](https://github.com/JasperFx/wolverine/issues/3385); the practical guidance is simple and -is the better design anyway: +**Only message-identified sagas are supported over a gRPC service hop.** The `saga-id` envelope +header does not cross the hop — neither the client nor server propagation interceptor carries it — +so a header-identified saga invoked through a gRPC service cannot resolve an id at all. + +As of 6.18.0 that fails with an explicit diagnostic rather than an opaque one: the caller gets a +`StatusCode.InvalidArgument` `RpcException` whose detail names both the cause and the fix. + +> Could not determine a saga id for this request. A saga started or continued over a gRPC hop must +> carry its identity ON THE MESSAGE BODY: the 'saga-id' envelope header is not propagated across a +> gRPC call, so a header-identified saga cannot work over gRPC. Put the saga identity on the request +> message itself (a property Wolverine can match to the saga id, or one marked with +> `[SagaIdentity]`). + +`InvalidArgument` rather than `Internal` is deliberate ([AIP-193](https://google.aip.dev/193)): the +request cannot succeed as sent, and no amount of retrying will change that — it is a contract +problem, not a transient server fault. + +The practical guidance is simple, and is the better design anyway: ::: tip Put the saga identity on the request DTO. It makes the contract self-describing for non-Wolverine diff --git a/docs/guide/http/as-parameters.md b/docs/guide/http/as-parameters.md index a11ffb343..816acb9f1 100644 --- a/docs/guide/http/as-parameters.md +++ b/docs/guide/http/as-parameters.md @@ -253,6 +253,28 @@ member's default / initializer in both modes. The flag applies to query string b `[AsParameters]` members and for endpoint method arguments bound from the query string alike. Route argument binding is unaffected (an unparseable route value already returns `404`). +### Collections + +The flag covers **collection** query string parameters (`T[]`, `List`, `IList`, +`IReadOnlyList`, `IEnumerable`) too. With the flag off, an element that fails to parse is +silently dropped, which is especially dangerous for an optional filter: the endpoint sees `null` +(or a partial collection) and quietly returns an *unfiltered* `200` while the caller believes the +results were filtered. + +With the flag on, binding a collection is **all or nothing** — a single unparseable element +rejects the whole request with a `400` naming the parameter, rather than silently dropping the bad +element and keeping the good ones. For +`[FromQuery] public Colour[]? Colours { get; set; }` on `GET /widgets`: + +| Request | Flag off (default) | Flag on | +| --- | --- | --- | +| `GET /widgets?Colours=Red&Colours=Blue` | `200` — binds `[Red, Blue]` | `200` — binds `[Red, Blue]` | +| `GET /widgets` (missing) | `200` — keeps the initializer (`null`) | `200` — keeps the initializer (`null`) | +| `GET /widgets?Colours=Purple` | `200` — `Colours` is `null` | `400` — ProblemDetails naming `Colours` | +| `GET /widgets?Colours=Red&Colours=Purple` | `200` — binds only `[Red]` | `400` — ProblemDetails naming `Colours` | + +String collections are unaffected in both modes, as there is nothing to parse. + ::: warning `RejectUnparseableQueryValues` is opt-in (defaults to `false`) throughout Wolverine 6.x to preserve the previous lenient behavior, but the default flips to `true` (strict) in Wolverine 7.0. If you diff --git a/docs/guide/http/querystring.md b/docs/guide/http/querystring.md index bca081623..b195707eb 100644 --- a/docs/guide/http/querystring.md +++ b/docs/guide/http/querystring.md @@ -7,8 +7,9 @@ cannot be parsed -- the value passed to your method will be the `default` for wh type is. If you want a *present but unparseable* query string value to return a `400 Bad Request` instead (matching ASP.NET Core minimal APIs), opt into `WolverineHttpOptions.RejectUnparseableQueryValues` — see -[Strict Query String Binding](./as-parameters#strict-query-string-binding). The strict behavior -becomes the default in Wolverine 7.0. +[Strict Query String Binding](./as-parameters#strict-query-string-binding). That flag also covers +collection query string parameters, where an unparseable element otherwise gets silently dropped. +The strict behavior becomes the default in Wolverine 7.0. ::: Wolverine supports passing query string values to your HTTP method arguments for diff --git a/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs b/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs index 73d7dde17..b7881f304 100644 --- a/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs +++ b/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs @@ -247,6 +247,148 @@ public async Task missing_values_keep_initializers_when_flag_is_off() response.PageSize.ShouldBe(5); response.Token.ShouldBe(StrictQueryModel.DefaultToken); } + + // GH-3398: the collection case. An unparseable *element* of a multi-valued query string + // parameter was silently dropped, taking the whole collection to null/empty. That is worse + // than a plain 400: an optional filter predicate silently disappears and the endpoint answers + // 200 with an unfiltered result set. + + private Task expectCollection400(Action configure) + { + return _fixture.StrictHost.Scenario(x => + { + configure(x); + x.StatusCodeShouldBe(400); + x.ContentTypeShouldBe("application/problem+json"); + }); + } + + [Fact] + public async Task malformed_enum_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Colours", "Purple")); + + var text = await result.ReadAsTextAsync(); + text.ShouldContain("Colours"); + text.ShouldContain("Purple"); + } + + [Fact] + public async Task one_bad_element_rejects_the_whole_collection() + { + // All-or-nothing: a valid element does not rescue the request. Silently keeping "Red" and + // dropping "Purple" would still be a silently wrong answer. + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Red&Colours=Purple"); + x.StatusCodeShouldBe(400); + x.ContentTypeShouldBe("application/problem+json"); + }); + + var text = await result.ReadAsTextAsync(); + text.ShouldContain("Colours"); + text.ShouldContain("Purple"); + } + + [Fact] + public async Task malformed_int_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Ids", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("Ids"); + } + + [Fact] + public async Task malformed_guid_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Tokens", "not-a-guid")); + + (await result.ReadAsTextAsync()).ShouldContain("Tokens"); + } + + [Fact] + public async Task malformed_generic_list_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Pages", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("Pages"); + } + + [Fact] + public async Task malformed_collection_element_on_direct_method_argument_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections/direct").QueryString("numbers", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("numbers"); + } + + [Fact] + public async Task valid_collection_values_bind_in_strict_mode() + { + var token = Guid.NewGuid(); + + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url($"/strict/collections?Colours=Red&Colours=Blue&Ids=1&Ids=2&Tokens={token}&Pages=3"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBe([StrictColour.Red, StrictColour.Blue]); + response.Ids.ShouldBe([1, 2]); + response.Tokens.ShouldBe([token]); + response.Pages.ShouldBe([3]); + } + + [Fact] + public async Task omitted_collections_keep_initializers_in_strict_mode() + { + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url("/strict/collections"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBeNull(); + response.Ids.ShouldBeNull(); + response.Tokens.ShouldBeNull(); + response.Pages.ShouldBeEmpty(); + } + + [Fact] + public async Task malformed_collection_values_are_lenient_when_flag_is_off() + { + // The pre-3398 lenient behavior is preserved by default in 6.x: bad elements are dropped + var result = await _fixture.LenientHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Purple&Ids=abc&Pages=abc"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBeNull(); + response.Ids.ShouldBeNull(); + response.Pages.ShouldBeEmpty(); + } + + [Fact] + public async Task partially_malformed_collection_values_are_lenient_when_flag_is_off() + { + var result = await _fixture.LenientHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Red&Colours=Purple"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBe([StrictColour.Red]); + } } public enum StrictSortOrder @@ -255,6 +397,28 @@ public enum StrictSortOrder Date = 2 } +public enum StrictColour +{ + Red, + Green, + Blue +} + +public class StrictCollectionQueryModel +{ + [FromQuery] + public StrictColour[]? Colours { get; set; } + + [FromQuery] + public int[]? Ids { get; set; } + + [FromQuery] + public Guid[]? Tokens { get; set; } + + [FromQuery] + public List Pages { get; set; } = []; +} + public class StrictQueryModel { public static readonly Guid DefaultToken = Guid.Parse("11111111-2222-3333-4444-555555555555"); @@ -291,4 +455,16 @@ public static string GetDirect(int number) { return number.ToString(); } + + [WolverineGet("/strict/collections")] + public static StrictCollectionQueryModel GetCollections([AsParameters] StrictCollectionQueryModel query) + { + return query; + } + + [WolverineGet("/strict/collections/direct")] + public static string GetCollectionDirect(int[] numbers) + { + return numbers.Length.ToString(); + } } diff --git a/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs b/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs index dc3ed912d..bb8f7cf67 100644 --- a/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs +++ b/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs @@ -8,10 +8,20 @@ namespace Wolverine.Http.CodeGen; internal class ParsedArrayQueryStringValue : SyncFrame, IReadHttpFrame { private string? _property; + private readonly bool _rejectUnparseableValue; - public ParsedArrayQueryStringValue(Type parameterType, string parameterName) + public ParsedArrayQueryStringValue(Type parameterType, string parameterName, bool rejectUnparseableValue = false) { Variable = new HttpElementVariable(parameterType, parameterName!, this); + + // GH-3398 strict query string binding for collections: only meaningful for parsed + // (non-string) elements. The 400 + ProblemDetails short circuit writes the response + // asynchronously, so the frame has to force the generated method into async mode. + _rejectUnparseableValue = rejectUnparseableValue && parameterType.GetElementType() != typeof(string); + if (_rejectUnparseableValue) + { + IsAsync = true; + } } public void AssignToProperty(string usage) @@ -63,8 +73,10 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) writer.Write($"{Variable.Usage}_List.Add({Variable.Usage}ValueParsed);"); writer.FinishBlock(); // parsing block + writeRejectionBlock(method, writer, $"{Variable.Usage}Value", elementAlias); + writer.FinishBlock(); // foreach blobck - + if (Mode == AssignMode.WriteToVariable) { writer.Write($"var {Variable.Usage} = {Variable.Usage}_List.ToArray();"); @@ -77,4 +89,15 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) Next?.GenerateCode(method, writer); } + + private void writeRejectionBlock(GeneratedMethod method, ISourceWriter writer, string valueUsage, + string elementAlias) + { + if (!_rejectUnparseableValue) + { + return; + } + + StrictQueryBinding.WriteRejectionBlock(method, writer, Variable.Name, valueUsage, elementAlias); + } } \ No newline at end of file diff --git a/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs b/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs index 72713b58e..505830f55 100644 --- a/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs +++ b/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs @@ -21,14 +21,48 @@ public interface IGeneratesCode void GenerateCode(GeneratedMethod method, ISourceWriter writer); } +/// +/// Shared emission for the GH-3372 / GH-3398 opt-in strict query string binding. A query string +/// value that is present but cannot be parsed short circuits the request with a 400 ProblemDetails +/// naming the offending parameter, matching ASP.NET Core minimal API binding. +/// +internal static class StrictQueryBinding +{ + /// + /// Emits the "else" arm of a collection element's TryParse: a present but unparseable element + /// rejects the entire binding. All-or-nothing is deliberate -- keeping the parseable elements + /// and dropping the bad one would still hand the endpoint a silently wrong filter (GH-3398). + /// + public static void WriteRejectionBlock(GeneratedMethod method, ISourceWriter writer, string parameterName, + string valueUsage, string elementAlias) + { + writer.Write($"BLOCK:else if (!string.IsNullOrEmpty({valueUsage}))"); + writer.Write( + $"await {nameof(HttpHandler.WriteQueryValueParsingProblem)}(httpContext, \"{parameterName}\", {valueUsage}, \"{elementAlias}\").ConfigureAwait(false);"); + writer.Write(method.ToExitStatement()); + writer.FinishBlock(); + } +} + internal class ParsedCollectionQueryStringValue : SyncFrame, IReadHttpFrame { private readonly Type _collectionElementType; + private readonly bool _rejectUnparseableValue; - public ParsedCollectionQueryStringValue(Type parameterType, string parameterName) + public ParsedCollectionQueryStringValue(Type parameterType, string parameterName, + bool rejectUnparseableValue = false) { Variable = new HttpElementVariable(parameterType, parameterName!, this); _collectionElementType = GetCollectionElementType(parameterType); + + // GH-3398 strict query string binding for collections: only meaningful for parsed + // (non-string) elements. The 400 + ProblemDetails short circuit writes the response + // asynchronously, so the frame has to force the generated method into async mode. + _rejectUnparseableValue = rejectUnparseableValue && _collectionElementType != typeof(string); + if (_rejectUnparseableValue) + { + IsAsync = true; + } } public HttpElementVariable Variable { get; } @@ -72,6 +106,12 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) writer.Write($"{Variable.Usage}.Add({Variable.Name}ValueParsed);"); writer.FinishBlock(); // parsing block + if (_rejectUnparseableValue) + { + StrictQueryBinding.WriteRejectionBlock(method, writer, Variable.Name, $"{Variable.Name}Value", + elementAlias); + } + writer.FinishBlock(); // foreach blobck } diff --git a/src/Http/Wolverine.Http/HttpChain.cs b/src/Http/Wolverine.Http/HttpChain.cs index 9e46036ba..96f3ff1ff 100644 --- a/src/Http/Wolverine.Http/HttpChain.cs +++ b/src/Http/Wolverine.Http/HttpChain.cs @@ -712,14 +712,16 @@ public bool FindQuerystringVariable(Type variableType, string routeOrParameterNa if (parameterType.IsArray && RouteParameterStrategy.CanParse(parameterType.GetElementType()!)) { - variable = new ParsedArrayQueryStringValue(parameterType, key).Variable; + variable = new ParsedArrayQueryStringValue(parameterType, key, + rejectUnparseableValue: _parent.RejectUnparseableQueryValues).Variable; variable.Name = key; _querystringVariables.Add(variable); } if (ParsedCollectionQueryStringValue.CanParse(parameterType)) { - variable = new ParsedCollectionQueryStringValue(parameterType, key).Variable; + variable = new ParsedCollectionQueryStringValue(parameterType, key, + rejectUnparseableValue: _parent.RejectUnparseableQueryValues).Variable; variable.Name = key; _querystringVariables.Add(variable); } diff --git a/src/Http/Wolverine.Http/WolverineHttpOptions.cs b/src/Http/Wolverine.Http/WolverineHttpOptions.cs index bd9941fe1..7a4c33d23 100644 --- a/src/Http/Wolverine.Http/WolverineHttpOptions.cs +++ b/src/Http/Wolverine.Http/WolverineHttpOptions.cs @@ -206,9 +206,12 @@ public void UseApiVersioning(Action configure) /// This matches the behavior of ASP.NET Core minimal API parameter binding. A missing /// query string value still binds the parameter's default / property initializer value in /// either mode. Applies to query string binding for endpoint method arguments and for - /// [AsParameters] members bound from the query string. This is currently opt-in + /// [AsParameters] members bound from the query string, including collection parameters + /// (arrays, List<T>, IEnumerable<T>, etc.) where a single unparseable element rejects the + /// entire binding. This is currently opt-in /// (default false) to preserve the previous lenient behavior, but the default flips /// to true (strict) in Wolverine 7.0. See https://github.com/JasperFx/wolverine/issues/3372 + /// and https://github.com/JasperFx/wolverine/issues/3398 /// public bool RejectUnparseableQueryValues { get; set; } diff --git a/src/Wolverine.Grpc.Tests/SagaOverGrpc/saga_over_grpc_tests.cs b/src/Wolverine.Grpc.Tests/SagaOverGrpc/saga_over_grpc_tests.cs index 94a37dc57..967ad10cd 100644 --- a/src/Wolverine.Grpc.Tests/SagaOverGrpc/saga_over_grpc_tests.cs +++ b/src/Wolverine.Grpc.Tests/SagaOverGrpc/saga_over_grpc_tests.cs @@ -60,17 +60,24 @@ public async Task can_start_and_continue_a_message_identified_saga_over_grpc() #endregion [Fact] - public async Task starting_a_header_identified_saga_over_grpc_fails_with_opaque_status_today() + public async Task starting_a_header_identified_saga_over_grpc_fails_with_an_actionable_diagnostic() { var client = _fixture.CreateClient(); var ex = await Should.ThrowAsync(async () => await client.Start(new StartCountingRequest { Label = "first" })); - // CHARACTERIZATION of current behavior (GH-3385). saga-id is not propagated across a gRPC - // hop, so PullSagaIdFromEnvelopeFrame throws IndeterminateSagaStateIdException, which maps to - // Internal with a message that doesn't tell the developer what to do about it. - ex.StatusCode.ShouldBe(StatusCode.Internal); - ex.Status.Detail.ShouldContain("saga state id"); + // GH-3385. A header-identified saga still cannot work over a gRPC hop — the 'saga-id' header + // is not propagated across the call, so no id can be resolved. That is a caller-side contract + // problem, not a transient server fault, so it is InvalidArgument (AIP-193) rather than the + // Internal it used to report... + ex.StatusCode.ShouldBe(StatusCode.InvalidArgument); + + // ...and the detail now names the cause AND the remedy, instead of the bare + // "Could not determine a valid saga state id for Envelope ..." that told the developer + // nothing they could act on. + ex.Status.Detail.ShouldContain("ON THE MESSAGE BODY"); + ex.Status.Detail.ShouldContain("[SagaIdentity]"); + ex.Status.Detail.ShouldContain("wolverinefx.net/guide/grpc/sagas"); } } diff --git a/src/Wolverine.Grpc/WolverineGrpcExceptionMapper.cs b/src/Wolverine.Grpc/WolverineGrpcExceptionMapper.cs index e863021a3..27ee0dd4a 100644 --- a/src/Wolverine.Grpc/WolverineGrpcExceptionMapper.cs +++ b/src/Wolverine.Grpc/WolverineGrpcExceptionMapper.cs @@ -1,4 +1,5 @@ using Grpc.Core; +using Wolverine.Persistence.Sagas; namespace Wolverine.Grpc; @@ -33,6 +34,14 @@ public static StatusCode Map(Exception exception) RpcException rpc => rpc.StatusCode, OperationCanceledException => StatusCode.Cancelled, TimeoutException => StatusCode.DeadlineExceeded, + + // GH-3385. The request reached a saga handler that identifies its saga from the envelope + // (a `saga-id` header or [SagaIdentity] on a header), and no saga id could be resolved. + // Over a gRPC hop that is not a transient server fault — it is a contract problem that + // the caller cannot fix by retrying, because the header never crosses the hop at all. + // InvalidArgument per AIP-193, with a detail that says what to do about it. + IndeterminateSagaStateIdException => StatusCode.InvalidArgument, + ArgumentException => StatusCode.InvalidArgument, KeyNotFoundException => StatusCode.NotFound, FileNotFoundException => StatusCode.NotFound, @@ -59,10 +68,25 @@ public static RpcException ToRpcException(Exception exception) return existing; } - var status = new Status(Map(exception), exception.Message); + var status = new Status(Map(exception), detailFor(exception)); return new RpcException(status); } + // GH-3385: the core exception's message ("Could not determine a valid saga state id for + // Envelope ...") is true but useless to someone calling a gRPC service — it names no cause and + // no remedy, and the remedy is not obvious, because the reason the id is missing is that the + // saga-id header does not cross a gRPC hop at all. Say that, and say what to do instead. + private static string detailFor(Exception exception) + { + if (exception is IndeterminateSagaStateIdException) + { + return + "Could not determine a saga id for this request. A saga started or continued over a gRPC hop must carry its identity ON THE MESSAGE BODY: the 'saga-id' envelope header is not propagated across a gRPC call, so a header-identified saga cannot work over gRPC. Put the saga identity on the request message itself (a property Wolverine can match to the saga id, or one marked with [SagaIdentity]). See https://wolverinefx.net/guide/grpc/sagas.html"; + } + + return exception.Message; + } + /// /// The inverse of — maps an incoming /// back to an idiomatic .NET exception type for client-side consumers. Applied by the