Add support for reference-based schemas in OpenAPI document #56175

captainsafia · 2024-06-10T19:59:31Z

Note: this PR targets a feature branch.

This pull request adds support for generating reference IDs for JSON schemas that are duplicated across the OpenAPI document. This implementation consists of three components:

A collection of IEqualityComparer implementations that allow us to compare whether two OpenApiSchema instances are equivalent
An IOpenApiDocumentTransformer implementation that walks the schemas in the OpenAPI document model and replaces inline schemas with reference ID-based schemas where applicable
Additional logic to the schema store to support storing OpenAPI schemas alongside their reference IDs
- Note: this PR generates a random GUID for each schema reference ID. Human-readable schema reference IDs will come in a follow-up PR.

In this implementation, references are generated based on the equality of the generated schemas, not the .NET type they are associated with. With this strategy, we avoid leaking details of .NET type system to the OpenAPI document. For example, types that map to the same JSON schema (like IEnumerable<Todo> and Todo[]) will use the same reference ID. This strategy also allows us to account for transformations made to the OpenAPI schema via transformers in the final document.

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

JamesNK · 2024-06-10T23:54:30Z

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

+            return false;
+        }
+
+        return GetHashCode(x) == GetHashCode(y);


The two objects might return true but not be the same if equals is implemented like this. Won't that be a problem?

Say more? It should function as an equality operator as long as it's symmetric, reflexive, and transitive, no?

I think the typical expectation is that types with the same hash codes might be equal whereas types with different hash codes are definitely not equal.

Assuming I've done my implementation correctly, GetHashCode for OpenApiSchema and should return a unique value for each equivalent schema object (e.g. the bucket size is always 1).

When working with hashcodes I don't think you can guarantee the bucket size is 1 given the nature of the objects. The data in the object is being reduced down to a 2^32 number. There are infinite possible ways of defining schemas that are reduced to a hashcode so it seems like at some point there must be hash collisions.

I question what is happening here - using hash code as equals goes against guidance - but on the other hand you're confident that's its ok. I don't feel certain enough in my understanding of hash tables to be sure. Maybe ping an expert in this area?

Your questions here are legit. After some rubber ducking, it appears there is a gap here when evaluating OpenApiByte and OpenApiBinary types since they encapsulate a byte area that we were evaluating correctly. I did some hardening on the comparer implementations in 623fa19

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs

JamesNK · 2024-06-11T00:03:39Z

In this implementation, references are generated based on the equality of the generated schemas, not the .NET type they are associated with. With this strategy, we avoid leaking details of .NET type system to the OpenAPI document. For example, types that map to the same JSON schema (like IEnumerable<Todo> and Todo[]) will use the same reference ID. This strategy also allows us to account for transformations made to the OpenAPI schema via transformers in the final document.

I agree about not duplicating schemas for different collection types. Did you consider having some logic that knows about collections and ensures that JSON array types are shared, e.g. anything that implements IEnumerable<T> is automatically shared, e.g.

IEnumerable<Todo>
Todo[]
List<Todo>
ImmutableArray<Todo>
etc

That seems like a simple and clean way to share collection type schemas.

captainsafia · 2024-06-11T20:18:11Z

Did you consider having some logic that knows about collections and ensures that JSON array types are shared, e.g. anything that implements IEnumerable is automatically shared, e.g.

I'm not sure what you mean by "JSON array types" are shared. Some of the logic around generating the actual schema for the array types is hidden behind STJ's JsonSchemaMapper support. For array-based schemas, it keys off whether it's JsonTypeInfoKind.Enumerable. Is using something like that what you're describing here?

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

amcasey · 2024-06-11T19:56:30Z

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

+            return false;
+        }
+
+        return GetHashCode(x) == GetHashCode(y);


Say more? It should function as an equality operator as long as it's symmetric, reflexive, and transitive, no?

src/OpenApi/src/Comparers/OpenApiDiscriminatorComparer.cs

amcasey · 2024-06-11T19:59:56Z

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

