Add support for generating OpenAPI request bodies #55040

captainsafia · 2024-04-09T22:32:13Z

Note: I'm trying Copilot-generated pull request descriptions for the first time. 🤪

This pull request adds support for generating request body objects into the OpenAPI document.

Key changes include:

Addition of form parameters handling:

src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs: Added new extension methods TryGetFormParameters and TryGetBodyParameter to retrieve form and body parameters respectively from the ApiDescription.

Enhancements to OpenApiComponentService:

src/OpenApi/src/Services/OpenApiComponentService.cs: Introduced a concurrent dictionary _schemas to cache OpenAPI schemas for well-defined types in ASP.NET Core. Also added the GetOrCreateSchema method to get or create a schema for a given type.

Refinements to OpenApiDocumentService:

src/OpenApi/src/Services/OpenApiDocumentService.cs: Integrated OpenApiComponentService into OpenApiDocumentService and added methods to handle request bodies, including form request bodies and JSON request bodies. [1] [2] [3] [4]

New extension methods and tests:

src/OpenApi/src/Extensions/TypeExtensions.cs: Added new extension methods GetSchemaReferenceId and IsAnonymousType for Type.
src/OpenApi/test/Extensions/TypeExtensionsTests.cs: Added corresponding tests for the new extension methods.

Minor changes and fixes:

src/OpenApi/sample/Program.cs: Imported Microsoft.AspNetCore.Mvc namespace, added AddOpenApi service, introduced a new mapping group "forms" with several endpoints, and updated the Todo record. [1] [2] [3] [4]
src/OpenApi/test/Services/OpenApiDocumentServiceTests.Info.cs: Replaced IServiceProvider with IKeyedServiceProvider in tests. [1] [2]

martincostello · 2024-04-10T06:36:31Z

src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs

+        if (bodyParameters.Count() == 1)
+        {
+            bodyParameter = bodyParameters.Single();


Use SingleOrDefault()?

I think that throws if there are multiple elements? (Unless you meant under the if, in which case, why?)

I would have gone the other way and suggested First since there's no reason to do another bounds check.

Oh that's me misremembering then. I thought Single() was one or exception, and SingleOrDefault() was one or null/default.

SingleOrDefault will only return null/default if the list is empty. If there are multiple elements, it'll through an exception. Although based on the check that we are doing in the if, that should never be the case hence my inclination to use Single. First could also be used here but I feel it obfuscates the intention that there should always be only one parameter resolved from the JSON request body in a given request.

martincostello · 2024-04-10T06:39:36Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+            return prefix + "AnonymousType";
+        }
+
+        return prefix + type.Name.Split('`').First();


(Teeny) perf vs readability option:

Suggested change

return prefix + type.Name.Split('`').First();

return prefix + type.Name.Split('`')[0];

That's not the thing that's going to slow this code down. 😆

src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs

amcasey · 2024-04-10T18:48:14Z

src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs

