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

fix: wrong validation with AsyncAPI v3 in VS Code and JetBrains IDEs #495

Merged
merged 21 commits into from
Apr 3, 2024

Conversation

Pakisan
Copy link
Member

@Pakisan Pakisan commented Mar 10, 2024

Fix specification validation erros in JetBrains and VS Code

Closes #494
Closes asyncapi/jasyncapi-idea-plugin#49

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide 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.

…erencing a a json schema

Fix missing queue definition for SQS channel binding

asyncapi#494
asyncapi/jasyncapi-idea-plugin#49
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pakisan you need to adapt the definition files, not the bundled schemas 🙂

@Pakisan
Copy link
Member Author

Pakisan commented Mar 11, 2024

@Pakisan you need to adapt the definition files, not the bundled schemas 🙂

Yeah, expected this reply😅, because I saw that schemas were generated automatically

How to build schemas? I need to change only definitions?

@jonaslagoni
Copy link
Member

jonaslagoni commented Mar 11, 2024

@Pakisan you can run the npm command npm run bundle to trigger the bundling process.

Yep, only need to change the definitions, and everything else will be taken care of in the release process 🙂

@Pakisan
Copy link
Member Author

Pakisan commented Mar 11, 2024

@Pakisan you can run the npm command npm run bundle to trigger the bundling process.

Yep, only need to change the definitions, and everything else will be taken care of in the release process 🙂

It may be related with references availability?

I can open https://asyncapi.com/definitions/3.0.0/Reference.json, but can't https://www.asyncapi.com/bindings/sqs/0.2.0/channel.json

~ curl https://www.asyncapi.com/bindings/sqs/0.2.0/channel.json
<!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="/_next/static/css/70fc8c1b3046a2a5.css" as="style"/><link rel="stylesheet" href="/_next/static/css/70fc8c1b3046a2a5.css" data-n-g=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="/_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="/_next/static/chunks/webpack-309fbebe2073f18c.js" defer=""></script><script src="/_next/static/chunks/framework-79bce4a3a540b080.js" defer=""></script><script src="/_next/static/chunks/main-5ab4e93313090fd8.js" defer=""></script><script src="/_next/static/chunks/pages/_app-e51460248b2aa259.js" defer=""></script><script src="/_next/static/chunks/pages/404-21869d41b96d4a23.js" defer=""></script><script src="/_next/static/uYuiB21UZi5IxEWUG1fEZ/_buildManifest.js" defer=""></script><script src="/_next/static/uYuiB21UZi5IxEWUG1fEZ/_ssgManifest.js" defer=""></script></head><body><div id="__next" data-reactroot=""></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{}},"page":"/404","query":{},"buildId":"uYuiB21UZi5IxEWUG1fEZ","nextExport":true,"autoExport":true,"isFallback":false,"scriptLoader":[]}</script></body></html>%

Because SQS channels has right reference definition:

"deadLetterQueue": {
"description": "A definition of the queue that will be used for un-processable messages.",
"$ref": "http://asyncapi.com/bindings/sqs/0.2.0/channel.json#/definitions/queue"
},

which will be handled wrongly

"deadLetterQueue": {
"description": "A definition of the queue that will be used for un-processable messages.",
"$ref": "#/definitions/bindings-sqs-0.2.0-channel"
},

@jonaslagoni
Copy link
Member

If you are using any of the schemas in https://github.com/asyncapi/spec-json-schemas/tree/master/schemas, you don't need to go remote to fetch the schemas, its only if you work with a definition for example.

The way the $id and $ref works in these definitions is that the bundler matches them locally, the same when you try to validate with the bundled schemas, that's simply part of the JSON Schema spec, that if a resource can be found locally that will be used.

So basically, just see the references as matching together, even if the remote URL does not exist 🙂 Even though it definitely should at some point

@jonaslagoni
Copy link
Member

@smoya what do we need to change to publish the bindings publically?

…erencing a a json schema

- Revert changes from auto generated schema
- Change allOf to anyOf in multiFormatSchema

asyncapi#494
asyncapi/jasyncapi-idea-plugin#49
Copy link
Member

smoya commented Mar 11, 2024

@jonaslagoni to either create a new or modify the following Netlify edge-function https://github.com/asyncapi/website/blob/master/netlify/edge-functions/serve-definitions.ts and add a new redirection rule into https://github.com/asyncapi/website/blob/master/netlify.toml

@Pakisan Pakisan requested a review from jonaslagoni March 12, 2024 13:06
@Pakisan Pakisan changed the title WIP: fix: AsyncAPI v3 shows warning in Studio and IntelliJ plugin when ref… fix: AsyncAPI v3 shows warning in Studio and IntelliJ plugin when ref… Mar 12, 2024
@Pakisan Pakisan changed the title fix: AsyncAPI v3 shows warning in Studio and IntelliJ plugin when ref… fix: wrong validation with AsyncAPI v3 in VS Code and JetBrains IDEs Mar 12, 2024
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch and fix 👌

