asyncapi / saunter

Saunter is a code-first AsyncAPI documentation generator for dotnet.
https://www.asyncapi.com/
MIT License
199 stars 56 forks source link

NJsonSchema generates discriminator syntax which is not AsyncAPI compatible #117

Open liam-careerhub opened 3 years ago

liam-careerhub commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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. https://github.com/asyncapi/spec/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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)