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

Initial Form-binding support #44653

Merged

Conversation

brunolins16
Copy link
Member

@brunolins16 brunolins16 commented Oct 19, 2022

General design:

  • No inference ([FromForm] required)
  • IFormCollection is supported and the source will be inferred
    • FromFromAttribute.Name not supported
  • No changes to the current array support
    • Only arrays (T[]) where T supports TryParse
    • Support string[] and StringValues
  • Inferred Accepts metadata (application/x-www-form-urlencoded / multipart/form-data)
    • multipart/form-data only when also include IFromFile or IFormFileCollection

Examples

app.MapPost("/form-collection", (IFormCollection form) => form["name"]);
app.MapPost("/form-collection", ([FromForm]IFormCollection form) => form["name"]);
app.MapPost("/string", ([FromForm] string name) => name);
app.MapPost("/string-array", ([FromForm] string[] names) => names);
app.MapPost("/string-values", ([FromForm] StringValues names) => names);
app.MapPost("/array-of-T", ([FromForm] int[] ids) => ids);
app.MapPost("/array-of-customT", ([FromForm] CustomType[] ids) => ids);

public class CustomType
{
    public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? provider, [MaybeNullWhen(false)] out CustomType result)
    {
        result = new CustomType();
        return true;
    }
}

Contributes to #39430

@brunolins16 brunolins16 added feature-minimal-actions Controller-like actions for endpoint routing area-web-frameworks labels Oct 19, 2022
@brunolins16 brunolins16 requested a review from a team October 19, 2022 23:05
@ghost ghost added the area-runtime label Oct 19, 2022
@brunolins16 brunolins16 changed the title Brunolins/minimal apis/form support v1 Initial Form-binding support Oct 19, 2022
@davidfowl
Copy link
Member

@brunolins16 any reason this is a draft?

@brunolins16
Copy link
Member Author

@brunolins16 any reason this is a draft?

Nope. I was waiting before I have all checks green to make it ready for review. 😬

@brunolins16 brunolins16 marked this pull request as ready for review October 20, 2022 06:11
@brunolins16
Copy link
Member Author

Please take a look 🙏@BrennanConroy @captainsafia @halter73

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care if someone accesses the same form data from multiple parameters? I assume it would just work because we are just reading indexes from an already created collection.

RequestDelegateFactoryContext factoryContext)
{
// Do not duplicate the metadata if there are multiple form file parameters
if (!factoryContext.ReadFormFile)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with how forms work, but is there any way to have this code path run and register just "multipart/form-data", then have one of the other paths run which would normally add "application/x-www-form-urlencoded" as well, but now they won't because we set ReadForm below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I get your question correctly but, while might be possible send file using application/x-www-form-urlencoded (i am not very familiar 🤷‍♂️ ) . The FormFeature only read files from a multipart/form-data request.

else if (HasMultipartFormContentType(contentType))

My approach here is to keep not adding duplicated metadata, however, since we don't know the parameter ordering and we process them sequentially, in case we found a IFormFile or IFormFileCollection an additional is added, for the first parameter only, with multipart/form-data and since the last metadata is the most relevant it should basically this one been used, make sense?

@brunolins16
Copy link
Member Author

Do we care if someone accesses the same form data from multiple parameters? I assume it would just work because we are just reading indexes from an already created collection.

It just works. if I am not wrong, we are calling Form.ReadAsync once and just accessing the created collection as you mentioned. It is a weird scenario, but I don't think we need to add restrictions. Multiples IFormFileCollection are supported already.

parameter,
valueExpression,
factoryContext,
"form file");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: make this a constant?

@build-analysis build-analysis bot mentioned this pull request Nov 3, 2022
2 tasks
@brunolins16 brunolins16 merged commit ac50803 into dotnet:main Nov 8, 2022
@brunolins16 brunolins16 deleted the brunolins/minimal-apis/form-support-v1 branch November 8, 2022 22:50
@ghost ghost added this to the 8.0-preview1 milestone Nov 8, 2022
@davidfowl
Copy link
Member

@brunolins16 where's the issue for this PR?

@brunolins16
Copy link
Member Author

@davidfowl #39430. I decided no close it because we need to investigate the complex types support but I should have updated the description to include what we have already.

@ghost
Copy link

ghost commented Jan 23, 2023

Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants