feat: enable AsyncAPI v3 validation

[smoya]

Tests are failing. Read comments below.

[smoya]

Validation fails with the following errors:

[
   {
      "code":"asyncapi-document-resolved",
      "message":"'someOperation1' property must match exactly one schema in oneOf",
      "path":[
         "operations",
         "someOperation1"
      ],
      "range":{
         "end":{
            "character":46,
            "line":20
         },
         "start":{
            "character":21,
            "line":10
         }
      },
      "severity":0
   },
   {
      "code":"asyncapi-document-resolved",
      "message":"'someOperation2' property must match exactly one schema in oneOf",
      "path":[
         "operations",
         "someOperation2"
      ],
      "range":{
         "end":{
            "character":46,
            "line":35
         },
         "start":{
            "character":21,
            "line":24
         }
      },
      "severity":0
   }
]

The validation rule that fails is the asyncapi-document-resolved, which checks if the AsyncAPI document is valid (using JSON Schema) after resolving references.
Tests are failing due to the fact channel property under Operation one should be just a reference to a channel and not a channel object directly. This is the very first time we don't allow a field to, besides being a reference, to also be defined inline. The asyncapi-document-resolved rule applies after resolving the reference for the channel object, which becomes an invalid document since the expected channel value should be always a reference.

Thanks @jonaslagoni for helping (and actually finding) that out.

The error messages shown above are also wrong. We do some error filtering as stated in https://github.com/asyncapi/parser-js/blob/master/src/ruleset/functions/documentStructure.ts#L20, and it seems we are doing too much and, in this case, we skip the real error, which can be found in the list of errors prior to the filtering:

[
  {
    "instancePath": "/operations/someOperation1",
    "schemaPath": "#/required",
    "keyword": "required",
    "params": {
      "missingProperty": "$ref"
    },
    "message": "must have required property '$ref'"
  },
  {
    "instancePath": "/operations/someOperation1/channel",
    "schemaPath": "#/required",
    "keyword": "required",
    "params": {
      "missingProperty": "$ref"
    },
    "message": "must have required property '$ref'"
  },
  {
    "instancePath": "/operations/someOperation1",
    "schemaPath": "#/additionalProperties/oneOf",
    "keyword": "oneOf",
    "params": {
      "passingSchemas": null
    },
    "message": "must match exactly one schema in oneOf"
  },
  {
    "instancePath": "/operations/someOperation2",
    "schemaPath": "#/required",
    "keyword": "required",
    "params": {
      "missingProperty": "$ref"
    },
    "message": "must have required property '$ref'"
  },
  {
    "instancePath": "/operations/someOperation2/channel",
    "schemaPath": "#/required",
    "keyword": "required",
    "params": {
      "missingProperty": "$ref"
    },
    "message": "must have required property '$ref'"
  },
  {
    "instancePath": "/operations/someOperation2",
    "schemaPath": "#/additionalProperties/oneOf",
    "keyword": "oneOf",
    "params": {
      "passingSchemas": null
    },
    "message": "must match exactly one schema in oneOf"
  }
]

The expected error should be the ones with an instancePath pointing to the operation channel, where the message says must have required property '$ref'.

[smoya]

Based on my prior comment, this is what we need to do next in this PR:

1. To fix the bug in the error messages filter, so the error that stays is the one that points to each operation.channel field.
2. To skip validation of operation.channel for v3 in the asyncapi-document-resolved (still will happen in asyncapi-document-unresolved).

Alternatives for the second point will be to either allow by JSON Schema to define operation.channel as object aswell, which will then incompatible with the current spec, or rather to keep different JSON Schema files, one for pre and another for post reference resolution. I think this is also discarded as it is a lot of work and effort.

[smoya]

I’m thinking that we should not go in the direction of manipulating the schema for the operation.channel in the mid term. Having to manipulate the schema from now on is not either good nor maintainable IMHO.

The alternative is to remove the asyncapi-document-resolved rule forever and just relay on the asyncapi-document-unresolved. The point is that we are filtering errors here, so we would need to return them and not filter. What is the real implication of it?

