Merge pull request #3921 from microsoft/bugfix/the-max-patch

the max patch
microsoft · Dec 19, 2023 · a24c27d · a24c27d
2 parents 2f49fac + 285cf23
commit a24c27d
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Fixed a bug where the discriminator validation rule would report false positives on nullable union types.
+
 ## [1.9.1] - 2023-12-13
 
 ### Added

diff --git a/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs b/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs
@@ -2,7 +2,7 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Linq;
-
+using Microsoft.OpenApi.Any;
 using Microsoft.OpenApi.Models;
 
 namespace Kiota.Builder.Extensions;
@@ -60,8 +60,8 @@ public static bool IsArray(this OpenApiSchema? schema)
         return "array".Equals(schema?.Type, StringComparison.OrdinalIgnoreCase) && schema.Items != null &&
             (schema.Items.IsComposedEnum() ||
             schema.Items.IsEnum() ||
-            IsSemanticallyMeaningful(schema.Items) ||
-            FlattenEmptyEntries(new OpenApiSchema[] { schema.Items }, static x => x.AnyOf.Union(x.AllOf).Union(x.OneOf).ToList(), 1).FirstOrDefault() is OpenApiSchema flat && IsSemanticallyMeaningful(flat));
+            schema.Items.IsSemanticallyMeaningful() ||
+            FlattenEmptyEntries(new OpenApiSchema[] { schema.Items }, static x => x.AnyOf.Union(x.AllOf).Union(x.OneOf).ToList(), 1).FirstOrDefault() is OpenApiSchema flat && flat.IsSemanticallyMeaningful());
     }
 
     public static bool IsObject(this OpenApiSchema? schema)
@@ -70,13 +70,14 @@ public static bool IsObject(this OpenApiSchema? schema)
     }
     public static bool IsInclusiveUnion(this OpenApiSchema? schema)
     {
-        return schema?.AnyOf?.Count(IsSemanticallyMeaningful) > 1;
+        return schema?.AnyOf?.Count(static x => IsSemanticallyMeaningful(x, true)) > 1;
+        // so we don't consider any of object/nullable as a union type
     }
 
     public static bool IsInherited(this OpenApiSchema? schema)
     {
         if (schema is null) return false;
-        var meaningfulSchemas = schema.AllOf.FlattenSchemaIfRequired(static x => x.AllOf).Where(IsSemanticallyMeaningful).ToArray();
+        var meaningfulSchemas = schema.AllOf.FlattenSchemaIfRequired(static x => x.AllOf).Where(static x => x.IsSemanticallyMeaningful()).ToArray();
         return meaningfulSchemas.Count(static x => !string.IsNullOrEmpty(x.Reference?.Id)) == 1 && meaningfulSchemas.Count(static x => string.IsNullOrEmpty(x.Reference?.Id)) == 1;
     }
 
@@ -86,20 +87,21 @@ public static bool IsInherited(this OpenApiSchema? schema)
         if (!schema.IsIntersection()) return schema;
         var result = new OpenApiSchema(schema);
         result.AllOf.Clear();
-        var meaningfulSchemas = schema.AllOf.Where(IsSemanticallyMeaningful).Select(MergeIntersectionSchemaEntries).Where(x => x is not null).OfType<OpenApiSchema>();
+        var meaningfulSchemas = schema.AllOf.Where(static x => x.IsSemanticallyMeaningful()).Select(MergeIntersectionSchemaEntries).Where(x => x is not null).OfType<OpenApiSchema>();
         meaningfulSchemas.SelectMany(static x => x.Properties).ToList().ForEach(x => result.Properties.TryAdd(x.Key, x.Value));
         return result;
     }
 
     public static bool IsIntersection(this OpenApiSchema? schema)
     {
-        var meaningfulSchemas = schema?.AllOf?.Where(IsSemanticallyMeaningful);
+        var meaningfulSchemas = schema?.AllOf?.Where(static x => x.IsSemanticallyMeaningful());
         return meaningfulSchemas?.Count() > 3 || meaningfulSchemas?.Count(static x => !string.IsNullOrEmpty(x.Reference?.Id)) > 1 || meaningfulSchemas?.Count(static x => string.IsNullOrEmpty(x.Reference?.Id)) > 1;
     }
 
     public static bool IsExclusiveUnion(this OpenApiSchema? schema)
     {
-        return schema?.OneOf?.Count(IsSemanticallyMeaningful) > 1;
+        return schema?.OneOf?.Count(static x => IsSemanticallyMeaningful(x, true)) > 1;
+        // so we don't consider one of object/nullable as a union type
     }
     private static readonly HashSet<string> oDataTypes = new(StringComparer.OrdinalIgnoreCase) {
         "number",
@@ -121,15 +123,26 @@ public static bool IsODataPrimitiveType(this OpenApiSchema schema)
     }
     public static bool IsEnum(this OpenApiSchema schema)
     {
-        return (schema?.Enum?.Any() ?? false) && (string.IsNullOrEmpty(schema.Type) || "string".Equals(schema.Type, StringComparison.OrdinalIgnoreCase)); // number and boolean enums are not supported
+        if (schema is null) return false;
+        return schema.Enum.OfType<OpenApiString>().Any(static x => !string.IsNullOrEmpty(x.Value)) &&
+                (string.IsNullOrEmpty(schema.Type) || "string".Equals(schema.Type, StringComparison.OrdinalIgnoreCase)) ||
+                schema.AnyOf.Where(static x => x.IsSemanticallyMeaningful(true)).Count(static x => x.IsEnum()) == 1 && !schema.AnyOf.Where(static x => x.IsSemanticallyMeaningful(true)).Any(static x => !x.IsEnum()) ||
+                schema.OneOf.Where(static x => x.IsSemanticallyMeaningful(true)).Count(static x => x.IsEnum()) == 1 && !schema.OneOf.Where(static x => x.IsSemanticallyMeaningful(true)).Any(static x => !x.IsEnum())
+        ; // number and boolean enums are not supported
     }
     public static bool IsComposedEnum(this OpenApiSchema schema)
     {
         return (schema.IsInclusiveUnion() && schema.AnyOf.Any(static x => x.IsEnum())) || (schema.IsExclusiveUnion() && schema.OneOf.Any(static x => x.IsEnum()));
     }
-    private static bool IsSemanticallyMeaningful(this OpenApiSchema schema)
+    private static bool IsSemanticallyMeaningful(this OpenApiSchema schema, bool ignoreNullableObjects = false)
     {
-        return schema.Properties.Any() || schema.Items != null || !string.IsNullOrEmpty(schema.Type) || !string.IsNullOrEmpty(schema.Format) || !string.IsNullOrEmpty(schema.Reference?.Id);
+        return schema.Properties.Any() ||
+                schema.Items != null ||
+                (!string.IsNullOrEmpty(schema.Type) &&
+                    ((ignoreNullableObjects && !"object".Equals(schema.Type, StringComparison.OrdinalIgnoreCase)) ||
+                    !ignoreNullableObjects)) ||
+                !string.IsNullOrEmpty(schema.Format) ||
+                !string.IsNullOrEmpty(schema.Reference?.Id);
     }
     public static IEnumerable<string> GetSchemaReferenceIds(this OpenApiSchema schema, HashSet<OpenApiSchema>? visitedSchemas = null)
     {

diff --git a/tests/Kiota.Builder.Tests/Extensions/OpenApiSchemaExtensionsTests.cs b/tests/Kiota.Builder.Tests/Extensions/OpenApiSchemaExtensionsTests.cs
@@ -2,7 +2,7 @@
 using System.Collections.Generic;
 
 using Kiota.Builder.Extensions;
-
+using Microsoft.OpenApi.Any;
 using Microsoft.OpenApi.Models;
 
 using Xunit;
@@ -515,4 +515,123 @@ public void IsArrayFalseOnNullItems()
         };
         Assert.False(schema.IsArray());
     }
+    [Fact]
+    public void IsEnumFailsOnEmptyMembers()
+    {
+        var schema = new OpenApiSchema
+        {
+            Type = "string",
+            Enum = new List<IOpenApiAny>(),
+        };
+        Assert.False(schema.IsEnum());
+
+        schema.Enum.Add(new OpenApiString(""));
+        Assert.False(schema.IsEnum());
+    }
+    private static readonly OpenApiSchema enumSchema = new OpenApiSchema
+    {
+        Title = "riskLevel",
+        Enum = new List<IOpenApiAny>
+            {
+                new OpenApiString("low"),
+                new OpenApiString("medium"),
+                new OpenApiString("high"),
+                new OpenApiString("hidden"),
+                new OpenApiString("none"),
+                new OpenApiString("unknownFutureValue")
+            },
+        Type = "string"
+    };
+    [Fact]
+    public void IsEnumIgnoresNullableUnions()
+    {
+        var schema = new OpenApiSchema
+        {
+            AnyOf = new List<OpenApiSchema>
+            {
+                enumSchema,
+                new OpenApiSchema
+                {
+                    Type = "object",
+                    Nullable = true
+                }
+            }
+        };
+        Assert.True(schema.IsEnum());
+    }
+    [Fact]
+    public void IsEnumFailsOnNullableInheritance()
+    {
+        var schema = new OpenApiSchema
+        {
+            AllOf = new List<OpenApiSchema>
+            {
+                enumSchema,
+                new OpenApiSchema
+                {
+                    Type = "object",
+                    Nullable = true
+                }
+            }
+        };
+        Assert.False(schema.IsEnum());
+    }
+    [Fact]
+    public void IsEnumIgnoresNullableExclusiveUnions()
+    {
+        var schema = new OpenApiSchema
+        {
+            OneOf = new List<OpenApiSchema>
+            {
+                enumSchema,
+                new OpenApiSchema
+                {
+                    Type = "object",
+                    Nullable = true
+                }
+            }
+        };
+        Assert.True(schema.IsEnum());
+    }
+    private static readonly OpenApiSchema numberSchema = new OpenApiSchema
+    {
+        Type = "number",
+        Format = "double",
+    };
+    [Fact]
+    public void IsEnumDoesNotMaskExclusiveUnions()
+    {
+        var schema = new OpenApiSchema
+        {
+            OneOf = new List<OpenApiSchema>
+            {
+                enumSchema,
+                numberSchema,
+                new OpenApiSchema
+                {
+                    Type = "object",
+                    Nullable = true
+                }
+            }
+        };
+        Assert.False(schema.IsEnum());
+    }
+    [Fact]
+    public void IsEnumDoesNotMaskUnions()
+    {
+        var schema = new OpenApiSchema
+        {
+            AnyOf = new List<OpenApiSchema>
+            {
+                enumSchema,
+                numberSchema,
+                new OpenApiSchema
+                {
+                    Type = "object",
+                    Nullable = true
+                }
+            }
+        };
+        Assert.False(schema.IsEnum());
+    }
 }