`string[]` bound to querystring in minimal API should return `400` when not present

[andrewlock]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

When you have a minimal API that takes a string[] (or StringValues) parameter which binds to the querystring, for example:

app.MapGet("/", (string[] q) => q);

calling the API without a querystring (e.g. /) should give a 400 response saying BadHttpRequestException: Required parameter "string[] q" was not provided from query string.

But this does not happen - instead q is an empty array. This differs from both of the following other examples:

app.MapGet("/", (string q) => q); //
app.MapGet("/", (StringValues q) => q); 

which both return a 400 response as expected.

Expected Behavior

When binding a required parameter in a minimal API, a value must be present in the binding source (typically the querystring when binding arrays), otherwise the API returns a 400. This is the documented behaviour which works for other parameter types.

Steps To Reproduce

To reproduce, create an api like this: app.MapGet("/", (string[] q) => q); and call the URL / (without a querystring). The response is [], where it should be a 400 Bad Request.

Exceptions (if any)

No response

.NET Version

7.0.101

Anything else?

I found this behaviour while reading some of the Expression generation code in RequestDelegateFactory. In particular CreateArgument() calls BindParameterFromValue which calls BindParameterFromExpression.

The argument expression generated by this code when binding a string[] parameter (called q in this example) to the querystring is similar to the following (some artisitic license):

Task Invoke(HttpContext httpContext)
{
    bool wasParamCheckFailure = false; // Added by RequestDelegateFactory.Create()

    string[] q_local = httpContext.Request.Query["q"] // 👈 This is the problem
    if (q_local == null) // 👈 Because this is never true.
    {
        wasParamCheckFailure = true;
        Log.RequiredParameterNotProvided(httpContext, "string[]", "q", "query");
    }

    if(wasParamCheckFailure) // Added by RequestDelegateFactory.Create()
    {
        httpContext.Response.StatusCode = 400;
        return Task.CompletedTask;
    }

    // .. call handler, handle response
}

I think the problem lies in the line above. Query["q"] returns StringValues.Empty when the item is not present, and is implicitly converted to a string[]. (string[])StringValues.Empty always returns an empty array, so is never null, so the check never succeeds.

[andrewlock]

I've noticed that this actually applies to all arrays of TryParse-ables too, not just string[], because the expression for int[] (for example) uses a string[] tempStringArray.

[captainsafia]

Triage: We had a fair bit of discussion about how we would handle these types of the StringValues/``string[] arguments in the RequestDelegateFactory. Some of this was captured in the discussions on #41789 (comment). For now, the distinction is intended behavior.

As an alternative to this, you could implement the check for StringValues.Empty on the argument in an endpoint filter.

cc: @halter73 for more on the background conversation around this

[halter73]

Starting in .NET 7 with #41789, StringValues behaves the way you want string[] to behave, so you can take a StringValues parameter instead and cast to string[] as needed as a workaround.

Here's our initial conversation about nullable arrays. #32516 (comment). I think the string[] behavior was intentional, and I'd be hesitant to break people expecting and getting an empty array in this case

Hi @andrewlock. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

[andrewlock]

Thanks for the context. I haven't been affected by this directly, so I'm not super passionate, it was just something I noticed when reading the code.

That said, it seems... odd... to have the inconsistency. As far as I can work out, this is now the only case where a nullable parameter can't ever actually be null.

The fact that (StringValues? myval) => { } gives myval as null, yet (string[]? myval) => { } won't be null is a weird disconnect IMO. Additionally, the StringValues? "trick" doesn't work if you're binding an optional array of TryParse-ables (like int[]?). The array can't ever be null here.

As for breaking people, I'd be surprised people would be marking arrays as explicitly nullable if they weren't ever expecting the parameter to be null 🤔

That said, I understand it could be a breaking change, in which case I'd suggest at least updating the documentation to highlight this exception to the rule?

[halter73]

That said, it seems... odd... to have the inconsistency. As far as I can work out, this is now the only case where a nullable parameter can't ever actually be null.

I think you're right that arrays are currently the only type of parameter that can never be null, but it's also the only supported parameter type other than StringValues that can already uniquely represent missing parameters without using null. If you were binding an array from the body, there's a difference between [] and an empty body that needs to be represented.

To me this is similar to the following in DI where dependency will never be null:

public MyService(IEnumerable<MyMissingDependency> dependency = null)

The fact that (StringValues? myval) => { } gives myval as null, yet (string[]? myval) => { } won't be null is a weird disconnect IMO.

I completely agree this is a weird disconnect. In a world where more people were aware of the existence of StringValues.Empty and redundancy of Nullable<StringValues>, I would have liked to use StringValues.Empty instead of null here as well. I don't think empty arrays are nearly as obscure.

As for breaking people, I'd be surprised people would be marking arrays as explicitly nullable if they weren't ever expecting the parameter to be null 🤔

I'm not as worried about passing null into a string[]? parameter. If someone went out of their way to make the parameter nullable, passing in null seems reasonable to me. After all, the original proposal from @davidfowl in #32516 (comment) was, "If there are no values we pass an empty array unless the user makes the array nullable."

However, going from calling the route handler with an empty array to short-circuiting with a 400 in the non-nullable string[]/int[]/whatever[] case does seem very risky. And then if we're going to pass in an empty array in the non-nullable case, passing in null for the exact same request in the nullable case also seems inconsistent.

That said, I understand it could be a breaking change, in which case I'd suggest at least updating the documentation to highlight this exception to the rule?

Agreed. We should update that section to call out the behavior of both StringValues and string/tryparsable arrays.

[mitchdenny]

@halter73 what was the actual action here? From my reading we won't be changing any implementation here - but maybe a docs update?