WDYT @magicmatatjahu @jonaslagoni

[magicmatatjahu]

@smoya Issues are fixed :)

[smoya]

@smoya Issues are fixed :)

Thanks for the help @magicmatatjahu 🙌 🙌 !!

I still have this concern and I would love to know your opinion as you are the one with most knowledge on this code.

I would like to have this PR merged but to keep iterating after so we go for a better and maintainable alternative.

I created a follow up issue so we don't block this anymore.

cc @jonaslagoni @Amzani

[magicmatatjahu]

@smoya

I’m thinking that we should not go in the direction of manipulating the schema for the operation.channel in the mid term. Having to manipulate the schema from now on is not either good nor maintainable IMHO.

I agree that it's not good but parser should throws all errors, so I can accept situation when the source code is "ugly" but DX is better. We have also option to change the JSON Schemas to allow also inline objects in given places, but it won't follow the spec. There is also a idea to allow inline objects in spec itself - but then 3.1.0 is needed.

The alternative is to remove the asyncapi-document-resolved rule forever and just relay on the asyncapi-document-unresolved. The point is that we are filtering errors here, so we would need to return them and not filter. What is the real implication of it?

asyncapi-document-resolved is very needed because we should also check the correctness of imported objects from external files/https (external refs). Without this rule how to check if something imported is ok?

[smoya]

There is also a idea to allow inline objects in spec itself - but then 3.1.0 is needed.

Yeah, this is something I want to bring up and discuss with the community.

asyncapi-document-resolved is very needed because we should also check the correctness of imported objects from external files/https (external refs). Without this rule how to check if something imported is ok?

Yeah, I agree there is no better alternative and, for sure, this validation is an important addition on top of the basic schema validation.

Maybe we could work on an automation, for every time we find a property with only a reference to a Reference Object , to modify that schema to allow the object as well (oneOf or just right the definition object as you did).

[jonaslagoni]

As I see it there is one additional approach that is unmentioned, and that is we could make the reference resolver ignore the specific path i.e.

• channelObject.properties.servers.items.$ref
• operationSchema.properties.channel.$ref
• operationSchema.properties.messages.items.$ref
• operationReplySchema.properties.channel.$ref
• operationReplySchema.properties.messages.items.$ref
  Just for that specific rule, but it might be hard to pull of. There are also some more unknowns such as can the library we use do this? (I know https://apitools.dev/json-schema-ref-parser/docs/options.html can) Can we still ensure the refs are correct and point to a valid location?

So yea, I think your current solution makes sense 👍

@smoya have you added enough test cases to test for all those paths and that they are caught?

The alternative is to remove the asyncapi-document-resolved rule forever and just relay on the asyncapi-document-unresolved. The point is that we are filtering errors here, so we would need to return them and not filter. What is the real implication of it?

What @magicmatatjahu said 👍

There is also a idea to allow inline objects in spec itself - but then 3.1.0 is needed.

I don't see this as making much sense from a spec point of view, so it might be really hard to do just because reference resolvement becomes hard to do 😄

[jonaslagoni]

Are these names on cases still valid?

[smoya]

@magicmatatjahu pinging you because you added this test. What are the errors you expect here to be filtered?

[magicmatatjahu]

Spectral works in two modes - with schema resolved and unresolved (with untouched refs). Even if we wrote a ref in a place then Spectral would not throw an error (because it operates on resolved schema). This is more of a test for checking if rule isn't "breaking" resolved/unresolved spectral behaviour, just in case for us :)

[magicmatatjahu]

If you want you can change the name of the test because it should have a different name, I can't remember why I called it that.

[smoya]

Changed. Thanks!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[jonaslagoni]

👍

[jonaslagoni]

/rtm

[asyncapi-bot]

🎉 This PR is included in version 2.2.0-next-major-spec.1 🎉

The release is available on:

• npm package (@next-major-spec dist-tag)
• GitHub release

Your semantic-release bot 📦🚀