feat: enable AsyncAPI v3 validation #825

smoya · 2023-08-01T11:24:52Z

Tests are failing. Read comments below.

smoya · 2023-08-01T14:14:06Z

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 · 2023-08-01T14:21:36Z

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

To fix the bug in the error messages filter, so the error that stays is the one that points to each operation.channel field.
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.

src/ruleset/formats.ts

…validate

smoya · 2023-08-07T11:28:50Z

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 · 2023-08-07T13:03:43Z

@smoya Issues are fixed :)

smoya · 2023-08-08T08:20:13Z

@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 · 2023-08-08T08:24:18Z

@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 · 2023-08-08T09:20:00Z

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 😄

test/ruleset/formats.spec.ts

jonaslagoni · 2023-08-08T09:47:37Z

test/ruleset/rules/asyncapi-document-resolved.spec.ts

+  },
+
+  {
+    name: 'valid case for 3.X.X (case when other $ref errors should also occur but we filter them out)',


Are these names on cases still valid?

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

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 :)

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.

Changed. Thanks!

sonarcloud · 2023-08-08T11:42:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

jonaslagoni

👍

jonaslagoni · 2023-08-09T11:15:22Z

/rtm

asyncapi-bot · 2023-08-17T08:57:59Z

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

The release is available on:

Your semantic-release bot 📦🚀

jonaslagoni marked this pull request as ready for review August 1, 2023 11:25

jonaslagoni requested review from fmvilas, magicmatatjahu, jonaslagoni and asyncapi-bot-eve as code owners August 1, 2023 11:25

feat: enable AsyncAPI v3 validation

7b4d21c

smoya force-pushed the feat/enableV3Validation branch from cb22b1a to 7b4d21c Compare August 1, 2023 11:27

smoya mentioned this pull request Aug 2, 2023

feat: add spec 3.0 validation support asyncapi/studio#747

Merged

smoya force-pushed the feat/enableV3Validation branch from 2be79d9 to 37c28bf Compare August 4, 2023 12:04

smoya commented Aug 4, 2023

View reviewed changes

src/ruleset/formats.ts Outdated Show resolved Hide resolved

smoya force-pushed the feat/enableV3Validation branch from 37c28bf to 511fb5f Compare August 4, 2023 12:08

allow Channel instead of reference for operation.channel in order to …

7128146

…validate

smoya force-pushed the feat/enableV3Validation branch from 511fb5f to 7128146 Compare August 4, 2023 12:58

changes

6038799

smoya mentioned this pull request Aug 8, 2023

Avoid if posible to manipulate the schema in rules for v3 #827

Closed

add comment

a858c62

jonaslagoni requested changes Aug 8, 2023

View reviewed changes

smoya added 2 commits August 8, 2023 12:14

remove unused code

8d9c9e6

clarify testcase

ccb3c85

smoya requested a review from jonaslagoni August 8, 2023 11:41

jonaslagoni approved these changes Aug 9, 2023

View reviewed changes

asyncapi-bot added the ready-to-merge label Aug 9, 2023

asyncapi-bot merged commit 3fa3290 into asyncapi:next-major-spec Aug 9, 2023
11 checks passed

asyncapi-bot added the released on @next-major-spec label Aug 17, 2023

smoya deleted the feat/enableV3Validation branch December 11, 2023 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable AsyncAPI v3 validation #825

feat: enable AsyncAPI v3 validation #825

smoya commented Aug 1, 2023 •

edited

Loading

smoya commented Aug 1, 2023 •

edited

Loading

smoya commented Aug 1, 2023

smoya commented Aug 7, 2023 •

edited

Loading

magicmatatjahu commented Aug 7, 2023

smoya commented Aug 8, 2023 •

edited

Loading

magicmatatjahu commented Aug 8, 2023 •

edited

Loading

smoya commented Aug 8, 2023

jonaslagoni left a comment

jonaslagoni Aug 8, 2023

smoya Aug 8, 2023

magicmatatjahu Aug 8, 2023 •

edited

Loading

magicmatatjahu Aug 8, 2023

smoya Aug 8, 2023

sonarcloud bot commented Aug 8, 2023

jonaslagoni left a comment

jonaslagoni commented Aug 9, 2023

asyncapi-bot commented Aug 17, 2023

feat: enable AsyncAPI v3 validation #825

feat: enable AsyncAPI v3 validation #825

Conversation

smoya commented Aug 1, 2023 • edited Loading

smoya commented Aug 1, 2023 • edited Loading

smoya commented Aug 1, 2023

smoya commented Aug 7, 2023 • edited Loading

magicmatatjahu commented Aug 7, 2023

smoya commented Aug 8, 2023 • edited Loading

magicmatatjahu commented Aug 8, 2023 • edited Loading

smoya commented Aug 8, 2023

jonaslagoni left a comment

Choose a reason for hiding this comment

jonaslagoni Aug 8, 2023

Choose a reason for hiding this comment

smoya Aug 8, 2023

Choose a reason for hiding this comment

magicmatatjahu Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

magicmatatjahu Aug 8, 2023

Choose a reason for hiding this comment

smoya Aug 8, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Aug 8, 2023

jonaslagoni left a comment

Choose a reason for hiding this comment

jonaslagoni commented Aug 9, 2023

asyncapi-bot commented Aug 17, 2023

smoya commented Aug 1, 2023 •

edited

Loading

smoya commented Aug 1, 2023 •

edited

Loading

smoya commented Aug 7, 2023 •

edited

Loading

smoya commented Aug 8, 2023 •

edited

Loading

magicmatatjahu commented Aug 8, 2023 •

edited

Loading

magicmatatjahu Aug 8, 2023 •

edited

Loading