Suggested path for JsonSchema.Net integration #1191

gregsdennis · 2023-03-24T22:34:18Z

This branch is my recommended approach for using JsonSchema.Net. It will require a few paradigm shifts for the internals of OpenApi.Net, but these changes are necessary to take full advantage of what JsonSchema.Net has to offer.

Opening this as a draft as it's not nearly ready and so that we can discuss the changes.

I think with the approach I have here, we can completely remove the Readers library.

…ent should implement ibasedocument; added yaml<->json conversion

… and stj serialization

…ft 4 meta-schema; started converting tests

gregsdennis · 2023-03-26T19:54:15Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiSchemaValidationTests.cs

+                                    .Ref("#/components/schemas/Person")
+                            )
+                            .Ref("Person")


I'm not sure what the difference between these refs is supposed to be.

The original code is

Reference = new OpenApiReference { Type = ReferenceType.Schema, Id = "Person" } } }, Reference = new OpenApiReference { Id = "Person" }

What does this reference do? My .Ref() extension creates a $ref keyword, but I don't think that's really what's supposed to be happening here.

Components have a self-reference. It's an ugly implementation detail due to the way that we load $refs in as a dummy empty object (because the target of the reference hasn't been parsed yet) and then post-reading we run a reference resolver to replace the dummy objects with the target object. This means the inline object and the component object are actually the same object instance. Therefore, objects in the components collection also need to have a Reference property set or the writers cannot write out the $ref.

This model is going to change OpenAPI.Net v2 to a proxy pattern. We need to do this because in OpenAPI v3.1 you can now have summary and description properties that sit along side the $ref. With the current model we have no place to store that additional information.

Anyway, I think the proxy pattern for handling references is going to be a much better model anyway as it will not require doing the post-reading reference resolution step. It also makes writing much easier. You can see my attempt to implement the proxy pattern here https://github.com/microsoft/OpenAPI.NET/blob/dm/ProxyReference/src/Microsoft.OpenApi/Models/OpenApiSchemaReference.cs However, I did it on OpenAPISchema which is no longer appropriate since 3.1 made the distinction between OpenAPI reference objects and JSON Schema $ref.

Okay, so I can just remove all of those, probably. I'll need to go through and re-update all of those tests. (I'll probably end up closing this and cherry-picking it to another branch for a different PR.)

gregsdennis · 2023-03-26T19:56:47Z

test/Microsoft.OpenApi.Tests/Models/OpenApiComponentsTests.cs

+                        null, // These nulls are invalid for allOf; I'm not sure what they're trying to achieve.
                        null,
-                        new OpenApiSchema
-                        {
-                            Type = "string"
-                        },
+                        new JsonSchemaBuilder().Type(SchemaValueType.String),
                        null,
                        null
-                    }
-                }
+                    )


This is defining the BrokenComponents object for this test, but submitting null here is more than broken; it's completely invalid. I'm not sure JsonSchema.Net will even support doing this, and even if it does, you'll likely get a NullReferenceException.

…pdate tests

darrelmiller · 2023-04-02T14:48:25Z

src/Microsoft.OpenApi.Readers/OpenApiTextReaderReader.cs

@@ -50,7 +51,12 @@ public OpenApiDocument Read(TextReader input, out OpenApiDiagnostic diagnostic)
                return new OpenApiDocument();
            }

-            return new OpenApiYamlDocumentReader(this._settings).Read(yamlDocument, out diagnostic);
+            var asJsonNode = yamlDocument.ToJsonNode();


I believe we are going to make the switch to do Yaml => JsonNodes => OpenAPIDocument. This will facilitate us to use the JsonSchema.Net Examples and hopefully get rid of OpenApiAny and all of the OpenApiPrimitive classes.

darrelmiller · 2023-04-02T14:51:52Z

src/Microsoft.OpenApi.Readers/OpenApiTextReaderReader.cs

+            var asJsonNode = yamlDocument.ToJsonNode();
+
+            diagnostic = null; // TBD
+            return asJsonNode.Deserialize<OpenApiDocument>();


I honestly don't see how using Deserialize is going to work for us. Using an automatic deserialization mechanism strongly couples the serialized format to the in-memory model. We specifically wrote the deserializer classes so we can deserialize any version of OpenAPI into the "latest" in-memory model.

It is not clear to me from this PR how you are proposing we continue allowing our users to import OpenAPI without caring what version of OAS the input format is. And to clarify, this is one of our most important features.

Strictly speaking, this change isn't necessary because this code will be deleted.

Using an automatic deserialization mechanism strongly couples the serialized format to the in-memory model.

