Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal Actions - array support in method signatures from querystring binding #32516

Closed
jchannon opened this issue May 8, 2021 · 12 comments · Fixed by #39809
Closed

Minimal Actions - array support in method signatures from querystring binding #32516

jchannon opened this issue May 8, 2021 · 12 comments · Fixed by #39809
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@jchannon
Copy link
Contributor

jchannon commented May 8, 2021

Describe the bug

When using the ASPP.NET 6 minimal hosting, querystring binding does not work with arrays/lists

To Reproduce

app.MapGet("/qs/{id:int}", HttpHandlers.GetByQueryString);

public static string GetByQueryString(int id, string name, int[] values)
{
    return id + " " + name + string.Join(",", values);
}

Adding [FromQuery(Name = "values")] to the method signature does not help either.

Exceptions (if any)

An exception is thrown when sending in a request:

curl http://localhost:5000/qs/123\?name\=dave\&values\=1\&values\=2

Unhandled exception. System.InvalidOperationException: No public static bool Int32[].TryParse(string, out Int32[]) method found for values.

Further technical details

  • ASP.NET Core version 6
.NET SDK (reflecting any global.json):
 Version:   6.0.100-preview.5.21252.2
 Commit:    921395c33e

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  11.3
 OS Platform: Darwin
 RID:         osx.11.0-x64
 Base Path:   /Users/jonathan/.dotnet/sdk/6.0.100-preview.5.21252.2/

Host (useful for support):
  Version: 6.0.0-preview.5.21251.4
  Commit:  d2fba8fdc4

.NET SDKs installed:
  2.1.809 [/Users/jonathan/.dotnet/sdk]
  3.1.401 [/Users/jonathan/.dotnet/sdk]
  3.1.403 [/Users/jonathan/.dotnet/sdk]
  5.0.103 [/Users/jonathan/.dotnet/sdk]
  6.0.100-preview.5.21229.26 [/Users/jonathan/.dotnet/sdk]
  6.0.100-preview.5.21252.2 [/Users/jonathan/.dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.21 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.21 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.7 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.9 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.3 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0-preview.5.21229.9 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0-preview.5.21230.5 [/Users/jonathan/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.21 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.7 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.9 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.3 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0-preview.5.21229.1 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0-preview.5.21251.4 [/Users/jonathan/.dotnet/shared/Microsoft.NETCore.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download

@davidfowl davidfowl added the feature-minimal-actions Controller-like actions for endpoint routing label May 8, 2021
@jchannon jchannon changed the title Minimal Hosting - array support in method signatures Minimal Hosting - array support in method signatures from querystring binding May 8, 2021
@davidfowl
Copy link
Member

Would this only be for simple types? Today we don't support any complex types for model binding and it conflicts with this idea

@jchannon
Copy link
Contributor Author

jchannon commented May 8, 2021

Yup simple types only imo, it's a query string, you shouldn't be doing crazy stuff like json binding. I mean there's nothing stopping someone adding json in a querystring I just don't think the framework should support that

@halter73 halter73 changed the title Minimal Hosting - array support in method signatures from querystring binding Minimal Actions - array support in method signatures from querystring binding May 10, 2021
@BrennanConroy BrennanConroy added this to the Backlog milestone May 10, 2021
@ghost
Copy link

ghost commented May 10, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@BrennanConroy BrennanConroy added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 10, 2021
@halter73
Copy link
Member

halter73 commented Jul 8, 2021

What collection types should we support?

What should we do if we know the param comes from the route? Fail at startup? Provide a single-element array?

@DamianEdwards
Copy link
Member

With the newly added support for BindAsync(HttpContext) on the target parameter type it would be easy to support this in the app or a library via a custom type, e.g. MultiValueQuery values, where MultiValueQuery has a BindAsync method on it.

@rafikiassumani-msft rafikiassumani-msft added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Sep 29, 2021
@rafikiassumani-msft rafikiassumani-msft added triage-focus Add this label to flag the issue for focus at triage Priority:2 Work that is important, but not critical for the release Cost:S and removed Cost:S labels Jan 11, 2022
@davidfowl
Copy link
Member

davidfowl commented Jan 24, 2022

@halter73

What collection types should we support?

This is where this typically falls of the rails. There are A LOT of collection types including custom types that implement collection interfaces. It's possible we can do something like pattern match a .Add method similar to what the ConfigurationBinder does.

This currently also conflicts with JSON bodies sent as arrays so it would need to be explicitly declared as [FromQuery] in order to make this work.

@JamesNK I'd like your feedback here.

What should we do if we know the param comes from the route? Fail at startup? Provide a single-element array?

I think fail. This should work for headers and query but not route.

Open questions:

Reasons to avoid anything except for arrays in this scenario:

  • It makes the code simpler (yay)
  • It is more trimming friendly, we don't need to MakeGenericType on BCL types (I know we're forced to do this today for ValueTask<T> and Task<T> but we can keep it isolated). We just need to keep T[] instead of List<T> and we don't need to do crazy things to call Add on concrete types VS abstractions etc. etc.

So strawman proposal is:

  • Only arrays (T[])
  • Supports TryParse with the value of the query string parameter
  • Support string[]
  • Explicit sources are required for requests that do infer a body (POST/PUT/PATCH) (e.g. [FromQuery], [FromHeader])
  • Implicit binding is supported for methods that do not infer a body (GET/HEAD/DELETE)
  • If there are no values we pass an empty array unless the user makes the array nullable.

@davidfowl
Copy link
Member

@jchannon This is supported in .NET 7 preview1

@davidfowl
Copy link
Member

I mean preview2

@rafikiassumani-msft rafikiassumani-msft added Docs This issue tracks updating documentation and removed triage-focus Add this label to flag the issue for focus at triage labels Feb 10, 2022
@legistek
Copy link

Previously (in Framework) you could serialize an array of primitives to a JSON object in the query string like this:

?values=[1,2,3,4,5]

(escaped obviously)

and the method:

DoSomething(int[] values)

would bind correctly. That doesn't appear to be the case in .NET6 (or possibly earlier). I assume that was a known breaking change but I thought it worth asking? If it was not an intentional change then it's a different bug I guess.

@davidfowl
Copy link
Member

This issue isn't about MVC's binding semantics and minimal API supports a subset of what MVC supports. If you want to see what is supported for MVC today look at https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-6.0#collections

@legistek
Copy link

Thanks. Sorry this seemed like the closest issue to what I'm experiencing that wasn't locked by the bots.

@davidfowl
Copy link
Member

davidfowl commented Feb 23, 2022

Thanks. Sorry this seemed like the closest issue to what I'm experiencing that wasn't locked by the bots.

😄. FWIW, it's not supported in MVC either, that's by design.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing Priority:2 Work that is important, but not critical for the release
Projects
None yet
9 participants