+        if (bodyParameters.Count() == 1)
+        {
+            bodyParameter = bodyParameters.Single();


I think that throws if there are multiple elements? (Unless you meant under the if, in which case, why?)

amcasey · 2024-04-10T18:49:08Z

src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs

+        if (bodyParameters.Count() == 1)
+        {
+            bodyParameter = bodyParameters.Single();


I would have gone the other way and suggested First since there's no reason to do another bounds check.

src/OpenApi/sample/Program.cs

amcasey · 2024-04-10T18:52:50Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+    {
+        if (!type.IsConstructedGenericType)
+        {
+            return type.Name.Replace("[]", "Array");


typeof(int[,])

amcasey · 2024-04-10T18:53:44Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+    /// </summary>
+    /// <param name="type">The <see cref="Type"/> to resolve a schema reference identifier for.</param>
+    /// <returns>The schema reference identifier associated with <paramref name="type"/>.</returns>
+    public static string GetSchemaReferenceId(this Type type)


Can type be a nested type?

amcasey · 2024-04-10T18:55:04Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+        }
+
+        var prefix = type.GetGenericArguments()
+            .Select(GetSchemaReferenceId)


I'd be a little nervous about blowing the stack here.

amcasey · 2024-04-10T18:55:52Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+    public static bool IsAnonymousType(this Type type) =>
+        type.GetTypeInfo().IsClass
+            && type.GetTypeInfo().IsDefined(typeof(CompilerGeneratedAttribute))
+            && !type.IsNested


Why can't anonymous types be nested?

amcasey · 2024-04-10T18:58:30Z

src/OpenApi/src/Extensions/TypeExtensions.cs

+            return prefix + "AnonymousType";
+        }
+
+        return prefix + type.Name.Split('`').First();


That's not the thing that's going to slow this code down. 😆

BrennanConroy · 2024-04-10T19:37:12Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+        {
+            // Assume "application/x-www-form-urlencoded" as the default media type
+            // to match the default assumed in IFormFeature.
+            supportedRequestFormats = [new ApiRequestFormat { MediaType = "application/x-www-form-urlencoded" }];


How does one change this?

For overriding whatever content-type is used, end-users can add Accepts metadata to the given endpoint.

BrennanConroy · 2024-04-10T19:44:25Z

src/OpenApi/test/Extensions/TypeExtensionsTests.cs

+        [(new { Id = 1, Name = "Todo" }).GetType(), "Int32StringAnonymousType"],
+        [typeof(IFormFile), "IFormFile"],
+        [typeof(IFormFileCollection), "IFormFileCollection"],
+        [typeof(Results<Ok<TodoWithDueDate>, Ok<Todo>>), "TodoWithDueDateOkTodoOkResults"],


Is the name generation defined in a spec or are we making it up?

If this is supposed to be human-readable, some underscores might help.

It's not defined in the spec. The only constraints that the spec puts on these keys is that they have to be case sensitive.

I opted for alphanumeric values (avoiding things like "<" or "`").

If this is supposed to be human-readable, some underscores might help.

Adding this would help although underscores as separators isn't a convention I've usually seen. Conventionally, most APIs tend to use PascalCase for type names.

I meant between types, not within types. TodoWithDueDateOk_TodoOk_Results

That would be TodoWithDueDate_Ok_Todo_Ok_Results in this case?
Throwing out random thought: ResultsOf_OkOf_TodoWithDueDate_And_OkOf_Todo

There's enough context that the printer could skip it for arity 1, but sure, might as well go all the way. I like "Of", but I find "And" pretty wordy.

src/OpenApi/src/Services/OpenApiComponentService.cs

amcasey · 2024-04-10T21:00:17Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+        return null;
+    }
+
+    private OpenApiRequestBody GetFormRequestBody(ApiDescription description, IEnumerable<ApiParameterDescription> formParameters)


Nit: Personally, I would find it cleaner to pass in the supported request formats. It feels weird to pull the parameters out of the description and then pass both those parameters and the description they came from to this helper.

And below.

amcasey · 2024-04-10T21:14:27Z

src/OpenApi/src/Services/OpenApiComponentService.cs

+        return _schemas.GetOrAdd(schemaId, _ => CreateSchema());
+    }
+
+    internal static OpenApiSchema CreateSchema()


amcasey · 2024-04-10T21:16:27Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+            schema.Properties[parameter.Name] = _componentService.GetOrCreateSchema(parameter.Type);
+        }
+
+        foreach (var requestForm in supportedRequestFormats)


I might call this "requestFormat" since we're talking about forms in the same scope.

Yes, I believe this was a typo on my part. 😅

amcasey · 2024-04-10T21:19:16Z

src/OpenApi/test/Extensions/TypeExtensionsTests.cs

+        [(new { Id = 1, Name = "Todo" }).GetType(), "Int32StringAnonymousType"],
+        [typeof(IFormFile), "IFormFile"],
+        [typeof(IFormFileCollection), "IFormFileCollection"],
+        [typeof(Results<Ok<TodoWithDueDate>, Ok<Todo>>), "TodoWithDueDateOkTodoOkResults"],


If this is supposed to be human-readable, some underscores might help.

amcasey · 2024-04-10T21:19:50Z

src/OpenApi/test/Extensions/TypeExtensionsTests.cs

+        [typeof(IFormFile), false],
+        [typeof(IFormFileCollection), false],
+        [typeof(Results<Ok<TodoWithDueDate>, Ok<Todo>>), false],
+        [typeof(TestDelegate), false]


Test a nested type?

captainsafia · 2024-04-11T16:03:41Z

After noodling on it some more, I decide to key the dictionary tracking the schemas by Type instead of reference ID. We will still need to solve the problem of generating reference IDs from .NET types when we materialize the schemas that are stored into the cache into the document.

My original prototype keyed by Type as well and I diverged from this here but realized that there's value in caching by type because having information about the Type associated with the schema makes it easier to apply schema filters.

Also, I accidentally amended a commit while rebasing changes instead of creating a new commit. Force of habit. 😅 I'm used to rebasing and amending on branches before I make them public in a PR. This unfortunately means that the history for this PR is a little wonky. 😞

amcasey

I think not rolling your own type stringification was the right call. 😆

captainsafia · 2024-04-16T18:01:53Z

I think not rolling your own type stringification was the right call. 😆

Yes -- and luckily we won't have to include this in our codepath in the long-term because the runtime's implementation will support generating schema reference IDs.

Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]> This PR adds support for OpenAPI document generation, sans schema generation to Microsoft.AspNetCore.OpenApi. Relevant changes are available in individual PRs: - Add entry-point APIs for OpenAPI support (#54789) - Support resolving OpenApiPaths entries from document (#54847) - Support generating OpenAPI operation and associated fields (#54903) - Add APIs for OpenAPI document transformers (#54935) - Add support for generating OpenAPI parameters (#55041) - Add support for generating OpenAPI responses (#55020) - Add support for generating OpenAPI request bodies (#55040)

captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc labels Apr 9, 2024

captainsafia requested a review from a team as a code owner April 9, 2024 22:32

captainsafia requested review from BrennanConroy and amcasey April 9, 2024 22:32

martincostello reviewed Apr 10, 2024

View reviewed changes

amcasey reviewed Apr 10, 2024

View reviewed changes

BrennanConroy reviewed Apr 10, 2024

View reviewed changes

amcasey reviewed Apr 10, 2024

View reviewed changes

captainsafia added 3 commits April 10, 2024 17:21

Support generating request bodies for JSON and form inputs

6295da7

Update schema reference ID generation

eadc564

Key schema cache by Type instead of reference ID

987a385

captainsafia force-pushed the safia/get-requestbody branch from 0f78d11 to 987a385 Compare April 11, 2024 15:58

Document and make GetSchema private

3381e60

BrennanConroy approved these changes Apr 16, 2024

View reviewed changes

amcasey approved these changes Apr 16, 2024

View reviewed changes

captainsafia merged commit d82dab5 into feature/openapi Apr 16, 2024
21 checks passed

captainsafia deleted the safia/get-requestbody branch April 16, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generating OpenAPI request bodies #55040

Add support for generating OpenAPI request bodies #55040

captainsafia commented Apr 9, 2024

martincostello Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

martincostello Apr 10, 2024

captainsafia Apr 10, 2024

martincostello Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

BrennanConroy Apr 10, 2024

captainsafia Apr 10, 2024

BrennanConroy Apr 10, 2024

amcasey Apr 10, 2024

captainsafia Apr 10, 2024

amcasey Apr 10, 2024

BrennanConroy Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

captainsafia Apr 10, 2024

amcasey Apr 10, 2024

amcasey Apr 10, 2024

captainsafia commented Apr 11, 2024

amcasey left a comment

captainsafia commented Apr 16, 2024

	return prefix + type.Name.Split('`').First();
	return prefix + type.Name.Split('`')[0];

Add support for generating OpenAPI request bodies #55040

Add support for generating OpenAPI request bodies #55040

Conversation

captainsafia commented Apr 9, 2024

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

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

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

Choose a reason for hiding this comment

captainsafia commented Apr 11, 2024

amcasey left a comment

Choose a reason for hiding this comment

captainsafia commented Apr 16, 2024