How so? I'm doing the actual parsing with YamlSharp; both YAML and JSON are supported through this. It parses into the YamlDocument model, which I then translate to JsonNode. Then I just use the built-in JsonSerializer to deserialize it into OpenApiDocument.

Going the other way, if you want JSON, you just use JsonSerializer.Serialize(openApiDoc). If you want YAML, you use this method which we'll need to expose as an extension.

We specifically wrote the deserializer classes so we can deserialize any version of OpenAPI into the "latest" in-memory model.

This supports the same thing. I can drop a 2.0 or a 3.1 schema into my DevTest test and it still deserializes just fine. Users don't need to care which OpenAPI version they have.

How does the JsonDeserializer.Deserialize convert body parameters into a requestBody object? How does the JsonSerializer.Deserialize know how to convert produces/consumes into media type objects? We have many of these kinds of translations that System.Text.Json knows nothing about.

I don't follow. The paths use OpenApiParameter which isn't associated with OpenApiRequestBody (only used in components) that I can see.

In our V2 Deserializer, we convert parameters that are of type body or form into a request body. See code here:

https://github.com/microsoft/OpenAPI.NET/blob/vnext/src/Microsoft.OpenApi.Readers/V2/OpenApiOperationDeserializer.cs#L126-L138

darrelmiller · 2023-04-02T14:54:55Z

src/Microsoft.OpenApi.Readers/YamlConverter.cs

