[AsParameters]: reject unparseable values in collection query parameters (GH-3398)

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 <Badge type="tip" text="6.18" />
+
+The flag covers **collection** query string parameters (`T[]`, `List<T>`, `IList<T>`,
+`IReadOnlyList<T>`, `IEnumerable<T>`) 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

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

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<IScenarioResult> expectCollection400(Action<Alba.Scenario> 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<StrictCollectionQueryModel>();
+        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<StrictCollectionQueryModel>();
+        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<StrictCollectionQueryModel>();
+        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<StrictCollectionQueryModel>();
+        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<int> 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();
+    }
 }

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);
+    }
 }

src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs

@@ -21,14 +21,48 @@ public interface IGeneratesCode
     void GenerateCode(GeneratedMethod method, ISourceWriter writer);
 }
 
+/// <summary>
+/// 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.
+/// </summary>
+internal static class StrictQueryBinding
+{
+    /// <summary>
+    /// 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).
+    /// </summary>
+    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
         }
 

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);
             }

src/Http/Wolverine.Http/WolverineHttpOptions.cs

@@ -206,9 +206,12 @@ public void UseApiVersioning(Action<WolverineApiVersioningOptions> configure)
     /// This matches the behavior of ASP.NET Core minimal API parameter binding. A <b>missing</b>
     /// 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&lt;T&gt;, IEnumerable&lt;T&gt;, 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
     /// </summary>
     public bool RejectUnparseableQueryValues { get; set; }