As I am not codeowner someone else gotta have a look 😄

cc @GreenRover, @char0n, @dalelane, @derberg, @fmvilas, @smoya

@Pakisan
Copy link
Member Author

Pakisan commented Mar 12, 2024

@fmvilas @derberg @dalelane @smoya @char0n @GreenRover

Sorry for massive calling, but this issue affects users in VS Code and JetBrains IDEs

I checked fix here - https://www.jsonschemavalidator.net and inside my plugin

Because our cli is thinking that everything is ok with provided schemas - asyncapi/jasyncapi-idea-plugin#49 (comment)

GreenRover
GreenRover previously approved these changes Mar 12, 2024
Copy link
Collaborator

@GreenRover GreenRover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i not understand the if then construct but the rest looks good.

dalelane
dalelane previously approved these changes Mar 14, 2024
@Pakisan
Copy link
Member Author

Pakisan commented Mar 27, 2024

@Pakisan would highly recommend splitting the bundler fix for bindings into a separate PR so it's not blocked by the continued discussion regarding multiformat schema object.

Main problem, after addition of new key word schemaFormat, is collisions between Schema, Json Schema, Avro Schema with weak constraints for additional properties. That's why I have added additionalProperties: false to Avro, for example

There really should be no collision when the validation occurs based on the conditioning of the value in schemaFormat 🤔 additionalProperties should have no effect other then for the schema format in use.

{
  "$id": "http://asyncapi.com/definitions/3.0.0/anySchema.json",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "description": "An object representing either a Reference, a Schema or a Multi Format Schema",
  "properties": {
    "schemaFormat": {
      "type": "string",
      "description": "Supported Schema format"
    },
    "schema": {
      "type": "object",
      "description": "Schema definition"
    },
    "$ref": {
      "type": "string",
      "description": "Reference to schema"
    }
  },
  "oneOf": [
    {
      "description": "Because of $ref collision in Reference and AsyncAPI Schema(includes $ref from Json Schema)",
      "not": {"required": ["schemaFormat", "schema"]},
      "$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
    },
    {
      "type": "object",
      "required": ["schemaFormat", "schema"],
      "not": {"required": ["$ref"]},
      "minProperties": 2,
      "maxProperties": 2,
      "$ref": "http://asyncapi.com/definitions/3.0.0/multiFormatSchema.json"
    }
  ]
}

Here is simplified restriction to tell validator which schema to use. Our tests ok, because AJV is not in strict mode, but other validators, for example this will tell you that this two variants are satisfied:

Screenshot 2024-03-27 at 15 18 58

It's because it checks:

  • multiFormatSchema requires this two fields - OK // expected
  • schema allows additional properties - OK // not expected

and as result, it tells that oneOf is not satisfied

@jonaslagoni
Copy link
Member

jonaslagoni commented Mar 27, 2024

