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

Fix form handling and add NuGet pkg docs #55321

Merged
merged 4 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/OpenApi/sample/Controllers/TestController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ public string GetByIdAndName(RouteParamsContainer paramsContainer)
return paramsContainer.Id + "_" + paramsContainer.Name;
}

[HttpPost]
[Route("/forms")]
public IActionResult PostForm([FromForm] MvcTodo todo)
{
return Ok(todo);
}

public class RouteParamsContainer
{
[FromRoute]
Expand All @@ -21,4 +28,6 @@ public class RouteParamsContainer
[MinLength(5)]
public string? Name { get; set; }
}

public record MvcTodo(string Title, string Description, bool IsCompleted);
}
2 changes: 2 additions & 0 deletions src/OpenApi/sample/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@

forms.MapPost("/form-file", (IFormFile resume) => Results.Ok(resume.FileName));
forms.MapPost("/form-files", (IFormFileCollection files) => Results.Ok(files.Count));
forms.MapPost("/form-file-multiple", (IFormFile resume, IFormFileCollection files) => Results.Ok(files.Count + resume.FileName));
forms.MapPost("/form-todo", ([FromForm] Todo todo) => Results.Ok(todo));
forms.MapPost("/forms-pocos-and-files", ([FromForm] Todo todo, IFormFile file) => Results.Ok(new { Todo = todo, File = file.FileName }));

var v1 = app.MapGroup("v1")
.WithGroupName("v1");
Expand Down
54 changes: 54 additions & 0 deletions src/OpenApi/src/PACKAGE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
## About

Microsoft.AspNetCore.OpenApi is a NuGet package that provides built-in support for generating OpenAPI documents from minimal or controller-based APIs in ASP.NET Core.

## Key Features

* Supports viewing generated OpenAPI documents at runtime via a parameterized endpoint (`/openapi/{documentName}.json`)
* Supports generating an OpenAPI document at build-time
* Supports customizing the generated document via document transformers

## How to Use

To start using Microsoft.AspNetCore.OpenApi in your ASP.NET Core application, follow these steps:

### Installation

```sh
dotnet add package Microsoft.AspNetCore.OpenApi
```

### Configuration

In your Program.cs file, register the services provided by this package in the DI container and map the provided OpenAPI document endpoint in the application.

```C#
var builder = WebApplication.CreateBuilder();

// Registers the required services
builder.Services.AddOpenApi();

var app = builder.Build();

// Adds the /openapi/{documentName}.json endpoint to the application
app.MapOpenApi();

app.Run();
```

