Initial Form-binding support by brunolins16 · Pull Request #44653 · dotnet/aspnetcore

brunolins16 · 2022-10-19T23:05:40Z

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

davidfowl · 2022-10-20T05:35:51Z

@brunolins16 any reason this is a draft?

brunolins16 · 2022-10-20T06:11:26Z

@brunolins16 any reason this is a draft?

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

brunolins16 · 2022-10-31T17:19:29Z

Please take a look 🙏@BrennanConroy @captainsafia @halter73

BrennanConroy

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.

BrennanConroy · 2022-10-31T18:23:40Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

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


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.

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.

aspnetcore/src/Http/Http/src/Features/FormFeature.cs

Line 171 in 8dc4325

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?

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs

brunolins16 · 2022-10-31T20:28:14Z

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.

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

captainsafia · 2022-11-02T01:24:14Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+            parameter,
+            valueExpression,
+            factoryContext,
+            "form file");


Nit: make this a constant?

src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs

davidfowl · 2023-01-14T17:17:47Z

@brunolins16 where's the issue for this PR?

brunolins16 · 2023-01-23T21:57:42Z

@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 · 2023-01-23T21:57:46Z

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.

brunolins16 added 2 commits October 19, 2022 15:50

Adding initial Form-support

dd6fabb

clean up

833ce38

brunolins16 added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Oct 19, 2022

brunolins16 requested a review from a team October 19, 2022 23:05

ghost added the area-runtime label Oct 19, 2022

brunolins16 changed the title ~~Brunolins/minimal apis/form support v1~~ Initial Form-binding support Oct 19, 2022

brunolins16 removed the area-runtime label Oct 19, 2022

brunolins16 marked this pull request as ready for review October 20, 2022 06:11

brunolins16 requested review from BrennanConroy, Tratcher and halter73 as code owners October 20, 2022 06:11

BrennanConroy reviewed Oct 31, 2022

View reviewed changes

brunolins16 added 3 commits October 31, 2022 16:43

PR feedback

c931b8e

PR feedback

65745d3

Fix unit test

dadca52

BrennanConroy approved these changes Nov 1, 2022

View reviewed changes

brunolins16 added 3 commits November 1, 2022 13:34

Adding Form Accepts Metadata later

5c892fe

clean up

455735e

Fix warnings

a8c18b3

captainsafia reviewed Nov 2, 2022

View reviewed changes

halter73 approved these changes Nov 2, 2022

View reviewed changes

src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs Show resolved Hide resolved

Adding a test for InferMetadata

1b4a884

build-analysis bot mentioned this pull request Nov 3, 2022

Tracking Nuget 429s dotnet/arcade#10885

Closed

2 tasks

brunolins16 merged commit ac50803 into dotnet:main Nov 8, 2022

brunolins16 deleted the brunolins/minimal-apis/form-support-v1 branch November 8, 2022 22:50

ghost added this to the 8.0-preview1 milestone Nov 8, 2022

davidfowl mentioned this pull request Jan 14, 2023

Please consider adding multipart-form-data #46103

Closed

This was referenced Jan 23, 2023

Support Form binding for Minimal endpoints #39430

Closed

New FromForm binding support dotnet/AspNetCore.Docs#28167

Closed

amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023

Conversation

brunolins16 commented Oct 19, 2022 • edited by davidfowl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General design:

Examples

Uh oh!

davidfowl commented Oct 20, 2022

Uh oh!

brunolins16 commented Oct 20, 2022

Uh oh!

brunolins16 commented Oct 31, 2022

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

brunolins16 Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunolins16 commented Oct 31, 2022

Uh oh!

Uh oh!

captainsafia Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidfowl commented Jan 14, 2023

Uh oh!

brunolins16 commented Jan 23, 2023

Uh oh!

ghost commented Jan 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brunolins16 commented Oct 19, 2022 •

edited by davidfowl

Loading