+
+internal sealed class OpenApiAnyComparer : IEqualityComparer<IOpenApiAny>
+{
+    public static OpenApiAnyComparer Instance { get; } = new OpenApiAnyComparer();


I usually give such types a private constructor, but I'm guessing you considered that to be too much ceremony.

Do you mean providing a private constructor over a static Instance property or both? What's he value of the private constructor in the later case?

I usually provide both so no one fails to notice the Instance member and instantiates it themselves.

src/OpenApi/src/Comparers/OpenApiTagComparer.cs

src/OpenApi/src/Transformers/Implementations/OpenApiSchemaReferenceTransformer.cs

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs

captainsafia · 2024-06-13T15:25:09Z

Apologies for the review request spam! I've rebased against main and changed the branch target for this PR from a feature branch to main. Support for friendly $ref names is in 9ae6437.

martincostello · 2024-06-13T15:50:00Z

src/OpenApi/src/Extensions/JsonTypeInfoExtensions.cs

+
+internal static class JsonTypeInfoExtensions
+{
+    private static readonly Dictionary<Type, string> _simpleTypeToName = new()


Are there any other primitive-y types that should be added? For example I added DateOnly, TimeOnly, (U)Int128 and some others to the latest release of Swashbuckle (admittedly for a different purpose): JSON serializer context

DateOnly, TimeOnly, and a few others on the referenced link get handled by the JsonSchemaMapper for us. I'll do a few more comparisons but I think the set we have here is good enough.

martincostello · 2024-06-13T15:51:01Z

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs

@@ -17,16 +19,39 @@ internal sealed class OpenApiSchemaStore
    private readonly ConcurrentDictionary<OpenApiSchemaKey, JsonObject> _schemas = new()


Some similar code here in case that's useful too.

src/OpenApi/test/Extensions/JsonTypeInfoExtensionsTests.cs

martincostello · 2024-06-13T15:55:42Z

...pshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=forms.verified.txt

+      "DateTime": {
+        "type": "string",
+        "format": "date-time"
+      },
+      "IFormFile": {
+        "type": "string",
+        "format": "binary"
+      },
+      "IFormFileCollection": {
+        "type": "array",
+        "items": {
+          "$ref": "#/components/schemas/IFormFile"
+        }
+      },


Does this potentially bleed out some ".NET-ness" where more generic names could be used (form-file or whatever)?

Potentially, if applicable, I think I might prefer FormFile over form-file since it is a bit more homogenous with remaining conventions and more closely aligns with valid identifier names in most programming languages. There's probably some implementation bleed around disclosing that this is an interface.

JamesNK · 2024-06-14T01:50:17Z

I'm not sure what you mean by "JSON array types" are shared. Some of the logic around generating the actual schema for the array types is hidden behind STJ's JsonSchemaMapper support. For array-based schemas, it keys off whether it's JsonTypeInfoKind.Enumerable. Is using something like that what you're describing here?

Yes.

IMO when generating schemas for types then I think Type is a good key. In some cases we want to massage the type, such as make int[], IEnumerable<int>, ImmutableArray<int>, etc, all resolve to the same schema since they're all just arrays of the same schema. And primitive types can be special cased, e.g. Int32 -> int, etc.

Why is it better to use the schema as a key than the type?

captainsafia · 2024-06-14T21:59:58Z

IMO when generating schemas for types then I think Type is a good key. In some cases we want to massage the type, such as make int[], IEnumerable, ImmutableArray, etc, all resolve to the same schema since they're all just arrays of the same schema. And primitive types can be special cased, e.g. Int32 -> int, etc.

The behavior that you described here matches the current behavior of this implementation. I added this snapshot test to more clearly illustrate the behavior of the reference schema.

When it comes to generating the schemas themselves, we're still using the .NET type. It's only when we generate reference IDs that we bring in the uniqueness of schemas. This makes the implementation more tolerant to:

Types that share the same schema but are represented with a different .NET type like the collection and key-value pair collections mentioned above.
Schemas that have been modified as the result of a users schema transformers. For example, if the schema for a type is modified when it is used in a response object vs. a request object.

captainsafia · 2024-06-17T20:17:01Z

👈🏽 for reviews -- I think I've addressed a bulk of the feedback around the comparers in 623fa19. Would like to get this in for preview6 if possible. 🙏🏽

src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs

halter73 · 2024-06-18T17:28:46Z

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs

+                {
+                    counter++;
+                    _referenceIdCounter[targetReferenceId] = counter;
+                    _schemasWithReference[schema] = $"{targetReferenceId}{counter}";


Is it possible for referenceId "Foo" and "Foo2" to already be incoming referenceId's? I'm thinking something along the lines of X509Certificate and X509Certificate2 but DTOs. It looks like this could conflict if the type without the 2 got an incremented counter due to special annotations.

Yep, this is a possible (although unlikely) edge case. In the event that this happens, the implementation will throw an exception to the user about the unexpected conflict.

There's a couple of ways I could see around solving this:

Throw a more descriptive exception when we try to insert duplicate IDs so the user has a little bit more insight into what is going on here.

Provide an options hook that allows users to configure custom schema name rules that override are defaults.

I'm leaning towards the later given it's a generally useful feature that gives users more flexibility around the naming conventions. That way, you can map things appropriately if there are overloaded type names in your application.

I filed #56305 to discuss the second approach further.

halter73 · 2024-06-18T17:39:30Z

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs

+        throw new InvalidOperationException("The schema reference ID must be set on the schema.");
+    }
+
+    public Dictionary<OpenApiSchema, string?> SchemasByReference => _schemasWithReference;


Should this be an ImmutableDictionary? Also, should we just make it an internal property instead of a field+property?

Simplified this to a property. I'm not the biggest fan of ImmutableDictionary given the requirements on using the builder pattern for it, especially because we're building up the list of schemas as the schema service gets called.

src/OpenApi/src/Transformers/Implementations/OpenApiSchemaReferenceTransformer.cs

JamesNK · 2024-06-18T23:39:57Z

I'll try to make time this week to rebase my gRPC transcoding + OpenAPI PR on top of this and do some more testing. I'm curious to see the results in (semi) real world usage.

captainsafia · 2024-06-18T23:44:23Z

I'll try to make time this week to rebase my gRPC transcoding + OpenAPI PR on top of this and do some more testing. I'm curious to see the results in (semi) real world usage.

Sounds good! I'm curious to see how this flows with the gRPC integration. Happy to refine/add new APIs if anything shakes out as a result of that.

captainsafia requested a review from a team as a code owner June 10, 2024 19:59

dotnet-issue-labeler bot added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jun 10, 2024

captainsafia requested review from BrennanConroy and amcasey June 10, 2024 20:00

martincostello reviewed Jun 10, 2024

View reviewed changes

src/OpenApi/src/Comparers/OpenApiAnyComparer.cs Show resolved Hide resolved

JamesNK reviewed Jun 10, 2024

View reviewed changes

src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs Outdated Show resolved Hide resolved

amcasey reviewed Jun 11, 2024

View reviewed changes

captainsafia force-pushed the safia/openapi-ref branch from 0a8f9c5 to 9ae6437 Compare June 13, 2024 15:23

captainsafia requested review from wtgodbe, a team, halter73 and mgravell as code owners June 13, 2024 15:23

captainsafia changed the base branch from feature/openapi-refs to main June 13, 2024 15:24

captainsafia removed request for a team, mgravell and wtgodbe June 13, 2024 15:25

martincostello reviewed Jun 13, 2024

View reviewed changes

captainsafia added 2 commits June 14, 2024 14:38

Add support for reference-based schemas in OpenAPI document

311866d

Use ReferenceEquals in equality checks and address feedback

b4d6a6b

captainsafia added 2 commits June 14, 2024 14:38

Support generating friendly reference names

9086b3e

Add schema spec tests, move controller tests, move dedupe logic

5b44e5e

captainsafia force-pushed the safia/openapi-ref branch from f68e774 to 5b44e5e Compare June 14, 2024 21:41

Add more scenarios to snapsho tests

7372bb8

build-analysis bot mentioned this pull request Jun 15, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

captainsafia added 2 commits June 14, 2024 18:58

Harden OpenApi comparer implementations

623fa19

Add tests for schema transformers + refs

67508ad

martincostello mentioned this pull request Jun 17, 2024

Microsoft.AspNetCore.OpenApi should generate references to shared schemas rather then duplicating them #56264

Closed

1 task

halter73 reviewed Jun 17, 2024

View reviewed changes

src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs Show resolved Hide resolved

halter73 approved these changes Jun 18, 2024

View reviewed changes

Address more feedback

cde17fb

captainsafia enabled auto-merge (squash) June 18, 2024 23:06

captainsafia merged commit 874aaf4 into main Jun 18, 2024
26 checks passed

captainsafia deleted the safia/openapi-ref branch June 18, 2024 23:15

dotnet-policy-service bot added this to the 9.0-preview6 milestone Jun 18, 2024

martincostello mentioned this pull request Jun 19, 2024

OpenAPI schema references feedback #56318

Closed

This was referenced Jul 3, 2024

.NET 9 OpenAPI doesn't seem to export schemas (e.g. types) #56586

Closed

Microsoft.AspNetCore.OpenApi: Support API versioning (required for Swashbuckle feature parity) #56314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reference-based schemas in OpenAPI document #56175

Add support for reference-based schemas in OpenAPI document #56175

captainsafia commented Jun 10, 2024

JamesNK Jun 10, 2024 •

edited

Loading

amcasey Jun 11, 2024

captainsafia Jun 11, 2024

JamesNK Jun 14, 2024 •

edited

Loading

captainsafia Jun 16, 2024

JamesNK commented Jun 11, 2024

captainsafia commented Jun 11, 2024

amcasey Jun 11, 2024

amcasey Jun 11, 2024

captainsafia Jun 12, 2024

amcasey Jun 12, 2024

captainsafia commented Jun 13, 2024

martincostello Jun 13, 2024

captainsafia Jun 14, 2024

martincostello Jun 13, 2024

martincostello Jun 13, 2024

captainsafia Jun 17, 2024

JamesNK commented Jun 14, 2024

captainsafia commented Jun 14, 2024

captainsafia commented Jun 17, 2024

halter73 Jun 18, 2024

captainsafia Jun 18, 2024

captainsafia Jun 18, 2024

halter73 Jun 18, 2024

captainsafia Jun 18, 2024

JamesNK commented Jun 18, 2024

captainsafia commented Jun 18, 2024

		@@ -17,16 +19,39 @@ internal sealed class OpenApiSchemaStore
		private readonly ConcurrentDictionary<OpenApiSchemaKey, JsonObject> _schemas = new()

Add support for reference-based schemas in OpenAPI document #56175

Add support for reference-based schemas in OpenAPI document #56175

Conversation

captainsafia commented Jun 10, 2024

JamesNK Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Jun 11, 2024

captainsafia commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainsafia commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Jun 14, 2024

captainsafia commented Jun 14, 2024

captainsafia commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Jun 18, 2024

captainsafia commented Jun 18, 2024

JamesNK Jun 10, 2024 •

edited

Loading

JamesNK Jun 14, 2024 •

edited

Loading