@Pakisan I took a look at the old schemas and tested it locally which seems to work fine with just 3 changes.

  1. remove disabling of additionalProperties:
    "additionalProperties": false,
  2. remove the property schema:
  3. Remove clashing $ref for multiformat schema (i.e. when http://json-schema.org/draft-07/schema or http://asyncapi.com/definitions/3.0.0/schema.json is used only reference that and not http://asyncapi.com/definitions/3.0.0/Reference.json in union)
Full schema here:
{
  "description": "The Multi Format Schema Object represents a schema definition. It differs from the Schema Object in that it supports multiple schema formats or languages (e.g., JSON Schema, Avro, etc.).",
  "type": "object",
  "patternProperties": {
    "^x-[\\w\\d\\.\\x2d_]+$": {
      "$ref": "http://asyncapi.com/definitions/3.0.0/specificationExtension.json"
    }
  },
  "if": {
    "not": {
      "type": "object"
    }
  },
  "then": {
    "$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
  },
  "else": {
    "properties": {
      "schemaFormat": {
        "description": "A string containing the name of the schema format that is used to define the information. If schemaFormat is missing, it MUST default to application/vnd.aai.asyncapi+json;version={{asyncapi}} where {{asyncapi}} matches the AsyncAPI Version String. In such a case, this would make the Multi Format Schema Object equivalent to the Schema Object. When using Reference Object within the schema, the schemaFormat of the resource being referenced MUST match the schemaFormat of the schema that contains the initial reference. For example, if you reference Avro schema, then schemaFormat of referencing resource and the resource being reference MUST match.",
        "anyOf": [
          {
            "type": "string"
          },
          {
            "description": "All the schema formats tooling MUST support",
            "enum": [
              "application/schema+json;version=draft-07",
              "application/schema+yaml;version=draft-07",

              "application/vnd.aai.asyncapi;version=3.0.0",
              "application/vnd.aai.asyncapi+json;version=3.0.0",
              "application/vnd.aai.asyncapi+yaml;version=3.0.0"
            ]
          },
          {
            "description": "All the schema formats tools are RECOMMENDED to support",
            "enum": [
              "application/vnd.oai.openapi;version=3.0.0", 
              "application/vnd.oai.openapi+json;version=3.0.0", 
              "application/vnd.oai.openapi+yaml;version=3.0.0",

              "application/vnd.apache.avro;version=1.9.0",
              "application/vnd.apache.avro+json;version=1.9.0",
              "application/vnd.apache.avro+yaml;version=1.9.0",
              
              "application/raml+yaml;version=1.0"
            ]
          }
        ]
      }
    },
    "allOf": [
      {
        "if": {
          "not": {
            "description": "If no schemaFormat has been defined, default to schema or reference",
            "required": [
              "schemaFormat"
            ]
          }
        },
        "then": {
          "properties": {
            "schema": {
              "$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
            }
          }
        }
      },
      {
        "if": {
          "description": "If schemaFormat has been defined check if it's one of the AsyncAPI Schema Object formats",
          "required": [
            "schemaFormat"
          ],
          "properties": {
            "schemaFormat": {
              "enum": [
                "application/vnd.aai.asyncapi;version=2.0.0",
                "application/vnd.aai.asyncapi+json;version=2.0.0",
                "application/vnd.aai.asyncapi+yaml;version=2.0.0",
                "application/vnd.aai.asyncapi;version=2.1.0",
                "application/vnd.aai.asyncapi+json;version=2.1.0",
                "application/vnd.aai.asyncapi+yaml;version=2.1.0",
                "application/vnd.aai.asyncapi;version=2.2.0",
                "application/vnd.aai.asyncapi+json;version=2.2.0",
                "application/vnd.aai.asyncapi+yaml;version=2.2.0",
                "application/vnd.aai.asyncapi;version=2.3.0",
                "application/vnd.aai.asyncapi+json;version=2.3.0",
                "application/vnd.aai.asyncapi+yaml;version=2.3.0",
                "application/vnd.aai.asyncapi;version=2.4.0",
                "application/vnd.aai.asyncapi+json;version=2.4.0",
                "application/vnd.aai.asyncapi+yaml;version=2.4.0",
                "application/vnd.aai.asyncapi;version=2.5.0",
                "application/vnd.aai.asyncapi+json;version=2.5.0",
                "application/vnd.aai.asyncapi+yaml;version=2.5.0",
                "application/vnd.aai.asyncapi;version=2.6.0",
                "application/vnd.aai.asyncapi+json;version=2.6.0",
                "application/vnd.aai.asyncapi+yaml;version=2.6.0",
                "application/vnd.aai.asyncapi;version=3.0.0",
                "application/vnd.aai.asyncapi+json;version=3.0.0",
                "application/vnd.aai.asyncapi+yaml;version=3.0.0"
              ]
            }
          }
        },
        "then": {
          "properties": {
            "schema": {
              "$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
            }
          }
        }
      },
      {
        "if": {
          "required": [
            "schemaFormat"
          ],
          "properties": {
            "schemaFormat": {
              "enum": [
                "application/schema+json;version=draft-07",
                "application/schema+yaml;version=draft-07"
              ]
            }
          }
        },
        "then": {
          "properties": {
            "schema": {
              "$ref": "http://json-schema.org/draft-07/schema"
            }
          }
        }
      },
      {
        "if": {
          "required": [
            "schemaFormat"
          ],
          "properties": {
            "schemaFormat": {
              "enum": [
                "application/vnd.oai.openapi;version=3.0.0",
                "application/vnd.oai.openapi+json;version=3.0.0",
                "application/vnd.oai.openapi+yaml;version=3.0.0"
              ]
            }
          }
        },
        "then": {
          "properties": {
            "schema": {
              "oneOf": [
                {
                  "$ref": "http://asyncapi.com/definitions/3.0.0/Reference.json"
                },
                {
                  "$ref": "http://asyncapi.com/definitions/3.0.0/openapiSchema_3_0.json"
                }
              ]
            }
          }
        }
      },
      {
        "if": {
          "required": [
            "schemaFormat"
          ],
          "properties": {
            "schemaFormat": {
              "enum": [
                "application/vnd.apache.avro;version=1.9.0",
                "application/vnd.apache.avro+json;version=1.9.0",
                "application/vnd.apache.avro+yaml;version=1.9.0"
              ]
            }
          }
        },
        "then": {
          "properties": {
            "schema": {
              "oneOf": [
                {
                  "$ref": "http://asyncapi.com/definitions/3.0.0/Reference.json"
                },
                {
                  "$ref": "http://asyncapi.com/definitions/3.0.0/avroSchema_v1.json"
                }
              ]
            }
          }
        }
      }
    ]
  },
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "http://asyncapi.com/definitions/3.0.0/multiFormatSchema.json"
}

I tested it locally with Avro and OpenAPI references and it seems to have solved the problem without having to do major refactorings. Mind giving it a test?

@jonaslagoni
Copy link
Member

jonaslagoni commented Mar 27, 2024

Technically we could add the following to anySchema:

  "else": {
    "oneOf": [
      {
        "$ref": "http://asyncapi.com/definitions/3.0.0/Reference.json"
      },
      {
        "$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
      }
    ]
  },

However, because http://asyncapi.com/definitions/3.0.0/schema.json references http://json-schema.org/draft-07/schema that already contains $ref property, it's not really needed.

Scratch that, it classes with #495 (comment), better not to use double meaning for $ref if we can avoid it as described in #495 (comment)

@Pakisan
Copy link
Member Author

Pakisan commented Mar 27, 2024

@jonaslagoni add from my revision(a4c5ef0) all test specifications

@jonaslagoni
Copy link
Member

@Pakisan testing with them now

@jonaslagoni
Copy link
Member

@Pakisan all seem to pass with the changes in #495 (comment)

I can run npm run test with no problem and manually tested one of them.

@Pakisan
Copy link
Member Author

Pakisan commented Mar 27, 2024

@Pakisan all seem to pass with the changes in #495 (comment)

I can run npm run test with no problem and manually tested one of them.

Yep. I found that related MR is fix, not replacement for current. Let me apply your changes and check them

@Pakisan
Copy link
Member Author

Pakisan commented Mar 28, 2024

@jonaslagoni applied your changes

#495 (comment)

@Pakisan Pakisan requested a review from jonaslagoni March 28, 2024 14:07
definitions/3.0.0/anySchema.json Outdated Show resolved Hide resolved
"^x-[\\w\\d\\.\\x2d_]+$": {
"$ref": "http://asyncapi.com/definitions/3.0.0/specificationExtension.json"
}
},
"if": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, feel free to remove this if/then as #507 does ✌️

