Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Discriminator - Ensure warning shows up in recursive case #4350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Discriminator - Ensure warning shows up in recursive case #4350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
using Microsoft.OpenApi.Models;

namespace Kiota.Builder.Extensions;

public static class OpenApiSchemaExtensions
{
private static readonly Func<OpenApiSchema, IList<OpenApiSchema>> classNamesFlattener = x =>
(x.AnyOf ?? Enumerable.Empty<OpenApiSchema>()).Union(x.AllOf).Union(x.OneOf).ToList();

public static IEnumerable<string> GetSchemaNames(this OpenApiSchema schema)
{
if (schema == null)
Expand All @@ -30,13 +32,15 @@ public static IEnumerable<string> GetSchemaNames(this OpenApiSchema schema)
return new[] { schema.Xml.Name };
return Enumerable.Empty<string>();
}

internal static IEnumerable<OpenApiSchema> FlattenSchemaIfRequired(this IList<OpenApiSchema> schemas, Func<OpenApiSchema, IList<OpenApiSchema>> subsequentGetter)
{
if (schemas is null) return Enumerable.Empty<OpenApiSchema>();
return schemas.Count == 1 ?
schemas.FlattenEmptyEntries(subsequentGetter, 1) :
schemas;
}

private static IEnumerable<string> FlattenIfRequired(this IList<OpenApiSchema> schemas, Func<OpenApiSchema, IList<OpenApiSchema>> subsequentGetter)
{
return schemas.FlattenSchemaIfRequired(subsequentGetter).Where(static x => !string.IsNullOrEmpty(x.Title)).Select(static x => x.Title);
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method name is confusing. An array would technically return true even though semantically it's not scalar.

Also the implementation checks two separate concerns: the structure (object) and the location (reference) of the 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;
Expand Down Expand Up @@ -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<string> oDataTypes = new(StringComparer.OrdinalIgnoreCase) {
"number",
"integer",
};

public static bool IsODataPrimitiveType(this OpenApiSchema schema)
{
return schema.IsExclusiveUnion() &&
Expand All @@ -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<OpenApiString>().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;
Expand All @@ -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<string> GetSchemaReferenceIds(this OpenApiSchema schema, HashSet<OpenApiSchema>? visitedSchemas = null)
{
visitedSchemas ??= new();
Expand Down Expand Up @@ -173,6 +189,7 @@ public static IEnumerable<string> GetSchemaReferenceIds(this OpenApiSchema schem

return Enumerable.Empty<string>();
}

private static IEnumerable<OpenApiSchema> FlattenEmptyEntries(this IEnumerable<OpenApiSchema> schemas, Func<OpenApiSchema, IList<OpenApiSchema>> subsequentGetter, int? maxDepth = default)
{
if (schemas == null) return Enumerable.Empty<OpenApiSchema>();
Expand Down Expand Up @@ -205,6 +222,7 @@ private static IEnumerable<OpenApiSchema> FlattenEmptyEntries(this IEnumerable<O
}
return result;
}

internal static string GetDiscriminatorPropertyName(this OpenApiSchema schema)
{
if (schema == null)
Expand All @@ -222,6 +240,7 @@ internal static string GetDiscriminatorPropertyName(this OpenApiSchema schema)

return string.Empty;
}

internal static IEnumerable<KeyValuePair<string, string>> GetDiscriminatorMappings(this OpenApiSchema schema, ConcurrentDictionary<string, ConcurrentDictionary<string, bool>> inheritanceIndex)
{
if (schema == null)
Expand All @@ -245,7 +264,9 @@ internal static IEnumerable<KeyValuePair<string, string>> GetDiscriminatorMappin
return schema.Discriminator
.Mapping;
}

private static readonly Func<OpenApiSchema, bool> allOfEvaluatorForMappings = static x => x.Discriminator?.Mapping.Any() ?? false;

private static IEnumerable<string> GetAllInheritanceSchemaReferences(string currentReferenceId, ConcurrentDictionary<string, ConcurrentDictionary<string, bool>> inheritanceIndex)
{
ArgumentException.ThrowIfNullOrEmpty(currentReferenceId);
Expand All @@ -255,4 +276,3 @@ private static IEnumerable<string> GetAllInheritanceSchemaReferences(string curr
return Enumerable.Empty<string>();
}
}

39 changes: 36 additions & 3 deletions src/Kiota.Builder/Validation/MissingDiscriminator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -8,6 +9,7 @@
using Microsoft.OpenApi.Validations;

namespace Kiota.Builder.Validation;

public class MissingDiscriminator : ValidationRule<OpenApiDocument>
{
public MissingDiscriminator(GenerationConfiguration configuration) : base(nameof(MissingDiscriminator), (context, document) =>
Expand All @@ -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)))
Expand All @@ -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<string, ConcurrentDictionary<string, bool>> idx, string address)
{
foreach (var property in schema.Properties ?? Enumerable.Empty<KeyValuePair<string, OpenApiSchema>>())
{
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<string, ConcurrentDictionary<string, bool>> 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.");
Expand Down
42 changes: 42 additions & 0 deletions tests/Kiota.Builder.Tests/Validation/MissingDiscriminatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Xunit;

namespace Kiota.Builder.Tests.Validation;

public class MissingDiscriminatorTests
{
[Fact]
Expand Down Expand Up @@ -35,6 +36,7 @@ public void DoesntAddAWarningWhenBodyIsSimple()
var doc = reader.Read(stream, out var diag);
Assert.Empty(diag.Warnings);
}

[Fact]
public void AddsWarningOnInlineSchemas()
{
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -113,6 +153,7 @@ public void AddsWarningOnComponentSchemas()
var doc = reader.Read(stream, out var diag);
Assert.Single(diag.Warnings);
}

[Fact]
public void DoesntAddsWarningOnComponentSchemasWithDiscriminatorInformation()
{
Expand Down Expand Up @@ -158,6 +199,7 @@ public void DoesntAddsWarningOnComponentSchemasWithDiscriminatorInformation()
var doc = reader.Read(stream, out var diag);
Assert.Empty(diag.Warnings);
}

[Fact]
public void DoesntAddsWarningOnComponentSchemasScalars()
{
Expand Down
Loading