Fix up nullability and primitive type handing in schema generation (#56372)

captainsafia · web-flow · commit 4c8b5fefd708 · 2024-06-26T10:07:38.000-07:00
* Fix up nullability and primitive type handing in schema generation

* Guard against unsupported NIC and fix object schemas

* Address feedback

* Move primitive types to constants
diff --git a/src/OpenApi/src/Extensions/JsonNodeSchemaExtensions.cs b/src/OpenApi/src/Extensions/JsonNodeSchemaExtensions.cs
@@ -24,8 +24,6 @@ namespace Microsoft.AspNetCore.OpenApi;
 /// </summary>
 internal static class JsonNodeSchemaExtensions
 {
-    private static readonly NullabilityInfoContext _nullabilityInfoContext = new();
-
     private static readonly Dictionary<Type, OpenApiSchema> _simpleTypeToOpenApiSchema = new()
     {
         [typeof(bool)] = new() { Type = "boolean" },
@@ -350,7 +348,10 @@ internal static void ApplyPolymorphismOptions(this JsonNode schema, JsonSchemaEx
     /// <param name="context">The <see cref="JsonSchemaExporterContext"/> associated with the current type.</param>
     internal static void ApplySchemaReferenceId(this JsonNode schema, JsonSchemaExporterContext context)
     {
-        schema[OpenApiConstants.SchemaId] = context.TypeInfo.GetSchemaReferenceId();
+        if (context.TypeInfo.GetSchemaReferenceId() is { } schemaReferenceId)
+        {
+            schema[OpenApiConstants.SchemaId] = schemaReferenceId;
+        }
     }
 
     /// <summary>
@@ -365,7 +366,8 @@ internal static void ApplyNullabilityContextInfo(this JsonNode schema, Parameter
             return;
         }
 
-        var nullabilityInfo = _nullabilityInfoContext.Create(parameterInfo);
+        var nullabilityInfoContext = new NullabilityInfoContext();
+        var nullabilityInfo = nullabilityInfoContext.Create(parameterInfo);
         if (nullabilityInfo.WriteState == NullabilityState.Nullable)
         {
             schema[OpenApiSchemaKeywords.NullableKeyword] = true;
@@ -376,16 +378,12 @@ internal static void ApplyNullabilityContextInfo(this JsonNode schema, Parameter
     /// Support applying nullability status for reference types provided as a property or field.
     /// </summary>
     /// <param name="schema">The <see cref="JsonNode"/> produced by the underlying schema generator.</param>
-    /// <param name="attributeProvider">The <see cref="PropertyInfo" /> or <see cref="FieldInfo"/> associated with the schema.</param>
-    internal static void ApplyNullabilityContextInfo(this JsonNode schema, ICustomAttributeProvider attributeProvider)
+    /// <param name="propertyInfo">The <see cref="JsonPropertyInfo" /> associated with the schema.</param>
+    internal static void ApplyNullabilityContextInfo(this JsonNode schema, JsonPropertyInfo propertyInfo)
     {
-        var nullabilityInfo = attributeProvider switch
-        {
-            PropertyInfo propertyInfo => !propertyInfo.PropertyType.IsValueType ? _nullabilityInfoContext.Create(propertyInfo) : null,
-            FieldInfo fieldInfo => !fieldInfo.FieldType.IsValueType ? _nullabilityInfoContext.Create(fieldInfo) : null,
-            _ => null
-        };
-        if (nullabilityInfo is { WriteState: NullabilityState.Nullable } or { ReadState: NullabilityState.Nullable })
+        // Avoid setting explicit nullability annotations for `object` types so they continue to match on the catch
+        // all schema (no type, no format, no constraints).
+        if (propertyInfo.PropertyType != typeof(object) && (propertyInfo.IsGetNullable || propertyInfo.IsSetNullable))
         {
             schema[OpenApiSchemaKeywords.NullableKeyword] = true;
         }
diff --git a/src/OpenApi/src/Extensions/JsonTypeInfoExtensions.cs b/src/OpenApi/src/Extensions/JsonTypeInfoExtensions.cs
@@ -45,10 +45,19 @@ internal static class JsonTypeInfoExtensions
     /// generated reference ID.
     /// </summary>
     /// <param name="jsonTypeInfo">The <see cref="JsonTypeInfo"/> associated with the target schema.</param>
+    /// <param name="isTopLevel">
+    /// When <see langword="false" />, returns schema name for primitive
+    /// types to support use in list/dictionary types.
+    /// </param>
     /// <returns>The schema reference ID represented as a string name.</returns>
-    internal static string GetSchemaReferenceId(this JsonTypeInfo jsonTypeInfo)
+    internal static string? GetSchemaReferenceId(this JsonTypeInfo jsonTypeInfo, bool isTopLevel = true)
     {
         var type = jsonTypeInfo.Type;
+        if (isTopLevel && OpenApiConstants.PrimitiveTypes.Contains(type))
+        {
+            return null;
+        }
+
         // Short-hand if the type we're generating a schema reference ID for is
         // one of the simple types defined above.
         if (_simpleTypeToName.TryGetValue(type, out var simpleName))
@@ -59,14 +68,14 @@ internal static string GetSchemaReferenceId(this JsonTypeInfo jsonTypeInfo)
         if (jsonTypeInfo is JsonTypeInfo { Kind: JsonTypeInfoKind.Enumerable, ElementType: { } elementType })
         {
             var elementTypeInfo = jsonTypeInfo.Options.GetTypeInfo(elementType);
-            return $"ArrayOf{elementTypeInfo.GetSchemaReferenceId()}";
+            return $"ArrayOf{elementTypeInfo.GetSchemaReferenceId(isTopLevel: false)}";
         }
 
         if (jsonTypeInfo is JsonTypeInfo { Kind: JsonTypeInfoKind.Dictionary, KeyType: { } keyType, ElementType: { } valueType })
         {
             var keyTypeInfo = jsonTypeInfo.Options.GetTypeInfo(keyType);
             var valueTypeInfo = jsonTypeInfo.Options.GetTypeInfo(valueType);
-            return $"DictionaryOf{keyTypeInfo.GetSchemaReferenceId()}And{valueTypeInfo.GetSchemaReferenceId()}";
+            return $"DictionaryOf{keyTypeInfo.GetSchemaReferenceId(isTopLevel: false)}And{valueTypeInfo.GetSchemaReferenceId(isTopLevel: false)}";
         }
 
         return type.GetSchemaReferenceId(jsonTypeInfo.Options);
@@ -86,7 +95,7 @@ internal static string GetSchemaReferenceId(this Type type, JsonSerializerOption
         if (type.IsArray && type.GetElementType() is { } elementType)
         {
             var elementTypeInfo = options.GetTypeInfo(elementType);
-            return $"ArrayOf{elementTypeInfo.GetSchemaReferenceId()}";
+            return $"ArrayOf{elementTypeInfo.GetSchemaReferenceId(isTopLevel: false)}";
         }
 
         // Special handling for anonymous types
@@ -98,23 +107,14 @@ internal static string GetSchemaReferenceId(this Type type, JsonSerializerOption
             return $"{typeName}Of{propertyNames}";
         }
 
+        // Special handling for generic types that are collections
+        // Generic types become a concatenation of the generic type name and the type arguments
         if (type.IsGenericType)
         {
-            // Nullable types are suffixed with `?` (e.g. `Todo?`)
-            if (type.GetGenericTypeDefinition() == typeof(Nullable<>)
-                && Nullable.GetUnderlyingType(type) is { } underlyingType)
-            {
-                return $"{underlyingType.GetSchemaReferenceId(options)}?";
-            }
-            // Special handling for generic types that are collections
-            // Generic types become a concatenation of the generic type name and the type arguments
-            else
-            {
-                var genericTypeName = type.Name[..type.Name.LastIndexOf('`')];
-                var genericArguments = type.GetGenericArguments();
-                var argumentNames = string.Join("And", genericArguments.Select(arg => arg.GetSchemaReferenceId(options)));
-                return $"{genericTypeName}Of{argumentNames}";
-            }
+            var genericTypeName = type.Name[..type.Name.LastIndexOf('`')];
+            var genericArguments = type.GetGenericArguments();
+            var argumentNames = string.Join("And", genericArguments.Select(arg => arg.GetSchemaReferenceId(options)));
+            return $"{genericTypeName}Of{argumentNames}";
         }
         return type.Name;
     }
diff --git a/src/OpenApi/src/Services/OpenApiConstants.cs b/src/OpenApi/src/Services/OpenApiConstants.cs
@@ -27,4 +27,39 @@ internal static class OpenApiConstants
         OperationType.Patch,
         OperationType.Trace
     ];
+    // Represents primitive types that should never be represented as
+    // a schema reference and always inlined.
+    internal static readonly List<Type> PrimitiveTypes =
+    [
+        typeof(bool),
+        typeof(byte),
+        typeof(sbyte),
+        typeof(byte[]),
+        typeof(string),
+        typeof(int),
+        typeof(uint),
+        typeof(nint),
+        typeof(nuint),
+        typeof(Int128),
+        typeof(UInt128),
+        typeof(long),
+        typeof(ulong),
+        typeof(float),
+        typeof(double),
+        typeof(decimal),
+        typeof(Half),
+        typeof(ulong),
+        typeof(short),
+        typeof(ushort),
+        typeof(char),
+        typeof(object),
+        typeof(DateTime),
+        typeof(DateTimeOffset),
+        typeof(TimeOnly),
+        typeof(DateOnly),
+        typeof(TimeSpan),
+        typeof(Guid),
+        typeof(Uri),
+        typeof(Version)
+    ];
 }
diff --git a/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs b/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs
@@ -81,11 +81,18 @@ internal sealed class OpenApiSchemaService(
                     }
                 };
             }
+            // STJ uses `true` in place of an empty object to represent a schema that matches
+            // anything. We override this default behavior here to match the style traditionally
+            // expected in OpenAPI documents.
+            if (type == typeof(object))
+            {
+                schema = new JsonObject();
+            }
             schema.ApplyPrimitiveTypesAndFormats(context);
             schema.ApplySchemaReferenceId(context);
-            if (context.PropertyInfo is { AttributeProvider: { } attributeProvider })
+            if (context.PropertyInfo is { AttributeProvider: { } attributeProvider } jsonPropertyInfo)
             {
-                schema.ApplyNullabilityContextInfo(attributeProvider);
+                schema.ApplyNullabilityContextInfo(jsonPropertyInfo);
                 if (attributeProvider.GetCustomAttributes(inherit: false).OfType<ValidationAttribute>() is { } validationAttributes)
                 {
                     schema.ApplyValidationAttributes(validationAttributes);
diff --git a/src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs b/src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs
@@ -144,9 +144,8 @@ private void AddOrUpdateSchemaByReference(OpenApiSchema schema)
             // }
             // In this case, although the reference ID  based on the .NET type we would use is `string`, the
             // two schemas are distinct.
-            if (referenceId == null)
+            if (referenceId == null && GetSchemaReferenceId(schema) is { } targetReferenceId)
             {
-                var targetReferenceId = GetSchemaReferenceId(schema);
                 if (_referenceIdCounter.TryGetValue(targetReferenceId, out var counter))
                 {
                     counter++;
@@ -166,14 +165,14 @@ private void AddOrUpdateSchemaByReference(OpenApiSchema schema)
         }
     }
 
-    private static string GetSchemaReferenceId(OpenApiSchema schema)
+    private static string? GetSchemaReferenceId(OpenApiSchema schema)
     {
         if (schema.Extensions.TryGetValue(OpenApiConstants.SchemaId, out var referenceIdAny)
             && referenceIdAny is OpenApiString { Value: string referenceId })
         {
             return referenceId;
         }
 
-        throw new InvalidOperationException("The schema reference ID must be set on the schema.");
+        return null;
     }
 }
diff --git a/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt b/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt
@@ -25,7 +25,8 @@
             "in": "path",
             "required": true,
             "schema": {
-              "$ref": "#/components/schemas/string2"
+              "minLength": 5,
+              "type": "string"
             }
           }
         ],
@@ -35,17 +36,17 @@
             "content": {
               "text/plain": {
                 "schema": {
-                  "$ref": "#/components/schemas/string"
+                  "type": "string"
                 }
               },
               "application/json": {
                 "schema": {
-                  "$ref": "#/components/schemas/string"
+                  "type": "string"
                 }
               },
               "text/json": {
                 "schema": {
-                  "$ref": "#/components/schemas/string"
+                  "type": "string"
                 }
               }
             }
@@ -65,10 +66,12 @@
                 "type": "object",
                 "properties": {
                   "Title": {
-                    "$ref": "#/components/schemas/string2"
+                    "minLength": 5,
+                    "type": "string"
                   },
                   "Description": {
-                    "$ref": "#/components/schemas/string2"
+                    "minLength": 5,
+                    "type": "string"
                   },
                   "IsCompleted": {
                     "type": "boolean"
@@ -92,17 +95,7 @@
       }
     }
   },
-  "components": {
-    "schemas": {
-      "string": {
-        "type": "string"
-      },
-      "string2": {
-        "minLength": 5,
-        "type": "string"
-      }
-    }
-  },
+  "components": { },
   "tags": [
     {
       "name": "Test"
diff --git a/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=forms.verified.txt b/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=forms.verified.txt
@@ -200,13 +200,6 @@
   },
   "components": {
     "schemas": {
-      "boolean": {
-        "type": "boolean"
-      },
-      "DateTime": {
-        "type": "string",
-        "format": "date-time"
-      },
       "IFormFile": {
         "type": "string",
         "format": "binary"
@@ -217,13 +210,6 @@
           "$ref": "#/components/schemas/IFormFile"
         }
       },
-      "int": {
-        "type": "integer",
-        "format": "int32"
-      },
-      "string": {
-        "type": "string"
-      },
       "Todo": {
         "required": [
           "id",
@@ -234,16 +220,18 @@
         "type": "object",
         "properties": {
           "id": {
-            "$ref": "#/components/schemas/int"
+            "type": "integer",
+            "format": "int32"
           },
           "title": {
-            "$ref": "#/components/schemas/string"
+            "type": "string"
           },
           "completed": {
-            "$ref": "#/components/schemas/boolean"
+            "type": "boolean"
           },
           "createdAt": {
-            "$ref": "#/components/schemas/DateTime"
+            "type": "string",
+            "format": "date-time"
           }
         }
       }
diff --git a/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=responses.verified.txt b/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=responses.verified.txt
@@ -89,20 +89,6 @@
   },
   "components": {
     "schemas": {
-      "boolean": {
-        "type": "boolean"
-      },
-      "DateTime": {
-        "type": "string",
-        "format": "date-time"
-      },
-      "int": {
-        "type": "integer",
-        "format": "int32"
-      },
-      "string": {
-        "type": "string"
-      },
       "Todo": {
         "required": [
           "id",
@@ -113,16 +99,18 @@
         "type": "object",
         "properties": {
           "id": {
-            "$ref": "#/components/schemas/int"
+            "type": "integer",
+            "format": "int32"
           },
           "title": {
-            "$ref": "#/components/schemas/string"
+            "type": "string"
           },
           "completed": {
-            "$ref": "#/components/schemas/boolean"
+            "type": "boolean"
           },
           "createdAt": {
-            "$ref": "#/components/schemas/DateTime"
+            "type": "string",
+            "format": "date-time"
           }
         }
       }
diff --git a/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=schemas-by-ref.verified.txt b/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=schemas-by-ref.verified.txt
diff --git a/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=v1.verified.txt b/src/OpenApi/test/Integration/snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=v1.verified.txt
diff --git a/src/OpenApi/test/Services/OpenApiSchemaService/OpenApiComponentService.ResponseSchemas.cs b/src/OpenApi/test/Services/OpenApiSchemaService/OpenApiComponentService.ResponseSchemas.cs
diff --git a/src/OpenApi/test/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs b/src/OpenApi/test/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs

Original file line number	Diff line number	Diff line change
`@@ -24,8 +24,6 @@ namespace Microsoft.AspNetCore.OpenApi;`
`24`	`24`	`/// </summary>`
`25`	`25`	`internal static class JsonNodeSchemaExtensions`
`26`	`26`	`{`
`27`		`- private static readonly NullabilityInfoContext _nullabilityInfoContext = new();`
`28`		`-`
`29`	`27`	`private static readonly Dictionary<Type, OpenApiSchema> _simpleTypeToOpenApiSchema = new()`
`30`	`28`	`{`
`31`	`29`	`[typeof(bool)] = new() { Type = "boolean" },`
`@@ -350,7 +348,10 @@ internal static void ApplyPolymorphismOptions(this JsonNode schema, JsonSchemaEx`
`350`	`348`	`/// <param name="context">The <see cref="JsonSchemaExporterContext"/> associated with the current type.</param>`
`351`	`349`	`internal static void ApplySchemaReferenceId(this JsonNode schema, JsonSchemaExporterContext context)`
`352`	`350`	`{`
`353`		`- schema[OpenApiConstants.SchemaId] = context.TypeInfo.GetSchemaReferenceId();`
	`351`	`+ if (context.TypeInfo.GetSchemaReferenceId() is { } schemaReferenceId)`
	`352`	`+ {`
	`353`	`+ schema[OpenApiConstants.SchemaId] = schemaReferenceId;`
	`354`	`+ }`
`354`	`355`	`}`
`355`	`356`
`356`	`357`	`/// <summary>`
`@@ -365,7 +366,8 @@ internal static void ApplyNullabilityContextInfo(this JsonNode schema, Parameter`
`365`	`366`	`return;`
`366`	`367`	`}`
`367`	`368`
`368`		`- var nullabilityInfo = _nullabilityInfoContext.Create(parameterInfo);`
	`369`	`+ var nullabilityInfoContext = new NullabilityInfoContext();`
	`370`	`+ var nullabilityInfo = nullabilityInfoContext.Create(parameterInfo);`
`369`	`371`	`if (nullabilityInfo.WriteState == NullabilityState.Nullable)`
`370`	`372`	`{`
`371`	`373`	`schema[OpenApiSchemaKeywords.NullableKeyword] = true;`
`@@ -376,16 +378,12 @@ internal static void ApplyNullabilityContextInfo(this JsonNode schema, Parameter`
`376`	`378`	`/// Support applying nullability status for reference types provided as a property or field.`
`377`	`379`	`/// </summary>`
`378`	`380`	`/// <param name="schema">The <see cref="JsonNode"/> produced by the underlying schema generator.</param>`
`379`		`- /// <param name="attributeProvider">The <see cref="PropertyInfo" /> or <see cref="FieldInfo"/> associated with the schema.</param>`
`380`		`- internal static void ApplyNullabilityContextInfo(this JsonNode schema, ICustomAttributeProvider attributeProvider)`
	`381`	`+ /// <param name="propertyInfo">The <see cref="JsonPropertyInfo" /> associated with the schema.</param>`
	`382`	`+ internal static void ApplyNullabilityContextInfo(this JsonNode schema, JsonPropertyInfo propertyInfo)`
`381`	`383`	`{`
`382`		`- var nullabilityInfo = attributeProvider switch`
`383`		`- {`
`384`		`- PropertyInfo propertyInfo => !propertyInfo.PropertyType.IsValueType ? _nullabilityInfoContext.Create(propertyInfo) : null,`
`385`		`- FieldInfo fieldInfo => !fieldInfo.FieldType.IsValueType ? _nullabilityInfoContext.Create(fieldInfo) : null,`
`386`		`- _ => null`
`387`		`- };`
`388`		`- if (nullabilityInfo is { WriteState: NullabilityState.Nullable } or { ReadState: NullabilityState.Nullable })`
	`384`	+ // Avoid setting explicit nullability annotations for `object` types so they continue to match on the catch
	`385`	`+ // all schema (no type, no format, no constraints).`
	`386`	`+ if (propertyInfo.PropertyType != typeof(object) && (propertyInfo.IsGetNullable \|\| propertyInfo.IsSetNullable))`
`389`	`387`	`{`
`390`	`388`	`schema[OpenApiSchemaKeywords.NullableKeyword] = true;`
`391`	`389`	`}`
Original file line number	Diff line number	Diff line change
`@@ -81,11 +81,18 @@ internal sealed class OpenApiSchemaService(`
`81`	`81`	`}`
`82`	`82`	`};`
`83`	`83`	`}`
	`84`	+ // STJ uses `true` in place of an empty object to represent a schema that matches
	`85`	`+ // anything. We override this default behavior here to match the style traditionally`
	`86`	`+ // expected in OpenAPI documents.`
	`87`	`+ if (type == typeof(object))`
	`88`	`+ {`
	`89`	`+ schema = new JsonObject();`
	`90`	`+ }`
`84`	`91`	`schema.ApplyPrimitiveTypesAndFormats(context);`
`85`	`92`	`schema.ApplySchemaReferenceId(context);`
`86`		`- if (context.PropertyInfo is { AttributeProvider: { } attributeProvider })`
	`93`	`+ if (context.PropertyInfo is { AttributeProvider: { } attributeProvider } jsonPropertyInfo)`
`87`	`94`	`{`
`88`		`- schema.ApplyNullabilityContextInfo(attributeProvider);`
	`95`	`+ schema.ApplyNullabilityContextInfo(jsonPropertyInfo);`
`89`	`96`	`if (attributeProvider.GetCustomAttributes(inherit: false).OfType<ValidationAttribute>() is { } validationAttributes)`
`90`	`97`	`{`
`91`	`98`	`schema.ApplyValidationAttributes(validationAttributes);`
Original file line number	Diff line number	Diff line change
`@@ -144,9 +144,8 @@ private void AddOrUpdateSchemaByReference(OpenApiSchema schema)`
`144`	`144`	`// }`
`145`	`145`	// In this case, although the reference ID based on the .NET type we would use is `string`, the
`146`	`146`	`// two schemas are distinct.`
`147`		`- if (referenceId == null)`
	`147`	`+ if (referenceId == null && GetSchemaReferenceId(schema) is { } targetReferenceId)`
`148`	`148`	`{`
`149`		`- var targetReferenceId = GetSchemaReferenceId(schema);`
`150`	`149`	`if (_referenceIdCounter.TryGetValue(targetReferenceId, out var counter))`
`151`	`150`	`{`
`152`	`151`	`counter++;`
`@@ -166,14 +165,14 @@ private void AddOrUpdateSchemaByReference(OpenApiSchema schema)`
`166`	`165`	`}`
`167`	`166`	`}`
`168`	`167`
`169`		`- private static string GetSchemaReferenceId(OpenApiSchema schema)`
	`168`	`+ private static string? GetSchemaReferenceId(OpenApiSchema schema)`
`170`	`169`	`{`
`171`	`170`	`if (schema.Extensions.TryGetValue(OpenApiConstants.SchemaId, out var referenceIdAny)`
`172`	`171`	`&& referenceIdAny is OpenApiString { Value: string referenceId })`
`173`	`172`	`{`
`174`	`173`	`return referenceId;`
`175`	`174`	`}`
`176`	`175`
`177`		`- throw new InvalidOperationException("The schema reference ID must be set on the schema.");`
	`176`	`+ return null;`
`178`	`177`	`}`
`179`	`178`	`}`