@@ -70,7 +70,8 @@
},
"required": [
"type"
]
],
"additionalProperties": false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think these changes with additionalProperties is necessary, and on the other side, are you not allowed to have additional properties in Avro? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I remember Avro is allows only defined attributes. For example Record - https://avro.apache.org/docs/1.11.1/specification/#schema-record

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like additional properties is allowed on the different levels of Avro: https://asyncapi.slack.com/archives/C0230UAM6R3/p1712059889567579?thread_ts=1711713980.293369&cid=C0230UAM6R3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed restriction

Comment on lines 1 to 20
{
"if": {
"required": [
"schema"
]
},
"then": {
"$ref": "http://asyncapi.com/definitions/3.0.0/multiFormatSchema.json"
},
"else": {
"$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
},
"description": "An object representing either a schema or a multiFormatSchema based on the existence of the 'schema' property. If the property 'schema' is present, use the multi-format schema. Use the default AsyncAPI Schema otherwise.",
"$id": "http://asyncapi.com/definitions/3.0.0/anySchema.json",
"$schema": "http://json-schema.org/draft-07/schema#",
"$id": "http://asyncapi.com/definitions/3.0.0/anySchema.json"
"type": "object",
"description": "An object representing either a Reference, a Schema or a Multi Format Schema",
"oneOf": [
{
"description": "Because of $ref collision in Reference and AsyncAPI Schema(includes $ref from Json Schema)",
"not": {"required": ["schemaFormat", "schema"]},
"$ref": "http://asyncapi.com/definitions/3.0.0/schema.json"
},
{
"type": "object",
"required": ["schemaFormat", "schema"],
"not": {"required": ["$ref"]},
"minProperties": 2,
"maxProperties": 2,
"$ref": "http://asyncapi.com/definitions/3.0.0/multiFormatSchema.json"
}
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant the entire thing @Pakisan, anySchema does not need any changes to work 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀🌔

Thanks for all the time invested in solving this issue @Pakisan. We all had to do a very focused review on this but finally got sorted out 🙌

Thanks @jonaslagoni as well for your second review.

@smoya
Copy link
Member

smoya commented Apr 3, 2024

/rtm

@smoya smoya removed the do-not-merge label Apr 3, 2024
@asyncapi-bot asyncapi-bot merged commit d58bf5e into asyncapi:master Apr 3, 2024
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.5.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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