From 475df3c8bcf4ef794a41cc5ce73c63436d512a84 Mon Sep 17 00:00:00 2001 From: Camille Barneaud <1693643+gadcam@users.noreply.github.com> Date: Tue, 19 Mar 2024 00:57:01 +0100 Subject: [PATCH] Discriminator - Ensure warning shows up in recursive case --- .../Extensions/OpenApiSchemaExtensions.cs | 22 +++++++++- .../Validation/MissingDiscriminator.cs | 39 +++++++++++++++-- .../Validation/MissingDiscriminatorTests.cs | 42 +++++++++++++++++++ 3 files changed, 99 insertions(+), 4 deletions(-) diff --git a/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs b/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs index 4c705bdeeb..fb8b362708 100644 --- a/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs +++ b/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs @@ -6,10 +6,12 @@ using Microsoft.OpenApi.Models; namespace Kiota.Builder.Extensions; + public static class OpenApiSchemaExtensions { private static readonly Func> classNamesFlattener = x => (x.AnyOf ?? Enumerable.Empty()).Union(x.AllOf).Union(x.OneOf).ToList(); + public static IEnumerable GetSchemaNames(this OpenApiSchema schema) { if (schema == null) @@ -30,6 +32,7 @@ public static IEnumerable GetSchemaNames(this OpenApiSchema schema) return new[] { schema.Xml.Name }; return Enumerable.Empty(); } + internal static IEnumerable FlattenSchemaIfRequired(this IList schemas, Func> subsequentGetter) { if (schemas is null) return Enumerable.Empty(); @@ -37,6 +40,7 @@ internal static IEnumerable FlattenSchemaIfRequired(this IList FlattenIfRequired(this IList schemas, Func> subsequentGetter) { return schemas.FlattenSchemaIfRequired(subsequentGetter).Where(static x => !string.IsNullOrEmpty(x.Title)).Select(static x => x.Title); @@ -68,6 +72,12 @@ public static bool IsObject(this OpenApiSchema? schema) { return "object".Equals(schema?.Type, StringComparison.OrdinalIgnoreCase); } + + public static bool IsScalar(this OpenApiSchema? schema) + { + return schema?.IsObject() == false && schema?.Reference?.Id == null; + } + public static bool IsInclusiveUnion(this OpenApiSchema? schema) { return schema?.AnyOf?.Count(static x => IsSemanticallyMeaningful(x, true)) > 1; @@ -103,10 +113,12 @@ public static bool IsExclusiveUnion(this OpenApiSchema? schema) 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 oDataTypes = new(StringComparer.OrdinalIgnoreCase) { "number", "integer", }; + public static bool IsODataPrimitiveType(this OpenApiSchema schema) { return schema.IsExclusiveUnion() && @@ -121,18 +133,21 @@ public static bool IsODataPrimitiveType(this OpenApiSchema schema) schema.AnyOf.Count(static x => oDataTypes.Contains(x.Type)) == 1 && schema.AnyOf.Count(static x => "string".Equals(x.Type, StringComparison.OrdinalIgnoreCase)) == 1; } + public static bool IsEnum(this OpenApiSchema schema) { if (schema is null) return false; return schema.Enum.OfType().Any(static x => !string.IsNullOrEmpty(x.Value)) && (string.IsNullOrEmpty(schema.Type) || "string".Equals(schema.Type, StringComparison.OrdinalIgnoreCase)); // number and boolean enums are not supported } + public static bool IsComposedEnum(this OpenApiSchema schema) { if (schema is null) return false; return schema.AnyOf.Count(static x => !x.IsSemanticallyMeaningful(true)) == 1 && schema.AnyOf.Count(static x => x.IsEnum()) == 1 || schema.OneOf.Count(static x => !x.IsSemanticallyMeaningful(true)) == 1 && schema.OneOf.Count(static x => x.IsEnum()) == 1; } + public static bool IsSemanticallyMeaningful(this OpenApiSchema schema, bool ignoreNullableObjects = false) { if (schema is null) return false; @@ -145,6 +160,7 @@ public static bool IsSemanticallyMeaningful(this OpenApiSchema schema, bool igno !string.IsNullOrEmpty(schema.Format) || !string.IsNullOrEmpty(schema.Reference?.Id); } + public static IEnumerable GetSchemaReferenceIds(this OpenApiSchema schema, HashSet? visitedSchemas = null) { visitedSchemas ??= new(); @@ -173,6 +189,7 @@ public static IEnumerable GetSchemaReferenceIds(this OpenApiSchema schem return Enumerable.Empty(); } + private static IEnumerable FlattenEmptyEntries(this IEnumerable schemas, Func> subsequentGetter, int? maxDepth = default) { if (schemas == null) return Enumerable.Empty(); @@ -205,6 +222,7 @@ private static IEnumerable FlattenEmptyEntries(this IEnumerable> GetDiscriminatorMappings(this OpenApiSchema schema, ConcurrentDictionary> inheritanceIndex) { if (schema == null) @@ -245,7 +264,9 @@ internal static IEnumerable> GetDiscriminatorMappin return schema.Discriminator .Mapping; } + private static readonly Func allOfEvaluatorForMappings = static x => x.Discriminator?.Mapping.Any() ?? false; + private static IEnumerable GetAllInheritanceSchemaReferences(string currentReferenceId, ConcurrentDictionary> inheritanceIndex) { ArgumentException.ThrowIfNullOrEmpty(currentReferenceId); @@ -255,4 +276,3 @@ private static IEnumerable GetAllInheritanceSchemaReferences(string curr return Enumerable.Empty(); } } - diff --git a/src/Kiota.Builder/Validation/MissingDiscriminator.cs b/src/Kiota.Builder/Validation/MissingDiscriminator.cs index ad12d74ea2..e277852c58 100644 --- a/src/Kiota.Builder/Validation/MissingDiscriminator.cs +++ b/src/Kiota.Builder/Validation/MissingDiscriminator.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using Kiota.Builder.Configuration; @@ -8,6 +9,7 @@ using Microsoft.OpenApi.Validations; namespace Kiota.Builder.Validation; + public class MissingDiscriminator : ValidationRule { public MissingDiscriminator(GenerationConfiguration configuration) : base(nameof(MissingDiscriminator), (context, document) => @@ -17,7 +19,7 @@ public MissingDiscriminator(GenerationConfiguration configuration) : base(nameof if (document.Components != null) Parallel.ForEach(document.Components.Schemas, entry => { - ValidateSchema(entry.Value, context, idx, entry.Key); + ValidateSchemaRecursively(entry.Value, context, idx, entry.Key); }); var inlineSchemasToValidate = document.Paths ?.SelectMany(static x => x.Value.Operations.Values.Select(y => (x.Key, Operation: y))) @@ -26,16 +28,47 @@ public MissingDiscriminator(GenerationConfiguration configuration) : base(nameof .ToArray() ?? Array.Empty<(string, OpenApiSchema)>(); Parallel.ForEach(inlineSchemasToValidate, entry => { - ValidateSchema(entry.Schema, context, idx, entry.Key); + ValidateSchemaRecursively(entry.Schema, context, idx, entry.Key); }); }) { } + + private static void ValidateSchemaRecursively(OpenApiSchema schema, IValidationContext context, ConcurrentDictionary> idx, string address) + { + foreach (var property in schema.Properties ?? Enumerable.Empty>()) + { + ValidateSchemaRecursively(property.Value, context, idx, $"{address}.Properties.{property.Key}"); + } + + foreach (var allOfSchema in schema.AllOf) + { + ValidateSchemaRecursively(allOfSchema, context, idx, $"{address}.AllOf"); + } + + foreach (var oneOfSchema in schema.OneOf) + { + ValidateSchemaRecursively(oneOfSchema, context, idx, $"{address}.OneOf"); + } + + foreach (var anyOfSchema in schema.AnyOf) + { + ValidateSchemaRecursively(anyOfSchema, context, idx, $"{address}.AnyOf"); + } + + if (schema.Items != null) + { + ValidateSchemaRecursively(schema.Items, context, idx, $"{address}.Items"); + } + + ValidateSchema(schema, context, idx, address); + } + private static void ValidateSchema(OpenApiSchema schema, IValidationContext context, ConcurrentDictionary> idx, string address) { if (!schema.IsInclusiveUnion() && !schema.IsExclusiveUnion()) return; - if (schema.AnyOf.All(static x => !x.IsObject()) && schema.OneOf.All(static x => !x.IsObject())) + if (schema.AnyOf.All(static x => x.IsScalar()) && schema.OneOf.All(static x => x.IsScalar())) return; if (string.IsNullOrEmpty(schema.GetDiscriminatorPropertyName()) || !schema.GetDiscriminatorMappings(idx).Any()) context.CreateWarning(nameof(MissingDiscriminator), $"The schema {address} is a polymorphic type but does not define a discriminator. This will result in a serialization errors."); diff --git a/tests/Kiota.Builder.Tests/Validation/MissingDiscriminatorTests.cs b/tests/Kiota.Builder.Tests/Validation/MissingDiscriminatorTests.cs index d112997287..3718f63858 100644 --- a/tests/Kiota.Builder.Tests/Validation/MissingDiscriminatorTests.cs +++ b/tests/Kiota.Builder.Tests/Validation/MissingDiscriminatorTests.cs @@ -6,6 +6,7 @@ using Xunit; namespace Kiota.Builder.Tests.Validation; + public class MissingDiscriminatorTests { [Fact] @@ -35,6 +36,7 @@ public void DoesntAddAWarningWhenBodyIsSimple() var doc = reader.Read(stream, out var diag); Assert.Empty(diag.Warnings); } + [Fact] public void AddsWarningOnInlineSchemas() { @@ -70,6 +72,44 @@ public void AddsWarningOnInlineSchemas() var doc = reader.Read(stream, out var diag); Assert.Single(diag.Warnings); } + + [Fact] + public void AddsWarningOnNestedInlineSchemas() + { + var rule = new MissingDiscriminator(new()); + var documentTxt = @"openapi: 3.0.1 +info: + title: OData Service for namespace microsoft.graph + description: This OData service is located at https://graph.microsoft.com/v1.0 + version: 1.0.1 +paths: + /enumeration: + get: + responses: + '200': + content: + application/json: + schema: + type: array + items: + oneOf: + - type: object + properties: + type: + type: string + - type: object + properties: + type2: + type: string"; + using var stream = new MemoryStream(Encoding.UTF8.GetBytes(documentTxt)); + var reader = new OpenApiStreamReader(new OpenApiReaderSettings + { + RuleSet = new(new ValidationRule[] { rule }), + }); + var doc = reader.Read(stream, out var diag); + Assert.Single(diag.Warnings); + } + [Fact] public void AddsWarningOnComponentSchemas() { @@ -113,6 +153,7 @@ public void AddsWarningOnComponentSchemas() var doc = reader.Read(stream, out var diag); Assert.Single(diag.Warnings); } + [Fact] public void DoesntAddsWarningOnComponentSchemasWithDiscriminatorInformation() { @@ -158,6 +199,7 @@ public void DoesntAddsWarningOnComponentSchemasWithDiscriminatorInformation() var doc = reader.Read(stream, out var diag); Assert.Empty(diag.Warnings); } + [Fact] public void DoesntAddsWarningOnComponentSchemasScalars() {