feat: json schemas for bindings

[KhudaDad414]

Description

• this PR will add all of the JSON Schemas for different binding in different protocols.

Related issue(s)
See also #507

[github-actions]

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

[magicmatatjahu]

@KhudaDad414 Thanks for handling this task! I know that it's draft, but I advise you to keep the schemas in some folder, e.g. amqp/json_schemas and have a schema for messages, operations, etc. like amqp/json_schemas/message.json. What do you think about that? :)

[KhudaDad414]

@KhudaDad414 Thanks for handling this task! I know that it's draft, but I advise you to keep the schemas in some folder, e.g. amqp/json_schemas and have a schema for messages, operations, etc. like amqp/json_schemas/message.json. What do you think about that? :)

thanks for the input. I will put them inside a folder. any other advice regarding schema?

[derberg]

I think we need this https://github.com/asyncapi/asyncapi-node/blob/master/schemas/2.0.0.json#L10-L15 on every schema file to make sure no additional props are added, but extensions (props with x-) are allowed.

@fmvilas on bindings you can find sentences that only props from given list are allowed but I guess we still want to have specific bindings extendable, right?

[magicmatatjahu]

@KhudaDad414 I added some comments :) Good progress!

[KhudaDad414]

@derberg @magicmatatjahu The schemas are completed.

[GeraldLoeffler]

I suggest to:

• use JSON Schema draft-07, just like the AsyncAPI spec
• give every schema a unique ID, maybe following the pattern "$id": "http://asyncapi.com/<protocol>/bindings/server.json"
• use "additionalProperties": true throughout for forwards compatibility

[derberg]

I agree with @GeraldLoeffler with all except the additionalProperties, it must be false and only extensions should be allowed (as with patternProperties) like it is with the main spec.

One more comment from my side: definition.specificationExtension is repeated everywhere, I'm wondering if it can be moved to a single fine in this repo and referenced everywhere, or we should refer this definition from the spec file. I don't have strong opinion here, thoughts?

@rcoppen would you like to review schemas provided for IBMMQ?

[GeraldLoeffler]

fwiw, here is the section from my work-in-progress bindings specification that argues for additionalProperties: true: https://github.com/integrational/asyncapi-bindings/tree/master/anypointmq#backwards-compatibility

[derberg]

@GeraldLoeffler I looked at it and

All bindings defined in this specification allow additional, unspecified fields, so that a concrete instance of a binding object MAY contain arbitrary fields not defined in this specification

This is exactly the purpose of extensions.

As an example of backwards compatibility, consider a concrete binding object valid against version 1.1.0 of this specification. This binding object may specify bindingVersion: 1.1.0. This binding object may contain fields specific to version 1.1.0, which were not yet defined in, say, version 1.0.5 of this specification. However, because additional fields are allowed, this binding object is also valid against version 1.0.5 of this specification. The binding object valid against version 1.1.0 can therefore be used in a system that is only aware of version 1.0.5, although all fields not yet defined in version 1.0.5 of this specification will be ignored.

this is somehow valid, but did you consider that someone can add a property that means "A" and with the next binding version we add another property like that, but with a different meaning, "B"? and it can lead to a problem. This is why extensions are always with prefix

[KhudaDad414]

@derberg made the recommended changes and used https://github.com/asyncapi/asyncapi-node/blob/master/schemas/2.0.0.json#/definitions/schema instead of http://json-schema.org/draft-07/schema inside specification.

[derberg]

Oh man this was a tremendous effort from your side, hats off 🙇🏼
Sorry for delayed review but I had some bank holidays here in Poland.

I left few comments, sorry but mainly to improve schemas. I did amqp, http and ibmmq only as I noticed that things that needs improvement are mostly around the same, oneOf or default

Also please add description property and example property to all the properties from all the schemas. On the other hand it is not a hard requirement for me for a 1st version, you can do it in another PR if you do not want to overcomplicate this one.

For next review round, please, when you push fixes, don't do it randomly or in one huge commit. Would be awesome if you could do commit per fix. I mean, when you remove "default": "latest" from all the bindingVersion, then just do this change in all files and push as single commit. And then the rest

[derberg]

one more thing, for $id I think it is better to do http://asyncapi.com/bindings/amqp/channel.json instead of http://asyncapi.com/amqp/bindings/channel.json

[KhudaDad414]

@derberg Thanks for the review. I think I made all of the requested changes.

[derberg]

I'm afraid you need to look again into those oneOf. They seem to not work, I tried some online schema validation, to validate example from binding with schema, first from the like, amqp and there were errors related to oneOf.

We use AJV validator in the parser, so best would be to test those schemas against official bindings examples.

also, what about my comment about descriptions and examples?

last but not least -> for bindingVersion I think we need const pointing to the versions number`

Let me know if you need some help with oneOf. I recommend trying a combination of oneOf and allOff, I tried your current approach once and then skipped to use oneOf with allOf

[KhudaDad414]

@derberg thanks again for the review. I hope I am not wasting too much of your valuable time on this PR but I wanted to ask which website are you using, I'm using https://www.jsonschemavalidator.net/ and it is validating perfectly.
About description and example, I totally forgot about them. just started working on them.
and about bindingVersion I don't exactly know what to do. should I use the version that is inside README.md file as the default value?

[derberg]

@KhudaDad414 yup, I was using https://www.jsonschemavalidator.net/ and yeah, I see it works now 🤔 I must have done something wrong judging to quickly, sorry 🙏🏼

and about bindingVersion I don't exactly know what to do. should I use the version that is inside README.md file as the default value?

yeap, have a look at asyncapi spec for example https://github.com/asyncapi/asyncapi-node/blob/master/schemas/2.0.0.json#L17-L22

[derberg]

sorry, wrong button 😅

[KhudaDad414]

@derberg A review would be appreciated :)

[magicmatatjahu]

@KhudaDad414 Related to oneOf and not: required: [], you can use false schema like (for example amqp):

  "oneOf": [
    {
      "properties": {
        "is": { "const": "routingKey" },
        "queue": false
      },
      "required": [
        "exchange"
      ]
    },
    {
      "properties": {
        "is": { "const": "queue" },
        "exchange": false
      },
      "required": [
        "queue"
      ]
    }
  ],

But using not: required is also ok :)

[KhudaDad414]

@KhudaDad414 Related to oneOf and not: required: [], you can use false schema like (for example amqp):

  "oneOf": [
    {
      "properties": {
        "is": { "const": "routingKey" },
        "queue": false
      },
      "required": [
        "exchange"
      ]
    },
    {
      "properties": {
        "is": { "const": "queue" },
        "exchange": false
      },
      "required": [
        "queue"
      ]
    }
  ],

But using not: required is also ok :)

Thanks, didn't know that.🙂

[derberg]

@KhudaDad414 this is looking great now. This is a gigantic effort you did here! like seriously 🚀

once we merge, we need to think on the next step, how to integrate it with asyncapi-node repo