For more information on configuring and using Microsoft.AspNetCore.OpenApi, refer to the [official documentation](https://learn.microsoft.com/aspnet/core/fundamentals/minimal-apis/openapi).

## Main Types

<!-- The main types provided in this library -->

The main types provided by this library are:

* `OpenApiOptions`: Options for configuring OpenAPI document generation.
* `IDocumentTransformer`: Transformer that modifies the OpenAPI document generated by the library.

## Feedback & Contributing

<!-- How to provide feedback on this package and contribute to it -->

Microsoft.AspNetCore.OpenApi is released as open-source under the [MIT license](https://licenses.nuget.org/MIT). Bug reports and contributions are welcome at [the GitHub repository](https://github.com/dotnet/aspnetcore).
21 changes: 20 additions & 1 deletion src/OpenApi/src/Services/OpenApiComponentService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Add any tests with Stream and PipeReader?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// that appear as properties within complex types.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (type == typeof(IFormFile) || type == typeof(Stream) || type == typeof(PipeReader))
amcasey marked this conversation as resolved.
Show resolved Hide resolved
{
schema.Clear();
schema[OpenApiSchemaKeywords.TypeKeyword] = "string";
schema[OpenApiSchemaKeywords.FormatKeyword] = "binary";
}
else if (type == typeof(IFormFileCollection))
{
schema.Clear();
schema[OpenApiSchemaKeywords.TypeKeyword] = "array";
schema[OpenApiSchemaKeywords.ItemsKeyword] = new JsonObject
{
[OpenApiSchemaKeywords.TypeKeyword] = "string",
[OpenApiSchemaKeywords.FormatKeyword] = "binary"
};
}
schema.ApplyPrimitiveTypesAndFormats(type);
if (context.GetCustomAttributes(typeof(ValidationAttribute)) is { } validationAttributes)
{
schema.ApplyValidationAttributes(validationAttributes);
Expand Down
95 changes: 91 additions & 4 deletions src/OpenApi/src/Services/OpenApiDocumentService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ private OpenApiResponse GetResponse(ApiDescription apiDescription, int statusCod
.Select(responseFormat => responseFormat.MediaType);
foreach (var contentType in apiResponseFormatContentTypes)
{
var schema = apiResponseType.Type is {} type ? _componentService.GetOrCreateSchema(type) : new OpenApiSchema();
var schema = apiResponseType.Type is { } type ? _componentService.GetOrCreateSchema(type) : new OpenApiSchema();
response.Content[contentType] = new OpenApiMediaType { Schema = schema };
}

Expand Down Expand Up @@ -296,11 +296,98 @@ private OpenApiRequestBody GetFormRequestBody(IList<ApiRequestFormat> supportedR
Content = new Dictionary<string, OpenApiMediaType>()
};

// Forms are represented as objects with properties for each form field.
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

//
// public record Todo(int Id, string Title, bool Completed, DateTime CreatedAt)
// 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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// bind a single Todo model instance to the todo parameter. Grouping by name allows us to handle both cases.
var groupedFormParameters = formParameters.GroupBy(parameter => parameter.ParameterDescriptor.Name);
// If there is only one real parameter derived from the form body, then set it directly in the schema.
var hasMultipleFormParameters = groupedFormParameters.Count() > 1;
foreach (var parameter in groupedFormParameters)
{
schema.Properties[parameter.Name] = _componentService.GetOrCreateSchema(parameter.Type);
if (parameter.All(parameter => parameter.ModelMetadata.ContainerType is null))
{
var description = parameter.Single();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// Form files are keyed by their parameter name so we must capture the parameter name
// as a property in the schema.
if (description.Type == typeof(IFormFile) || description.Type == typeof(IFormFileCollection))
{
if (hasMultipleFormParameters)
{
schema.AllOf.Add(new OpenApiSchema
{
Type = "object",
Properties = new Dictionary<string, OpenApiSchema>
{
[description.Name] = _componentService.GetOrCreateSchema(description.Type)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

}
});
}
else
{
schema.Properties[description.Name] = _componentService.GetOrCreateSchema(description.Type);
}
}
else
{
if (hasMultipleFormParameters)
{
if (description.ModelMetadata.IsComplexType)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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": { ... }
			}
		}
	}
}

{
schema.AllOf.Add(_componentService.GetOrCreateSchema(description.Type));
}
else
{
schema.AllOf.Add(new OpenApiSchema
{
Type = "object",
Properties = new Dictionary<string, OpenApiSchema>
{
[description.Name] = _componentService.GetOrCreateSchema(description.Type)
}
});
}
}
else
{
// POCOs do not need to be subset under their parameter name in the properties grouping.
// The form-binding implementation will capture them implicitly.
if (description.ModelMetadata.IsComplexType)
{
schema = _componentService.GetOrCreateSchema(description.Type);
}
else
{
schema.Properties[description.Name] = _componentService.GetOrCreateSchema(description.Type);
}
}
}
}
else
{
if (hasMultipleFormParameters)
{
var propertySchema = new OpenApiSchema { Type = "object", Properties = new Dictionary<string, OpenApiSchema>() };
foreach (var description in parameter)
{
propertySchema.Properties[description.Name] = _componentService.GetOrCreateSchema(description.Type);
}
schema.AllOf.Add(propertySchema);
}
else
{
foreach (var description in parameter)
{
schema.Properties[description.Name] = _componentService.GetOrCreateSchema(description.Type);
}
}
}
}

foreach (var requestFormat in supportedRequestFormats)
Expand Down