Fix mapping for nested schemas and [Produces] attributes in OpenAPI implementation (#57852)

captainsafia · captainsafia · commit 1d2c6eedf859 · 2024-12-31T14:57:03.000-08:00
* Fix handling of [Produces] without type on controller

* Recursively map sub-schemas to reference cache

* Rely on MaxDepth behavior in STJ

* Improve nested type test
diff --git a/src/OpenApi/src/Services/OpenApiDocumentService.cs b/src/OpenApi/src/Services/OpenApiDocumentService.cs
@@ -382,7 +382,7 @@ private async Task<OpenApiResponse> GetResponseAsync(
             .SelectMany(attr => attr.ContentTypes);
         foreach (var contentType in explicitContentTypes)
         {
-            response.Content[contentType] = new OpenApiMediaType();
+            response.Content.TryAdd(contentType, new OpenApiMediaType());
         }
 
         return response;
diff --git a/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs b/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs
@@ -32,6 +32,7 @@ internal sealed class OpenApiSchemaService(
     IOptionsMonitor<OpenApiOptions> optionsMonitor)
 {
     private readonly OpenApiSchemaStore _schemaStore = serviceProvider.GetRequiredKeyedService<OpenApiSchemaStore>(documentName);
+    private readonly OpenApiJsonSchemaContext _jsonSchemaContext = new OpenApiJsonSchemaContext(new(jsonOptions.Value.SerializerOptions));
     private readonly JsonSerializerOptions _jsonSerializerOptions = new(jsonOptions.Value.SerializerOptions)
     {
         // In order to properly handle the `RequiredAttribute` on type properties, add a modifier to support
@@ -135,7 +136,9 @@ internal async Task<OpenApiSchema> GetOrCreateSchemaAsync(Type type, IServicePro
         {
             schemaAsJsonObject.ApplyParameterInfo(parameterDescription, _jsonSerializerOptions.GetTypeInfo(type));
         }
-        var deserializedSchema = JsonSerializer.Deserialize(schemaAsJsonObject, OpenApiJsonSchemaContext.Default.OpenApiJsonSchema);
+        // Use _jsonSchemaContext constructed from _jsonSerializerOptions to respect shared config set by end-user,
+        // particularly in the case of maxDepth.
+        var deserializedSchema = JsonSerializer.Deserialize(schemaAsJsonObject, _jsonSchemaContext.OpenApiJsonSchema);
         Debug.Assert(deserializedSchema != null, "The schema should have been deserialized successfully and materialize a non-null value.");
         var schema = deserializedSchema.Schema;
         await ApplySchemaTransformersAsync(schema, type, scopedServiceProvider, schemaTransformers, parameterDescription, cancellationToken);
diff --git a/src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs b/src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs
@@ -77,34 +77,38 @@ public JsonNode GetOrAdd(OpenApiSchemaKey key, Func<OpenApiSchemaKey, JsonNode>
     /// been encountered more than once in the document to avoid unnecessarily capturing unique
     /// schemas into the top-level document.
     /// </summary>
+    /// <remarks>
+    /// We don't do a depth check in the recursion call here since we assume that
+    /// System.Text.Json has already validate the depth of the schema based on
+    /// the configured JsonSerializerOptions.MaxDepth value.
+    /// </remarks>
     /// <param name="schema">The <see cref="OpenApiSchema"/> to add to the schemas-with-references cache.</param>
     /// <param name="captureSchemaByRef"><see langword="true"/> if schema should always be referenced instead of inlined.</param>
     public void PopulateSchemaIntoReferenceCache(OpenApiSchema schema, bool captureSchemaByRef)
     {
-        // Only capture top-level schemas by ref. Nested schemas will follow the
-        // reference by duplicate rules.
         AddOrUpdateSchemaByReference(schema, captureSchemaByRef: captureSchemaByRef);
         AddOrUpdateAnyOfSubSchemaByReference(schema);
+
         if (schema.AdditionalProperties is not null)
         {
-            AddOrUpdateSchemaByReference(schema.AdditionalProperties);
+            PopulateSchemaIntoReferenceCache(schema.AdditionalProperties, captureSchemaByRef);
         }
         if (schema.Items is not null)
         {
-            AddOrUpdateSchemaByReference(schema.Items);
+            PopulateSchemaIntoReferenceCache(schema.Items, captureSchemaByRef);
         }
         if (schema.AllOf is not null)
         {
             foreach (var allOfSchema in schema.AllOf)
             {
-                AddOrUpdateSchemaByReference(allOfSchema);
+                PopulateSchemaIntoReferenceCache(allOfSchema, captureSchemaByRef);
             }
         }
         if (schema.Properties is not null)
         {
             foreach (var property in schema.Properties.Values)
             {
-                AddOrUpdateSchemaByReference(property);
+                PopulateSchemaIntoReferenceCache(property, captureSchemaByRef);
             }
         }
     }
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/CreateSchemaReferenceIdTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/CreateSchemaReferenceIdTests.cs
@@ -65,7 +65,7 @@ public async Task GeneratesSchemaForPoco_WithSchemaReferenceIdCustomization()
 
         // Act
         builder.MapPost("/", (Todo todo) => { });
-        var options = new OpenApiOptions { CreateSchemaReferenceId = (type) => $"{type.Type.Name}Schema" };
+        var options = new OpenApiOptions { CreateSchemaReferenceId = (type) => type.Type.Name == "Todo" ? $"{type.Type.Name}Schema" : OpenApiOptions.CreateDefaultSchemaReferenceId(type) };
 
         // Assert
         await VerifyOpenApiDocument(builder, options, document =>
@@ -114,7 +114,7 @@ public async Task GeneratesInlineSchemaForPoco_WithCustomNullId()
 
         // Act
         builder.MapPost("/", (Todo todo) => { });
-        var options = new OpenApiOptions { CreateSchemaReferenceId = (type) => type.Type.Name == "Todo" ? null : $"{type.Type.Name}Schema" };
+        var options = new OpenApiOptions { CreateSchemaReferenceId = (type) => type.Type.Name == "Todo" ? null : OpenApiOptions.CreateDefaultSchemaReferenceId(type) };
 
         // Assert
         await VerifyOpenApiDocument(builder, options, document =>
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentServiceTestsBase.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentServiceTestsBase.cs
@@ -2,7 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Reflection;
-using System.Runtime.InteropServices;
+using System.Text;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting.Server;
 using Microsoft.AspNetCore.Hosting.Server.Features;
@@ -12,15 +12,18 @@
 using Microsoft.AspNetCore.Mvc.ActionConstraints;
 using Microsoft.AspNetCore.Mvc.ApiExplorer;
 using Microsoft.AspNetCore.Mvc.Controllers;
+using Microsoft.AspNetCore.Mvc.Formatters;
 using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.OpenApi;
 using Microsoft.AspNetCore.Routing;
 using Microsoft.AspNetCore.Routing.Constraints;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Options;
+using Microsoft.Net.Http.Headers;
 using Microsoft.OpenApi.Models;
 using Moq;
+using Xunit.Sdk;
 using static Microsoft.AspNetCore.OpenApi.Tests.OpenApiOperationGeneratorTests;
 
 public abstract class OpenApiDocumentServiceTestBase
@@ -53,7 +56,10 @@ internal static OpenApiDocumentService CreateDocumentService(IEndpointRouteBuild
             ApplicationName = nameof(OpenApiDocumentServiceTests)
         };
 
-        var options = new MvcOptions();
+        var options = new MvcOptions
+        {
+            OutputFormatters = { new TestJsonOutputFormatter() }
+        };
         var optionsAccessor = Options.Create(options);
 
         var constraintResolver = new Mock<IInlineConstraintResolver>();
@@ -87,6 +93,22 @@ internal static OpenApiDocumentService CreateDocumentService(IEndpointRouteBuild
         return documentService;
     }
 
+    private class TestJsonOutputFormatter : TextOutputFormatter
+    {
+        public TestJsonOutputFormatter()
+        {
+            SupportedMediaTypes.Add(new MediaTypeHeaderValue("application/json"));
+            SupportedMediaTypes.Add(new MediaTypeHeaderValue("text/json"));
+
+            SupportedEncodings.Add(Encoding.UTF8);
+        }
+
+        public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding)
+        {
+            return Task.FromResult(0);
+        }
+    }
+
     internal static OpenApiDocumentService CreateDocumentService(IEndpointRouteBuilder builder, OpenApiOptions openApiOptions)
     {
         var context = new ApiDescriptionProviderContext([]);
@@ -203,6 +225,14 @@ public ControllerActionDescriptor CreateActionDescriptor(string methodName = nul
         action.RouteValues.Add("controller", "Test");
         action.RouteValues.Add("action", action.MethodInfo.Name);
         action.ActionConstraints = [new HttpMethodActionConstraint(["GET"])];
+        action.EndpointMetadata = [..action.MethodInfo.GetCustomAttributes()];
+        if (controllerType is not null)
+        {
+            foreach (var attribute in controllerType.GetCustomAttributes())
+            {
+                action.EndpointMetadata.Add(attribute);
+            }
+        }
 
         action.Parameters = [];
         foreach (var parameter in action.MethodInfo.GetParameters())
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ResponseSchemas.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ResponseSchemas.cs
@@ -4,6 +4,7 @@
 using System.ComponentModel;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc;
 using Microsoft.OpenApi.Any;
 using Microsoft.OpenApi.Models;
 
@@ -292,8 +293,9 @@ await VerifyOpenApiDocument(builder, document =>
                 property =>
                 {
                     Assert.Equal("value", property.Key);
-                    Assert.Equal("object", property.Value.Type);
-                    Assert.Collection(property.Value.Properties,
+                    var propertyValue = property.Value.GetEffective(document);
+                    Assert.Equal("object", propertyValue.Type);
+                    Assert.Collection(propertyValue.Properties,
                     property =>
                     {
                         Assert.Equal("id", property.Key);
@@ -317,8 +319,9 @@ await VerifyOpenApiDocument(builder, document =>
                 property =>
                 {
                     Assert.Equal("error", property.Key);
-                    Assert.Equal("object", property.Value.Type);
-                    Assert.Collection(property.Value.Properties, property =>
+                    var propertyValue = property.Value.GetEffective(document);
+                    Assert.Equal("object", propertyValue.Type);
+                    Assert.Collection(propertyValue.Properties, property =>
                     {
                         Assert.Equal("code", property.Key);
                         Assert.Equal("integer", property.Value.Type);
@@ -404,8 +407,10 @@ await VerifyOpenApiDocument(builder, document =>
                 property =>
                 {
                     Assert.Equal("todo", property.Key);
-                    Assert.Equal("object", property.Value.Type);
-                    Assert.Collection(property.Value.Properties,
+                    Assert.NotNull(property.Value.Reference);
+                    var propertyValue = property.Value.GetEffective(document);
+                    Assert.Equal("object", propertyValue.Type);
+                    Assert.Collection(propertyValue.Properties,
                         property =>
                         {
                             Assert.Equal("id", property.Key);
@@ -529,8 +534,10 @@ await VerifyOpenApiDocument(builder, document =>
                     Assert.Equal("items", property.Key);
                     Assert.Equal("array", property.Value.Type);
                     Assert.NotNull(property.Value.Items);
-                    Assert.Equal("object", property.Value.Items.Type);
-                    Assert.Collection(property.Value.Items.Properties,
+                    Assert.NotNull(property.Value.Items.Reference);
+                    Assert.Equal("object", property.Value.Items.GetEffective(document).Type);
+                    var itemsValue = property.Value.Items.GetEffective(document);
+                    Assert.Collection(itemsValue.Properties,
                         property =>
                         {
                             Assert.Equal("id", property.Key);
@@ -653,6 +660,53 @@ await VerifyOpenApiDocument(builder, document =>
         });
     }
 
+    [Fact]
+    public async Task GetOpenApiResponse_SupportsProducesWithProducesResponseTypeOnController()
+    {
+        var actionDescriptor = CreateActionDescriptor(nameof(TestController.Get), typeof(TestController));
+
+        await VerifyOpenApiDocument(actionDescriptor, document =>
+        {
+            var operation = document.Paths["/"].Operations[OperationType.Get];
+            var responses = Assert.Single(operation.Responses);
+            var response = responses.Value;
+            Assert.True(response.Content.TryGetValue("application/json", out var mediaType));
+            var schema = mediaType.Schema.GetEffective(document);
+            Assert.Equal("object", schema.Type);
+            Assert.Collection(schema.Properties,
+                property =>
+                {
+                    Assert.Equal("id", property.Key);
+                    Assert.Equal("integer", property.Value.Type);
+                },
+                property =>
+                {
+                    Assert.Equal("title", property.Key);
+                    Assert.Equal("string", property.Value.Type);
+                },
+                property =>
+                {
+                    Assert.Equal("completed", property.Key);
+                    Assert.Equal("boolean", property.Value.Type);
+                },
+                property =>
+                {
+                    Assert.Equal("createdAt", property.Key);
+                    Assert.Equal("string", property.Value.Type);
+                    Assert.Equal("date-time", property.Value.Format);
+                });
+        });
+    }
+
+    [ApiController]
+    [Produces("application/json")]
+    public class TestController
+    {
+        [Route("/")]
+        [ProducesResponseType(typeof(Todo), StatusCodes.Status200OK)]
+        internal Todo Get() => new(1, "Write test", false, DateTime.Now);
+    }
+
     private class ClassWithObjectProperty
     {
         public object Object { get; set; }
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Shared/SharedTypes.DeeplyNestedType.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Shared/SharedTypes.DeeplyNestedType.cs
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs
diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/SchemaTransformerTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/SchemaTransformerTests.cs

Original file line number	Diff line number	Diff line change
`@@ -382,7 +382,7 @@ private async Task<OpenApiResponse> GetResponseAsync(`
`382`	`382`	`.SelectMany(attr => attr.ContentTypes);`
`383`	`383`	`foreach (var contentType in explicitContentTypes)`
`384`	`384`	`{`
`385`		`- response.Content[contentType] = new OpenApiMediaType();`
	`385`	`+ response.Content.TryAdd(contentType, new OpenApiMediaType());`
`386`	`386`	`}`
`387`	`387`
`388`	`388`	`return response;`
Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,7 @@ internal sealed class OpenApiSchemaService(`
`32`	`32`	`IOptionsMonitor<OpenApiOptions> optionsMonitor)`
`33`	`33`	`{`
`34`	`34`	`private readonly OpenApiSchemaStore _schemaStore = serviceProvider.GetRequiredKeyedService<OpenApiSchemaStore>(documentName);`
	`35`	`+ private readonly OpenApiJsonSchemaContext _jsonSchemaContext = new OpenApiJsonSchemaContext(new(jsonOptions.Value.SerializerOptions));`
`35`	`36`	`private readonly JsonSerializerOptions _jsonSerializerOptions = new(jsonOptions.Value.SerializerOptions)`
`36`	`37`	`{`
`37`	`38`	// In order to properly handle the `RequiredAttribute` on type properties, add a modifier to support
`@@ -135,7 +136,9 @@ internal async Task<OpenApiSchema> GetOrCreateSchemaAsync(Type type, IServicePro`
`135`	`136`	`{`
`136`	`137`	`schemaAsJsonObject.ApplyParameterInfo(parameterDescription, _jsonSerializerOptions.GetTypeInfo(type));`
`137`	`138`	`}`
`138`		`- var deserializedSchema = JsonSerializer.Deserialize(schemaAsJsonObject, OpenApiJsonSchemaContext.Default.OpenApiJsonSchema);`
	`139`	`+ // Use _jsonSchemaContext constructed from _jsonSerializerOptions to respect shared config set by end-user,`
	`140`	`+ // particularly in the case of maxDepth.`
	`141`	`+ var deserializedSchema = JsonSerializer.Deserialize(schemaAsJsonObject, _jsonSchemaContext.OpenApiJsonSchema);`
`139`	`142`	`Debug.Assert(deserializedSchema != null, "The schema should have been deserialized successfully and materialize a non-null value.");`
`140`	`143`	`var schema = deserializedSchema.Schema;`
`141`	`144`	`await ApplySchemaTransformersAsync(schema, type, scopedServiceProvider, schemaTransformers, parameterDescription, cancellationToken);`
Original file line number	Diff line number	Diff line change
`@@ -77,34 +77,38 @@ public JsonNode GetOrAdd(OpenApiSchemaKey key, Func<OpenApiSchemaKey, JsonNode>`
`77`	`77`	`/// been encountered more than once in the document to avoid unnecessarily capturing unique`
`78`	`78`	`/// schemas into the top-level document.`
`79`	`79`	`/// </summary>`
	`80`	`+ /// <remarks>`
	`81`	`+ /// We don't do a depth check in the recursion call here since we assume that`
	`82`	`+ /// System.Text.Json has already validate the depth of the schema based on`
	`83`	`+ /// the configured JsonSerializerOptions.MaxDepth value.`
	`84`	`+ /// </remarks>`
`80`	`85`	`/// <param name="schema">The <see cref="OpenApiSchema"/> to add to the schemas-with-references cache.</param>`
`81`	`86`	`/// <param name="captureSchemaByRef"><see langword="true"/> if schema should always be referenced instead of inlined.</param>`
`82`	`87`	`public void PopulateSchemaIntoReferenceCache(OpenApiSchema schema, bool captureSchemaByRef)`
`83`	`88`	`{`
`84`		`- // Only capture top-level schemas by ref. Nested schemas will follow the`
`85`		`- // reference by duplicate rules.`
`86`	`89`	`AddOrUpdateSchemaByReference(schema, captureSchemaByRef: captureSchemaByRef);`
`87`	`90`	`AddOrUpdateAnyOfSubSchemaByReference(schema);`
	`91`	`+`
`88`	`92`	`if (schema.AdditionalProperties is not null)`
`89`	`93`	`{`
`90`		`- AddOrUpdateSchemaByReference(schema.AdditionalProperties);`
	`94`	`+ PopulateSchemaIntoReferenceCache(schema.AdditionalProperties, captureSchemaByRef);`
`91`	`95`	`}`
`92`	`96`	`if (schema.Items is not null)`
`93`	`97`	`{`
`94`		`- AddOrUpdateSchemaByReference(schema.Items);`
	`98`	`+ PopulateSchemaIntoReferenceCache(schema.Items, captureSchemaByRef);`
`95`	`99`	`}`
`96`	`100`	`if (schema.AllOf is not null)`
`97`	`101`	`{`
`98`	`102`	`foreach (var allOfSchema in schema.AllOf)`
`99`	`103`	`{`
`100`		`- AddOrUpdateSchemaByReference(allOfSchema);`
	`104`	`+ PopulateSchemaIntoReferenceCache(allOfSchema, captureSchemaByRef);`
`101`	`105`	`}`
`102`	`106`	`}`
`103`	`107`	`if (schema.Properties is not null)`
`104`	`108`	`{`
`105`	`109`	`foreach (var property in schema.Properties.Values)`
`106`	`110`	`{`
`107`		`- AddOrUpdateSchemaByReference(property);`
	`111`	`+ PopulateSchemaIntoReferenceCache(property, captureSchemaByRef);`
`108`	`112`	`}`
`109`	`113`	`}`
`110`	`114`	`}`