fix: handle nullability more accurately during serialization for 3.0/2.0 (#2933)

* Handle nullability more accurately

* Cleanup

* Rename SerializeTypeProperty to TrySerializeTypeProperty

* Separate rp

* Cleanup

* Progress

* Cleanp

* Refactor

* Use cached json extension

* AddExtension only if Type is still unknown

* Cleanup

* Address review comments

* Address review comments

* Update OpenApiSchemaDeserializer.cs

* Update OpenApiSchemaDeserializer.cs

Author: Youssef1313

src/Microsoft.OpenApi/Models/OpenApiSchema.cs

@@ -21,6 +21,8 @@ namespace Microsoft.OpenApi
     /// </summary>
     public class OpenApiSchema : IOpenApiExtensible, IOpenApiSchema, IOpenApiSchemaMissingProperties, IOpenApiSchemaWithUnevaluatedProperties, IMetadataContainer
     {
+        private static readonly IEnumerable<JsonNode> s_singleNullElementList = [ JsonNullSentinel.JsonNull ];
+
         /// <inheritdoc />
         public string? Title { get; set; }
 
@@ -109,13 +111,8 @@ public string? ExclusiveMinimum
         /// <inheritdoc />
         public JsonSchemaType? Type { get; set; }
 
-        // x-nullable is filtered out by deserializers, but keep the check here in case it gets added from user code.
-        private bool IsNullable =>
-            (Type.HasValue && Type.Value.HasFlag(JsonSchemaType.Null)) ||
-            Extensions is not null &&
-            Extensions.TryGetValue(OpenApiConstants.NullableExtension, out var nullExtRawValue) &&
-            nullExtRawValue is JsonNodeExtension { Node: JsonNode jsonNode } &&
-            jsonNode.GetValueKind() is JsonValueKind.True;
+        private bool HasNullType
+            => Type.HasValue && Type.Value.HasFlag(JsonSchemaType.Null);
 
         /// <inheritdoc />
         public string? Const { get; set; }
@@ -517,57 +514,30 @@ private void SerializeInternal(IOpenApiWriter writer, OpenApiSpecVersion version
                 : Enum;
             writer.WriteOptionalCollection(OpenApiConstants.Enum, enumValue, (nodeWriter, s) => nodeWriter.WriteAny(s));
 
-            // Handle oneOf/anyOf with null type for v3.0 downcast
-            IList<IOpenApiSchema>? effectiveOneOf = OneOf;
-            IList<IOpenApiSchema>? effectiveAnyOf = AnyOf;
-            bool hasNullInComposition = false;
-            bool hasOneOfNullAndSingleEnumWith3_0 = false;
-            JsonSchemaType? inferredType = null;
-
             if (version == OpenApiSpecVersion.OpenApi3_0)
             {
-                (effectiveOneOf, var inferredOneOf, var nullInOneOf) = ProcessCompositionForNull(OneOf);
-                hasNullInComposition |= nullInOneOf;
-                inferredType = inferredOneOf ?? inferredType;
-                (effectiveAnyOf, var inferredAnyOf, var nullInAnyOf) = ProcessCompositionForNull(AnyOf);
-                hasNullInComposition |= nullInAnyOf;
-                inferredType = inferredAnyOf ?? inferredType;
-
-                hasOneOfNullAndSingleEnumWith3_0 = nullInOneOf && effectiveOneOf is { Count: 1 } &&
-                    effectiveOneOf[0].Enum is { Count: > 0 };
+                // If we have a schema that's only just { "type": "null" }, we serialize it as enum with null value.
+                if (Type == JsonSchemaType.Null &&
+                    OneOf is not { Count: > 0 } &&
+                    AnyOf is not { Count: > 0 } &&
+                    AllOf is not { Count: > 0 } &&
+                    Enum is not { Count: > 0 })
+                {
+                    writer.WriteOptionalCollection(OpenApiConstants.Enum, s_singleNullElementList, (nodeWriter, s) => nodeWriter.WriteAny(s));
+                }
             }
 
             // type
-            SerializeTypeProperty(writer, version, inferredType);
+            var serializedTypeProperty = TrySerializeTypeProperty(writer, version);
 
             // allOf
             writer.WriteOptionalCollection(OpenApiConstants.AllOf, AllOf, callback);
 
             // anyOf
-            writer.WriteOptionalCollection(OpenApiConstants.AnyOf, effectiveAnyOf, callback);
+            writer.WriteOptionalCollection(OpenApiConstants.AnyOf, AnyOf, callback);
 
             // oneOf
-            if (hasOneOfNullAndSingleEnumWith3_0)
-            {
-                writer.WriteRequiredCollection(OpenApiConstants.OneOf, effectiveOneOf!, (writer, element) =>
-                {                    
-                    var clonedToMutateEnum = element.CreateShallowCopy();
-                    if (clonedToMutateEnum is OpenApiSchema { Enum: { } existingEnum } concreteCloned)
-                    {
-                        concreteCloned.Enum = [.. existingEnum, JsonNullSentinel.JsonNull];
-                        callback(writer, clonedToMutateEnum);
-                    }
-                    else
-                    {
-                        callback(writer, element);
-                    }
-                });
-            }
-            else
-            {
-                writer.WriteOptionalCollection(OpenApiConstants.OneOf, effectiveOneOf, callback);
-            }
-
+            writer.WriteOptionalCollection(OpenApiConstants.OneOf, OneOf, callback);
 
             // not
             writer.WriteOptionalObject(OpenApiConstants.Not, Not, callback);
@@ -603,9 +573,14 @@ private void SerializeInternal(IOpenApiWriter writer, OpenApiSpecVersion version
             writer.WriteOptionalObject(OpenApiConstants.Default, Default, (w, d) => w.WriteAny(d));
 
             // nullable
-            if (version == OpenApiSpecVersion.OpenApi3_0)
+            if (version == OpenApiSpecVersion.OpenApi3_0 && serializedTypeProperty)
             {
-                SerializeNullable(writer, version, hasNullInComposition);
+                // https://spec.openapis.org/oas/v3.0.4.html#fixed-fields-20
+                // This keyword only takes effect if type is explicitly defined within the same Schema Object.
+                //
+                // If the user explicitly set IsNullable to true, we serialize it even if redundant.
+                // But if **we** are inferring it (from oneOf/anyOf), we don't serialize it when it's redundant.
+                SerializeNullable(writer, version);
             }
 
             // discriminator
@@ -832,7 +807,7 @@ private void SerializeAsV2(
             writer.WriteStartObject();
 
             // type
-            SerializeTypeProperty(writer, OpenApiSpecVersion.OpenApi2_0);
+            TrySerializeTypeProperty(writer, OpenApiSpecVersion.OpenApi2_0);
 
             // description
             writer.WriteProperty(OpenApiConstants.Description, Description);
@@ -995,31 +970,32 @@ private void SerializeAsV2(
             writer.WriteEndObject();
         }
 
-        private void SerializeTypeProperty(IOpenApiWriter writer, OpenApiSpecVersion version, JsonSchemaType? inferredType = null)
+        private bool TrySerializeTypeProperty(IOpenApiWriter writer, OpenApiSpecVersion version, JsonSchemaType? inferredType = null)
         {
             // Use original type or inferred type when the explicit type is not set
             var typeToUse = Type ?? inferredType;
 
             if (typeToUse is null)
             {
-                return;
+                return false;
             }
 
-            var unifiedType = IsNullable ? typeToUse.Value | JsonSchemaType.Null : typeToUse.Value;
-            var typeWithoutNull = unifiedType & ~JsonSchemaType.Null;
-
             switch (version)
             {
                 case OpenApiSpecVersion.OpenApi2_0 or OpenApiSpecVersion.OpenApi3_0:
+                    var typeWithoutNull = typeToUse.Value & ~JsonSchemaType.Null;
                     if (typeWithoutNull != 0 && !HasMultipleTypes(typeWithoutNull))
                     {
                         writer.WriteProperty(OpenApiConstants.Type, typeWithoutNull.ToFirstIdentifier());
+                        return true;
                     }
                     break;
                 default:
-                    WriteUnifiedSchemaType(unifiedType, writer);
-                    break;
+                    WriteUnifiedSchemaType(typeToUse.Value, writer);
+                    return true;
             }
+
+            return false;
         }
 
         private static bool IsPowerOfTwo(int x)
@@ -1054,9 +1030,9 @@ where type.HasFlag(flag)
             }
         }
 
-        private void SerializeNullable(IOpenApiWriter writer, OpenApiSpecVersion version, bool hasNullInComposition = false)
+        private void SerializeNullable(IOpenApiWriter writer, OpenApiSpecVersion version)
         {
-            if (IsNullable || hasNullInComposition)
+            if (HasNullType)
             {
                 switch (version)
                 {
@@ -1070,63 +1046,6 @@ private void SerializeNullable(IOpenApiWriter writer, OpenApiSpecVersion version
             }
         }
 
-        /// <summary>
-        /// Processes a composition (oneOf or anyOf) for null types, filtering out null schemas and inferring common type.
-        /// </summary>
-        /// <param name="composition">The list of schemas in the composition.</param>
-        /// <returns>A tuple with the effective list, inferred type, and whether null is present in composition.</returns>
-        private static (IList<IOpenApiSchema>? effective, JsonSchemaType? inferredType, bool hasNullInComposition)
-            ProcessCompositionForNull(IList<IOpenApiSchema>? composition)
-        {
-            if (composition is null || !composition.Any(static s => s.Type is JsonSchemaType.Null))
-            {
-                // Nothing to patch
-                return (composition, null, false);
-            }
-
-            var nonNullSchemas = composition
-                .Where(static s => s.Type is null or not JsonSchemaType.Null)
-                .ToList();
-
-            if (nonNullSchemas.Count > 0)
-            {
-                JsonSchemaType commonType = 0;
-
-                foreach (var schema in nonNullSchemas)
-                {
-                    if (schema.Type.HasValue)
-                    {
-                        commonType |= schema.Type.Value & ~JsonSchemaType.Null;
-                    }
-                    else if (schema.Enum is { Count: > 0 })
-                    {
-                        foreach (var enumValue in schema.Enum.Where(x => x is not null))
-                        {
-                            var currentType = enumValue.GetValueKind() switch
-                            {
-                                JsonValueKind.Array => JsonSchemaType.Array,
-                                JsonValueKind.String => JsonSchemaType.String,
-                                JsonValueKind.Number => JsonSchemaType.Number,
-                                JsonValueKind.True or JsonValueKind.False => JsonSchemaType.Boolean,
-                                JsonValueKind.Null => (JsonSchemaType)0,
-                                _ => JsonSchemaType.Object,
-                            };
-
-                            commonType |= currentType;
-                        }
-
-                        commonType |= JsonSchemaType.String;
-                    }
-                }
-
-                return (nonNullSchemas, commonType == 0 ? null : commonType, true);
-            }
-            else
-            {
-                return (null, null, true);
-            }
-        }
-
 #if NET5_0_OR_GREATER
         private static readonly Array jsonSchemaTypeValues = System.Enum.GetValues<JsonSchemaType>();
 #else

src/Microsoft.OpenApi/Reader/V2/OpenApiSchemaDeserializer.cs

@@ -1,12 +1,12 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
 
-using System.Text.Json.Nodes;
-
+using System;
 using System.Collections.Generic;
 using System.Globalization;
-using System;
 using System.Linq;
+using System.Text.Json;
+using System.Text.Json.Nodes;
 
 namespace Microsoft.OpenApi.Reader.V2
 {
@@ -16,6 +16,8 @@ namespace Microsoft.OpenApi.Reader.V2
     /// </summary>
     internal static partial class OpenApiV2Deserializer
     {
+        private const string EncounteredNullableExtensionTrueMetadataKey = "encounteredNullable";
+
         private static readonly FixedFieldMap<OpenApiSchema> _openApiSchemaFixedFields = new()
         {
             {
@@ -210,6 +212,29 @@ internal static partial class OpenApiV2Deserializer
                 "default",
                 (o, n, _, _) => o.Default = n
             },
+            {
+                OpenApiConstants.NullableExtension,
+                (o, n, _, _) =>
+                {
+                    if (bool.TryParse(n.GetScalarValue(), out var parsed) && parsed)
+                    {
+                        if (o.Type is not null && o.Type != 0)
+                        {
+                            o.Type |= JsonSchemaType.Null;
+                        }
+                        else
+                        {
+                            // We mirror the same behavior of OpenAPI 3.0 "nullable" which is a type modifier.
+                            // It only has effect if there is a "Type" set.
+                            // Given that we don't have a Type set yet, we want to keep track of it until the end of deserialization.
+                            // If, at a later point during deserialization, Type was set, we apply nullable to it.
+                            // Otherwise, we throw it away.
+                            o.Metadata ??= new Dictionary<string, object>();
+                            o.Metadata[EncounteredNullableExtensionTrueMetadataKey] = true;
+                        }
+                    }
+                }
+            },
             {
                 "discriminator", (o, n, _, _) =>
                 {
@@ -268,14 +293,13 @@ public static IOpenApiSchema LoadSchema(JsonNode node, OpenApiDocument hostDocum
 
             ParseMap(jsonObject, schema, _openApiSchemaFixedFields, _openApiSchemaPatternFields, hostDocument, context);
 
-            if (schema.Extensions is not null && schema.Extensions.ContainsKey(OpenApiConstants.NullableExtension))
+            if (schema.Metadata?.TryGetValue(EncounteredNullableExtensionTrueMetadataKey, out var value) == true)
             {
-                if (schema.Type.HasValue)
+                schema.Metadata.Remove(EncounteredNullableExtensionTrueMetadataKey);
+                if (schema.Type is not null && schema.Type != 0 && value is bool isNullable && isNullable)
+                {
                     schema.Type |= JsonSchemaType.Null;
-                else
-                    schema.Type = JsonSchemaType.Null;
-
-                schema.Extensions.Remove(OpenApiConstants.NullableExtension);
+                }
             }
 
             return schema;

src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs

@@ -1,12 +1,13 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
 
-using System.Text.Json.Nodes;
-
 using System;
 using System.Collections.Generic;
+using System.Data.SqlTypes;
 using System.Globalization;
 using System.Linq;
+using System.Text.Json;
+using System.Text.Json.Nodes;
 
 namespace Microsoft.OpenApi.Reader.V3
 {
@@ -16,6 +17,8 @@ namespace Microsoft.OpenApi.Reader.V3
     /// </summary>
     internal static partial class OpenApiV3Deserializer
     {
+        private const string EncounteredNullableTrueMetadataKey = "encounteredNullable";
+
         private static readonly FixedFieldMap<OpenApiSchema> _openApiSchemaFixedFields = new()
         {
             {
@@ -222,10 +225,20 @@ internal static partial class OpenApiV3Deserializer
                 {
                     if (bool.TryParse(n.GetScalarValue(), out var parsed) && parsed)
                     {
-                        if (o.Type.HasValue)
+                        if (o.Type is not null && o.Type != 0)
+                        {
                             o.Type |= JsonSchemaType.Null;
+                        }
                         else
-                            o.Type = JsonSchemaType.Null;
+                        {
+                            // "nullable" is a type modifier.
+                            // It only has effect if there is a "Type" set.
+                            // Given that we don't have a Type set yet, we want to keep track of it until the end of deserialization.
+                            // If, at a later point during deserialization, Type was set, we apply nullable to it.
+                            // Otherwise, we throw it away.
+                            o.Metadata ??= new Dictionary<string, object>();
+                            o.Metadata[EncounteredNullableTrueMetadataKey] = true;
+                        }
                     }
                 }
             },
@@ -385,14 +398,13 @@ public static IOpenApiSchema LoadSchema(JsonNode node, OpenApiDocument hostDocum
 
             ParseMap(jsonObject, schema, _openApiSchemaFixedFields, _openApiSchemaPatternFields, hostDocument, context);
 
-            if (schema.Extensions is not null && schema.Extensions.ContainsKey(OpenApiConstants.NullableExtension))
+            if (schema.Metadata?.TryGetValue(EncounteredNullableTrueMetadataKey, out var value) == true)
             {
-                if (schema.Type.HasValue)
+                schema.Metadata.Remove(EncounteredNullableTrueMetadataKey);
+                if (schema.Type is not null && schema.Type != 0 && value is bool isNullable && isNullable)
+                {
                     schema.Type |= JsonSchemaType.Null;
-                else
-                    schema.Type = JsonSchemaType.Null;
-
-                schema.Extensions.Remove(OpenApiConstants.NullableExtension);
+                }
             }
 
             return schema;