saunter icon indicating copy to clipboard operation
saunter copied to clipboard

Use NJsonSchema for the JSON Schema implementation

Open RicoSuter opened this issue 3 years ago • 5 comments

Hi Everyone

I'm the maintainer of NSwag (https://github.com/RicoSuter/NSwag) and I have a proposal.

NSwag is built on top of NJsonSchema (https://github.com/RicoSuter/NJsonSchema) which contains the model, schema generator and code generators for JSON Schema.

Would you be open to use NJsonSchema as the base of Saunter? This way you could leverage all the features of NJS and we could bundle our efforts in building great dev tools...

This would probably solve the following issues:

  • #48 NJS supports both Newtonsoft.Json (uses the contract resolver and not reflection for maximum compatibility) and the new System.Text.Json
  • #20 #22 NJS already supports NRT out-of-the-box via https://github.com/RicoSuter/Namotion.Reflection
  • #8 $refs are correctly resolved (document wide) and can be defined in the model (ActualSchema, Reference properties)

What do you think?

A simple spec implementation (simpler than NSwag) based on NJS can be checked out here, eg the spec model just uses the JsonSchema class: https://github.com/RicoSuter/SigSpec/blob/master/src/SigSpec.Core/SigSpecOperation.cs

/cc @devlux @tehmantra

RicoSuter avatar Nov 04 '20 21:11 RicoSuter

Looks like this solves a fair few problems and prevents Saunter from needing to be so concerned with the schema generation so it can focus on the AsyncAPI specifics.

m-wild avatar Nov 05 '20 10:11 m-wild

Hi @RicoSuter I tried todo the proposed migration. (i mostly wanted to check if that would fix the datetimeoffset issue im having) but I am stuck. can you please have a look at https://github.com/JanEggers/saunter/tree/nschema

first I tried to leave ISchemaRepository of this library in place and use JsonSchema.FromType. that generated a correct schema but didnt populate the ISchemaRepository with the nested schemas.

so I figured it would be better to use your SchemaResolver and call the schemagenerator directly. but when running the tests I see that the SchemaResolver generates multiple schemas but they are all empty.

JanEggers avatar Dec 04 '20 15:12 JanEggers

@JanEggers, I had a look at what you'd done, seemed pretty close. I found a couple of issues...

  • The Schema.Id wasn't being generated -- no idea why -- which caused the .ToDictionary(p => ...(p.Id)) to fail,
  • It seems we need to extend the JsonSchemaResolver so that NJsonSchema is aware of the references vs actual schemas,
  • The JSON serialization is failing with System.Text.Json, but works with Newtonsoft.Json -- due to our custom type converters?

I still can't get the $refs to work...

       "Message": {
          "Headers": null,
          "Payload": {
            "oneOf": [
              {
                "type": "null"
              },
              {
                               // <--- where is $ref?
              }
            ]
          },

@RicoSuter do we have to do something special to get the refs to work? This document is not exactly the same as JSON schema as there is no "definitions" section, but rather a "components" section where many different things can be refs...

https://github.com/tehmantra/saunter/compare/master...nschema

m-wild avatar Dec 17 '20 13:12 m-wild

Had a closer look at SigSpec to figure out how the $ref's work. Seems we we just missing the [JsonConverter(typeof(JsonReferenceConverter))] attribute.

Taking full advantage of NJsonSchema will mean mostly rolling back the work done to replace Newtonsoft.Json with System.Text.Json for any internal serialization. (e.g. serializing the AsyncApiDocument itself). However as NJsonSchema supports generating schemas from both Newtonsoft.Json and System.Text.Json attributes, it will be worth it.

I will continue to work on this.

m-wild avatar Dec 24 '20 06:12 m-wild

Check PR #103 comments - not sure about JSON Schema format.

m-wild avatar Aug 04 '21 23:08 m-wild

Marking this as closed since Saunter does now use NJsonSchema. Further issues (discriminator, nullable) will be dealt with elsewhere if they arise again.

m-wild avatar Aug 22 '22 04:08 m-wild