Fix form handling and add NuGet pkg docs #55321

captainsafia · 2024-04-23T20:33:44Z

I'm working through the backlog of bug reports on OpenAPI and adding test cases for each one. While doing this, I encountered issues with the way we are currently handling:

APIs that consume multiple arguments from the form body
Controller-based APIs that consume arguments from the form body

This PR adds test coverage for these cases and updates the logic for generating form schemas to support both cases.

src/OpenApi/src/PACKAGE.md

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/test/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Parameters.cs

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/src/PACKAGE.md

amcasey

Early questions

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/test/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Parameters.cs

BrennanConroy · 2024-04-25T00:06:30Z

src/OpenApi/src/Services/OpenApiComponentService.cs

@@ -42,7 +42,26 @@ private readonly ConcurrentDictionary<(Type, ParameterInfo?), JsonObject> _schem
    {
        OnSchemaGenerated = (context, schema) =>
        {
-            schema.ApplyPrimitiveTypesAndFormats(context.TypeInfo.Type);
+            var type = context.TypeInfo.Type;
+            // Fix up schemas generated for IFormFile, IFormFileCollection, Stream, and PipeReader


Add any tests with Stream and PipeReader?

Add tests here. It appears that Stream/PipeReader get the same treatment as FormFiles when procued by the ApiExplorer layer for MVC. Added a note about this in #55349.

amcasey

More questions.

src/OpenApi/src/Services/OpenApiComponentService.cs

amcasey · 2024-04-26T00:25:42Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

        var schema = new OpenApiSchema { Type = "object", Properties = new Dictionary<string, OpenApiSchema>() };
-        foreach (var parameter in formParameters)
+        // Group form parameters by their parameter name because MVC explodes form parameters that are bound from the
+        // same model instance into separate parameters, while minimal APIs does not.


Suggested change

// same model instance into separate parameters, while minimal APIs does not.

// same model instance into separate parameters in ApiExplorer, while minimal APIs does not.

amcasey · 2024-04-26T00:27:58Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

-            schema.Properties[parameter.Name] = _componentService.GetOrCreateSchema(parameter.Type);
+            if (parameter.All(parameter => parameter.ModelMetadata.ContainerType is null))
+            {
+                var description = parameter.Single();


I don't understand why it follows that if the container types are all null there must only be one item in the group.

parameter.ModelMetadata.ContainerType is null when the form arguments haven't been exploded into seperate ApiParameterDescriptions as in the MVC model. I'll add a comment about this.

amcasey · 2024-04-26T00:29:46Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+                {
+                    if (hasMultipleFormParameters)
+                    {
+                        if (description.ModelMetadata.IsComplexType)


This is pulled out because we can refer to the complex type by name and don't have to create an inline schema? Assuming so, is it possible for the complex type to be anonymous or unnameable?

All the schemas that we generate are currently inline at the moment. This is more-so about the fact that schemas for complex types don't need to be nested under the parameter name based on the way parameters are bound from the form.

For an endpoint like:

app.MapPost("/", ([FromForm] Todo todo => { });

We want to generate the following schema:

{ "properties": { "id": { ... }, "title": { ... }, "isCompleted": { ... } } }

Not this one:

{ "properties": { "todo": { "properties": { "id": { ... }, "title": { ... }, "isCompleted": { ... } } } } }

...pshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=forms.verified.txt

amcasey

More questions.

amcasey · 2024-04-26T23:08:12Z

src/OpenApi/src/Services/OpenApiComponentService.cs

-            schema.ApplyPrimitiveTypesAndFormats(context.TypeInfo.Type);
+            var type = context.TypeInfo.Type;
+            // Fix up schemas generated for IFormFile, IFormFileCollection, Stream, and PipeReader
+            // that appear as properties within complex types.


I think the goal here is to take a type like class Todo { public Stream Stream { get; set; } } and update the schema for the Stream property. Assuming so, does/should this act recursively?

Ish. The underlying schema generator will use $ref to process the recursive type so the schema generated at the top-level will work for this. We don't handle ref IDs properly at the moment (follow-up work incoming) so the schemas are only partially correct. I'll add a test to reflect the current state of things.

amcasey · 2024-04-26T23:12:40Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

        var schema = new OpenApiSchema { Type = "object", Properties = new Dictionary<string, OpenApiSchema>() };
-        foreach (var parameter in formParameters)
+        // Group form parameters by their parameter name because MVC explodes form parameters that are bound from the


I think this sentence is distinguishing between "form parameter" and "parameter", but I'm not sure how. I think the latter might correspond to MVC action parameters?

This grouping will apply to all form parameters, regardless of whether we are currently processing them for an MVC action or minimal endpoint. The comment is intended to clarify why grouping by name allows us to "un-explode" the ApiParameterDescriptions that are produced by ApiExplorer specifically for MVC.

amcasey · 2024-04-26T23:13:49Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+        // public void PostMvc([FromForm] Todo person) { }
+        // app.MapGet("/form-todo", ([FromForm] Todo todo) => Results.Ok(todo));
+        //
+        // In the example above, MVC will bind four separate arguments to the Todo model while minimal APIs will


I think, in this context, "bind" refers to how things are exposed in the API explorer, rather than to what we call "parameter binding" in aspnetcore?

Yep, this is about ApiExplorer. I'll tweak the verbiage.

amcasey · 2024-04-26T23:27:34Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+                            Type = "object",
+                            Properties = new Dictionary<string, OpenApiSchema>
+                            {
+                                [description.Name] = _componentService.GetOrCreateSchema(description.Type)


I wonder if it would be more readable with _componentService.GetOrCreateSchema(description.Type) pulled out. That might make it clearer that the same value is just being slotted into different places in the schema.

amcasey · 2024-04-26T23:31:53Z

This and this could affect correctness. The rest are basically hygiene suggestions or questions for my own edification.

Fix form handling and add NuGet pkg docs

fe5ff34

captainsafia requested a review from a team as a code owner April 23, 2024 20:33

dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Apr 23, 2024

captainsafia requested review from BrennanConroy and amcasey April 23, 2024 21:15

BrennanConroy reviewed Apr 23, 2024

View reviewed changes

amcasey reviewed Apr 23, 2024

View reviewed changes

Address feedback and add more tests

74b3063

BrennanConroy approved these changes Apr 25, 2024

View reviewed changes

Add tests for Stream and PipeReader

1908c19

amcasey reviewed Apr 26, 2024

View reviewed changes

More tests and feedback

8738bb6

captainsafia merged commit 6d9dca9 into main Apr 29, 2024
26 checks passed

captainsafia deleted the safia/openapi-tests-docs branch April 29, 2024 22:11

dotnet-policy-service bot added this to the 9.0-preview5 milestone Apr 29, 2024

MackinnonBuck mentioned this pull request Sep 12, 2024

Missing package README file #48392

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix form handling and add NuGet pkg docs #55321

Fix form handling and add NuGet pkg docs #55321

captainsafia commented Apr 23, 2024

amcasey left a comment

BrennanConroy Apr 25, 2024

captainsafia Apr 25, 2024

amcasey left a comment

amcasey Apr 26, 2024

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey left a comment

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey Apr 26, 2024

captainsafia Apr 29, 2024

amcasey commented Apr 26, 2024

	// same model instance into separate parameters, while minimal APIs does not.
	// same model instance into separate parameters in ApiExplorer, while minimal APIs does not.

Fix form handling and add NuGet pkg docs #55321

Fix form handling and add NuGet pkg docs #55321

Conversation

captainsafia commented Apr 23, 2024

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Apr 26, 2024