+namespace Microsoft.OpenApi.Readers
+{
+    /// <summary>
+    /// Provides extensions to convert YAML models to JSON models.


This is awesome. Thanks! Maggie is currently doing a spike to try and convert the ParseNode classes to work off JsonNode instead of YamlNode. If she is able to do that, then this class will enable us to continue supporting YAML parsing.

This code is basically copied from my YamlToJsonNode package. That lib uses YamlDotNet which is the basis for YamlSharp, so it's the same code for "different" types.

darrelmiller · 2023-04-02T14:59:19Z

src/Microsoft.OpenApi/Models/OpenApiHeader.cs

@@ -82,17 +77,17 @@ public class OpenApiHeader : IOpenApiSerializable, IOpenApiReferenceable, IOpenA
        /// <summary>
        /// Examples of the media type.
        /// </summary>
-        public IDictionary<string, OpenApiExample> Examples { get; set; } = new Dictionary<string, OpenApiExample>();
+        public IDictionary<string, OpenApiExample> Examples { get; set; }


It was an intentional design choice to ensure collections were non-null so that library users did not have to check if collections had been created before adding properties to the collection. However, due to the performance cost we should do a lazy instantiation of these properties in the get accessor instead of doing it at create time.

These initializations cause these properties to serialize. Lazy initialization will do the same. Not using custom JSON converters is preferred, where possible.

For ease-of-use, personally, I think a fluent builder is better. I made JsonSchema immutable, so I needed a separate JsonSchemaBuilder class. If you keep your types mutable, you won't need a separate builder class.

darrelmiller · 2023-04-02T15:19:45Z

@gregsdennis Meggie mentioned to me that you had some concern of how the OpenApiSchemaDeserizalizer would be able to parse schema keywords that were not previously known. We using this deserialization pattern extensively in our SDK generation and are able to parse unrecognized properties into a dictionary without any problem. Currently the ParseField does report an error if it finds an unknown property

OpenAPI.NET/src/Microsoft.OpenApi.Readers/ParseNodes/PropertyNode.cs

Line 83 in df1b405

Context.Diagnostic.Errors.Add(

However, it will be trivial to augment this for parsing of JsonNodes that contain properties of JsonSchema so that we can either handle keywords that have been registered in your JsonSchema keyword registry or if they are completely unknown. This will allow us to handle both the current 3.1 case where unknown keywords are allowed and the planned future case where unknown keywords are not.

baywet · 2023-04-04T13:25:22Z

src/Microsoft.OpenApi.Readers/YamlConverter.cs

+        /// <returns>A collection of nodes representing the YAML documents in the stream.</returns>
+        public static IEnumerable<JsonNode> ToJsonNode(this YamlStream yaml)
+        {
+            return yaml.Documents.Select(x => x.ToJsonNode());


Suggested change

return yaml.Documents.Select(x => x.ToJsonNode());

return yaml.Documents.Select(static x => x.ToJsonNode());

I don't think they're using a compiler version that supports this.

Evidence would suggest otherwise

OpenAPI.NET/src/Microsoft.OpenApi/Models/OpenApiOperation.cs

Line 286 in df1b405

.Where(static r => r.Value.Content != null)

Sure, okay. I'm not worried about these nuances right now. Let's please focus on making it work.

Docs: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/static-anonymous-functions

Removing the static modifier from an anonymous function in a valid program does not change the meaning of the program.

This is an inconsequential, editorial change.

baywet · 2023-04-04T13:28:18Z

src/Microsoft.OpenApi.Readers/YamlConverter.cs

+            var node = new JsonArray();
+            foreach (var value in yaml)
+            {
+                node.Add(value.ToJsonNode());
+            }
+
+            return node;


Suggested change

var node = new JsonArray();

foreach (var value in yaml)

{

node.Add(value.ToJsonNode());

}

return node;

return new JsonArray(yaml.Select(static x => x.ToJsonNode()).ToArray());

Your suggestion allocates an extra array.

right, sorry I meant this

Suggested change

var node = new JsonArray();

foreach (var value in yaml)

{

node.Add(value.ToJsonNode());

}

return node;

return new JsonArray(yaml.Select(static x => x.ToJsonNode()));

src/Microsoft.OpenApi/Models/OpenApiCallback.cs

gregsdennis · 2023-05-20T05:57:21Z

We using this deserialization pattern extensively in our SDK generation and are able to parse unrecognized properties into a dictionary without any problem. - @darrelmiller

My concern was less about unknown keywords and more about keywords defined in custom vocabularies, especially those which provide additional functionality, like assertions. If you're supporting 2020-12, you will need to be able to support this somehow, probably through some extension mechanism where people can define their own keyword implementations.

gregsdennis added 5 commits March 25, 2023 11:30

added draft4 keyword implementations for jsonschema.net; openapidocum…

f25b0e0

…ent should implement ibasedocument; added yaml<->json conversion

pass-through support for draft6+ exclusive*

f9eb5d0

initial experimentation with using jsonschema.net types more directly…

8a6192a

… and stj serialization

added more draft 4 support (need to update jsonschema.net); added dra…

09568a2

…ft 4 meta-schema; started converting tests

converted some of the tests to use JsonSchemaBuilder

d66ea86

gregsdennis commented Mar 26, 2023

View reviewed changes

gregsdennis added 5 commits March 27, 2023 11:28

updated header, media type, and parameter to use jsonschema

2be6fe7

update deserializers that'll probably be removed eventually anyway; u…

a1db8b6

…pdate tests

just serialization things

832efcd

use jsonschema.net v4.0.1

6177047

cleaned up pkg references

c14ca66

darrelmiller reviewed Apr 2, 2023

View reviewed changes

baywet reviewed Apr 4, 2023

View reviewed changes

MaggieKimani1 deleted the branch microsoft:mk/integrate-json-schema-library November 21, 2023 14:41

MaggieKimani1 closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggested path for JsonSchema.Net integration #1191

Suggested path for JsonSchema.Net integration #1191

gregsdennis commented Mar 24, 2023 •

edited

Loading

gregsdennis Mar 26, 2023

darrelmiller Apr 2, 2023

gregsdennis Apr 2, 2023

gregsdennis Mar 26, 2023

darrelmiller Apr 2, 2023

darrelmiller Apr 2, 2023

gregsdennis Apr 2, 2023

darrelmiller Apr 2, 2023

gregsdennis Apr 2, 2023 •

edited

Loading

darrelmiller Apr 4, 2023

darrelmiller Apr 2, 2023

gregsdennis Apr 2, 2023

darrelmiller Apr 2, 2023

gregsdennis Apr 2, 2023 •

edited

Loading

darrelmiller commented Apr 2, 2023 •

edited

Loading

baywet Apr 4, 2023

gregsdennis Apr 4, 2023

baywet Apr 4, 2023

gregsdennis Apr 4, 2023 •

edited

Loading

baywet Apr 4, 2023

gregsdennis Apr 4, 2023

baywet Apr 4, 2023

gregsdennis commented May 20, 2023 •

edited

Loading

	return yaml.Documents.Select(x => x.ToJsonNode());
	return yaml.Documents.Select(static x => x.ToJsonNode());

Suggested path for JsonSchema.Net integration #1191

Suggested path for JsonSchema.Net integration #1191

Conversation

gregsdennis commented Mar 24, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsdennis Apr 2, 2023 • edited Loading

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

gregsdennis Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

darrelmiller commented Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsdennis Apr 4, 2023 • edited Loading

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

gregsdennis commented May 20, 2023 • edited Loading

gregsdennis commented Mar 24, 2023 •

edited

Loading

gregsdennis Apr 2, 2023 •

edited

Loading

gregsdennis Apr 2, 2023 •

edited

Loading

darrelmiller commented Apr 2, 2023 •

edited

Loading

gregsdennis Apr 4, 2023 •

edited

Loading

gregsdennis commented May 20, 2023 •

edited

Loading