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

NJsonSchema generates discriminator syntax which is not AsyncAPI compatible #117

Open
liam-careerhub opened this issue Aug 18, 2021 · 7 comments

Comments

@liam-careerhub
Copy link

According to the spec, AsyncAPI should have a discriminator as a string,
https://www.asyncapi.com/docs/specifications/v2.1.0#schemaObject

However, NJsonSchema generates an object instead with a propertyName and mappings fields, which is valid according to the OpenAPI spec

https://github.com/RicoSuter/NJsonSchema/blob/288eb13e931562264ed9ddd12a83741c67a1ddc4/src/NJsonSchema/OpenApiDiscriminator.cs

https://swagger.io/specification/#discriminator-object

This means that Saunter generates invalid AsyncAPI code.

If you call iPetSchema.ToJson() one line 145 in the following file, you'll see the generated JSON and how it doesn't match the requirements:
https://github.com/tehmantra/saunter/blob/main/test/Saunter.Tests/Generation/SchemaGeneration/SchemaGenerationTests.cs

I've reverted to 0.4.0 for now as it's the last version of Saunter with working discriminator support.

I can't see an obvious way to adjust the generated schema in NJsonSchema, maybe this is something @RicoSuter can help with?

Let me know if there's any more information I can provide.

@m-wild
Copy link
Collaborator

m-wild commented Aug 18, 2021

More frustrations with the different schema formats 😞
There is quite a lengthy discussion in #103 about the correct way to represent nulls, and now we have more issues with the discriminator...

@RicoSuter it seems like we might need to have a specific format for AsyncAPI
I know we have the SchemaType enum, but it seems like this would have to be added directly into NJsonSchema. Is there a way we can support a custom serialization output without having to build it directly into NJsonSchema?

Thanks for your help.

@liam-careerhub
Copy link
Author

Yeah, I noticed the null issues too, where the generated schema has a lot of null values with the AsyncAPI UI doesn't like.

Maybe the best solution to this is to create a fork of NJsonSchema specifically for AsyncAPI? That is unless @RicoSuter thinks these customisations should be in NJsonSchema itself. That way you can take advantage of development in that project and curate how that merges into the AsyncApi variant?

Either that or NJsonSchema's serialiser would need to be completely customisable (if it isn't already, my knowledge is limited here.)

I think this project (Saunter) has huge potential, I'm a big fan of Swashbuckle and an AsyncAPI equivalent is something which I think is hugely valuable.

Let's see what Rico says I guess.

@RicoSuter
Copy link
Contributor

Can someone clarify what the correct null handling should be?

Is it the same as with OpenAPI or JSON Schema or again completely different?

I'm fine adding the AsyncAPI enum, had to do this for OpenAPI/Swagger as well because it's not just a simple rename of a property but the schema generator needs to generate "completely different" schemas...

@m-wild
Copy link
Collaborator

m-wild commented Nov 4, 2021

Thanks for replying @RicoSuter :)

In this case the issue is how to support polymorphism.


For representing nullables.
As of today AsyncAPI is JsonSchema-draft-07 compatible, which means we need to do:
For simple/primitive properties:

properties:
  foo:
    type: [string, 'null']

For references to other schema:

properties:
  foo:
    oneOf:
    - type: 'null'
    - '$ref': '#/components/schemas/foo'

Once AysncAPI supports later versions of JsonSchema, we can use $ref and type together.
asyncapi/spec#596

properties:
  foo:
    type: [ object, 'null' ]
    '$ref': '#/components/schemas/foo'

It seems like the discriminator issue alone is enough to force us to implement a different SchemaType in NJsonSchema?

@RicoSuter
Copy link
Contributor

It seems like the discriminator issue alone is enough to force us to implement a different SchemaType in NJsonSchema?

So we would add AsyncApi2 (and later AsyncApi3) or just AsyncApi as new SchemaType?

@m-wild
Copy link
Collaborator

m-wild commented Nov 24, 2021

That seems like the most appropriate option :)
Given we currently have values like SchemaType.OpenApi3 and SchemaType.Swagger2, it wouldn't be crazy to have SchemaType.AsyncApi2.

Alternative might be to have NJsonSchema allow custom SchemaTypes somehow?
But I'd be fine with just adding SchemaType.AsyncApi2.

Would you like me to work on this and submit a PR to NJsonSchema?

@RicoSuter
Copy link
Contributor

Alternative might be to have NJsonSchema allow custom SchemaTypes somehow?

problem is that njs needs specific code based on the schema anyway, no? Here for example a different inheritance generation.. another option is that you inherit from JsonSchemaGenerator and tweak the behavior with overrides.. that would be nicer in general (ie get rid of the schematype)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants