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.

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

Use NJsonSchema for the JSON Schema implementation #60

Closed
RicoSuter opened this issue Nov 4, 2020 · 6 comments · Fixed by #103
Closed

Use NJsonSchema for the JSON Schema implementation #60

RicoSuter opened this issue Nov 4, 2020 · 6 comments · Fixed by #103
Labels
💡 enhancement New feature or request

Comments

@RicoSuter
Copy link
Contributor

RicoSuter commented Nov 4, 2020

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:

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

@m-wild
Copy link
Collaborator

m-wild commented Nov 5, 2020

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 m-wild added the 💡 enhancement New feature or request label Nov 23, 2020
@JanEggers
Copy link
Contributor

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.

@m-wild m-wild mentioned this issue Dec 17, 2020
@m-wild
Copy link
Collaborator

m-wild commented Dec 17, 2020

@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...

master...nschema

@m-wild
Copy link
Collaborator

m-wild commented Dec 24, 2020

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
Copy link
Collaborator

m-wild commented Aug 4, 2021

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

@m-wild
Copy link
Collaborator

m-wild commented Aug 22, 2022

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 m-wild closed this as completed Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants