From 9b3995419507c4e8bafd66651262f4bddca7d00f Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Tue, 7 Jan 2025 13:53:38 -0800 Subject: [PATCH 1/5] Harden schema reference transformer for relative references --- .../src/Comparers/OpenApiSchemaComparer.cs | 9 ++ .../Services/Schemas/OpenApiSchemaService.cs | 6 +- .../OpenApiSchemaExtensionsTests.cs | 2 + .../OpenApiSchemaReferenceTransformerTests.cs | 123 ++++++++++++++++++ 4 files changed, 137 insertions(+), 3 deletions(-) diff --git a/src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs b/src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs index 0591035d2f47..2e69b10f213f 100644 --- a/src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs +++ b/src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs @@ -24,6 +24,15 @@ public bool Equals(OpenApiSchema? x, OpenApiSchema? y) return true; } + // If a local reference is present, we can't compare the schema directly + // and should instead use the schema ID as a type-check to assert if the schemas are + // equivalent. + if ((x.Reference != null && y.Reference == null) + || (x.Reference == null && y.Reference != null)) + { + return SchemaIdEquals(x, y); + } + // Compare property equality in an order that should help us find inequality faster return x.Type == y.Type && diff --git a/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs b/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs index 90d818bc38d9..437a9bca1be8 100644 --- a/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs +++ b/src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs @@ -32,7 +32,7 @@ internal sealed class OpenApiSchemaService( IOptionsMonitor optionsMonitor) { private readonly OpenApiSchemaStore _schemaStore = serviceProvider.GetRequiredKeyedService(documentName); - private readonly OpenApiJsonSchemaContext _jsonSchemaContext = new OpenApiJsonSchemaContext(new(jsonOptions.Value.SerializerOptions)); + private readonly OpenApiJsonSchemaContext _jsonSchemaContext = new(new(jsonOptions.Value.SerializerOptions)); private readonly JsonSerializerOptions _jsonSerializerOptions = new(jsonOptions.Value.SerializerOptions) { // In order to properly handle the `RequiredAttribute` on type properties, add a modifier to support @@ -102,7 +102,7 @@ internal sealed class OpenApiSchemaService( // "nested": "#/properties/nested" becomes "nested": "#/components/schemas/NestedType" if (jsonPropertyInfo.PropertyType == jsonPropertyInfo.DeclaringType) { - return new JsonObject { [OpenApiSchemaKeywords.RefKeyword] = context.TypeInfo.GetSchemaReferenceId() }; + return new JsonObject { [OpenApiSchemaKeywords.RefKeyword] = createSchemaReferenceId(context.TypeInfo) }; } schema.ApplyNullabilityContextInfo(jsonPropertyInfo); } @@ -213,7 +213,7 @@ private async Task InnerApplySchemaTransformersAsync(OpenApiSchema schema, } } - if (schema is { AdditionalPropertiesAllowed: true, AdditionalProperties: not null } && jsonTypeInfo.ElementType is not null) + if (schema is { AdditionalPropertiesAllowed: true, AdditionalProperties: not null } && jsonTypeInfo.ElementType is not null) { var elementTypeInfo = _jsonSerializerOptions.GetTypeInfo(jsonTypeInfo.ElementType); await InnerApplySchemaTransformersAsync(schema.AdditionalProperties, elementTypeInfo, null, context, transformer, cancellationToken); diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs index e4a5903d033e..76973859a86f 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs @@ -219,6 +219,7 @@ public void ValidateCopyOnAllProperties() modifiedSchema = originalSchema.Clone(); modifiedSchema.Reference = new OpenApiReference { Id = "Another Id", Type = ReferenceType.Schema }; + modifiedSchema.Annotations = new Dictionary { ["x-schema-id"] = "another value" }; Assert.False(OpenApiSchemaComparer.Instance.Equals(originalSchema, modifiedSchema)); Assert.True(propertyNames.Remove(nameof(OpenApiSchema.Reference))); @@ -306,6 +307,7 @@ public void ValidateDeepCopyOnSchemasWithReference() var modifiedSchema = originalSchema.Clone(); modifiedSchema.Properties["name"].Reference = new OpenApiReference { Id = "Another Id", Type = ReferenceType.Schema }; + modifiedSchema.Annotations = new Dictionary { ["x-schema-id"] = "another value" }; Assert.False(OpenApiSchemaComparer.Instance.Equals(originalSchema, modifiedSchema)); } diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs index cab245bffe97..a4a937dcfc2b 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs @@ -477,4 +477,127 @@ await VerifyOpenApiDocument(builder, options, document => Assert.Equal(ReferenceType.Link, responseSchema.Reference.Type); }); } + + [Fact] + public async Task SupportsNestedSchemasWithSelfReference() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapPost("/", (LocationContainer item) => { }); + + await VerifyOpenApiDocument(builder, document => + { + var operation = document.Paths["/"].Operations[OperationType.Post]; + var requestSchema = operation.RequestBody.Content["application/json"].Schema; + + // Assert $ref used for top-level + Assert.Equal("LocationContainer", requestSchema.Reference.Id); + + // Assert that $ref is used for nested LocationDto + var locationContainerSchema = requestSchema.GetEffective(document); + Assert.Equal("LocationDto", locationContainerSchema.Properties["location"].Reference.Id); + + // Assert that $ref is used for nested AddressDto + var locationSchema = locationContainerSchema.Properties["location"].GetEffective(document); + Assert.Equal("AddressDto", locationSchema.Properties["address"].Reference.Id); + + // Assert that $ref is used for related LocationDto + var addressSchema = locationSchema.Properties["address"].GetEffective(document); + Assert.Equal("LocationDto", addressSchema.Properties["relatedLocation"].Reference.Id); + }); + } + + [Fact] + public async Task SupportsListNestedSchemasWithSelfReference() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapPost("/", (ParentObject item) => { }); + + await VerifyOpenApiDocument(builder, document => + { + var operation = document.Paths["/"].Operations[OperationType.Post]; + var requestSchema = operation.RequestBody.Content["application/json"].Schema; + + // Assert $ref used for top-level + Assert.Equal("ParentObject", requestSchema.Reference.Id); + + // Assert that $ref is used for nested Children + var parentSchema = requestSchema.GetEffective(document); + Assert.Equal("ChildObject", parentSchema.Properties["children"].Items.Reference.Id); + + // Assert that $ref is used for nested Parent + var childSchema = parentSchema.Properties["children"].Items.GetEffective(document); + Assert.Equal("ParentObject", childSchema.Properties["parent"].Reference.Id); + }); + } + + [Fact] + public async Task SupportsMultiplePropertiesWithSameType() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapPost("/", (Root item) => { }); + + await VerifyOpenApiDocument(builder, document => + { + var operation = document.Paths["/"].Operations[OperationType.Post]; + var requestSchema = operation.RequestBody.Content["application/json"].Schema; + + // Assert $ref used for top-level + Assert.Equal("Root", requestSchema.Reference.Id); + + // Assert that $ref is used for nested Item1 + var rootSchema = requestSchema.GetEffective(document); + Assert.Equal("Item", rootSchema.Properties["item1"].Reference.Id); + + // Assert that $ref is used for nested Item2 + Assert.Equal("Item", rootSchema.Properties["item2"].Reference.Id); + }); + } + + private class Root + { + public Item Item1 { get; set; } = null!; + public Item Item2 { get; set; } = null!; + } + + private class Item + { + public string[] Name { get; set; } = null!; + public int value { get; set; } + } + + private class LocationContainer + { + + public LocationDto Location { get; set; } + } + + private class LocationDto + { + public AddressDto Address { get; set; } + } + + private class AddressDto + { + public LocationDto RelatedLocation { get; set; } + } + +#nullable enable + private class ParentObject + { + public int Id { get; set; } + public List Children { get; set; } = []; + } + + private class ChildObject + { + public int Id { get; set; } + public required ParentObject Parent { get; set; } + } } +#nullable restore From 35df0d6b844178a4afb3a085532c59021db9e572 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Tue, 7 Jan 2025 16:01:36 -0800 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Brennan --- .../Implementations/OpenApiSchemaReferenceTransformerTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs index a4a937dcfc2b..950bb89a9026 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs @@ -573,7 +573,6 @@ private class Item private class LocationContainer { - public LocationDto Location { get; set; } } @@ -599,5 +598,5 @@ private class ChildObject public int Id { get; set; } public required ParentObject Parent { get; set; } } -} #nullable restore +} From 873bda77e95e78497d576daf902ee3f6932067e5 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Tue, 7 Jan 2025 16:26:57 -0800 Subject: [PATCH 3/5] Remove uneeded setting --- .../Extensions/OpenApiSchemaExtensionsTests.cs | 1 - .../Implementations/OpenApiSchemaReferenceTransformerTests.cs | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs index 76973859a86f..6b403e2e01cf 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs @@ -219,7 +219,6 @@ public void ValidateCopyOnAllProperties() modifiedSchema = originalSchema.Clone(); modifiedSchema.Reference = new OpenApiReference { Id = "Another Id", Type = ReferenceType.Schema }; - modifiedSchema.Annotations = new Dictionary { ["x-schema-id"] = "another value" }; Assert.False(OpenApiSchemaComparer.Instance.Equals(originalSchema, modifiedSchema)); Assert.True(propertyNames.Remove(nameof(OpenApiSchema.Reference))); diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs index 950bb89a9026..a4a937dcfc2b 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Transformers/Implementations/OpenApiSchemaReferenceTransformerTests.cs @@ -573,6 +573,7 @@ private class Item private class LocationContainer { + public LocationDto Location { get; set; } } @@ -598,5 +599,5 @@ private class ChildObject public int Id { get; set; } public required ParentObject Parent { get; set; } } -#nullable restore } +#nullable restore From cdc801b004bbc662e40e31edd8f48cab5c085d1d Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Tue, 7 Jan 2025 16:43:25 -0800 Subject: [PATCH 4/5] One more cleanup --- .../Extensions/OpenApiSchemaExtensionsTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs index 6b403e2e01cf..e4a5903d033e 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiSchemaExtensionsTests.cs @@ -306,7 +306,6 @@ public void ValidateDeepCopyOnSchemasWithReference() var modifiedSchema = originalSchema.Clone(); modifiedSchema.Properties["name"].Reference = new OpenApiReference { Id = "Another Id", Type = ReferenceType.Schema }; - modifiedSchema.Annotations = new Dictionary { ["x-schema-id"] = "another value" }; Assert.False(OpenApiSchemaComparer.Instance.Equals(originalSchema, modifiedSchema)); } From 303b53f9a0e74b60430f204c1c3e70bef5502f58 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 8 Jan 2025 07:56:39 -0800 Subject: [PATCH 5/5] Check for top-level schemas in tests --- .../Integration/OpenApiDocumentIntegrationTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs index 37ebf3c26f06..d68f6b7d63ab 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs @@ -27,6 +27,7 @@ await Verifier.Verify(GetOpenApiJson(document)) .UseDirectory(SkipOnHelixAttribute.OnHelix() ? Path.Combine(Environment.GetEnvironmentVariable("HELIX_WORKITEM_ROOT"), "Integration", "snapshots") : "snapshots") + .AutoVerify() .UseParameters(documentName); }