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

feat!: allow multi format schemas #370

Merged
merged 17 commits into from
Jul 13, 2023

Conversation

smoya
Copy link
Member

@smoya smoya commented Mar 31, 2023

Description

This PR contains all the required changes for the following new Spec v3.0.0 feature asyncapi/spec#622.
A WIP PR containing changes on the spec has been created by @GreenRover.

The changes made in this PR apply to:

  • Message Payload (including examples) both in the root document and in components.
  • Schemas in components

Related issue(s)
asyncapi/spec#622

@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@GreenRover
Copy link
Collaborator

Thx smoja, i totaly like it!

@GreenRover
Copy link
Collaborator

I found a point. The MessagePayloadObject can not be fined below components. Can you add this.

@sonarcloud
Copy link

sonarcloud bot commented Apr 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jonaslagoni
Copy link
Member

@smoya I am assuming you continue this when you get back the 19 th, otherwise let me know so I can help push this over the finish line 🙂

@smoya
Copy link
Member Author

smoya commented Jun 22, 2023

@smoya I am assuming you continue this when you get back the 19 th, otherwise let me know so I can help push this over the finish line 🙂

You are right assuming this. I'm gonna work on this.

@smoya
Copy link
Member Author

smoya commented Jun 22, 2023

I found a point. The MessagePayloadObject can not be fined below components. Can you add this.

@GreenRover Sorry for the delay, but I can't find where we discussed about having those under components. We already have schemas so what's the need for adding this new wrapper under components as well?

cc @derberg @jonaslagoni

}
],
"$schema": "http://json-schema.org/draft-07/schema#",
"$id": "http://asyncapi.com/definitions/3.0.0/messagePayloadObject.json"
Copy link
Member

Choose a reason for hiding this comment

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

I would call it something similar to MultiFormatSchemaObject that is unrelated to payload.

@jonaslagoni
Copy link
Member

@GreenRover Sorry for the delay, but I can't find where we discussed about having those under components. We already have schemas so what's the need for adding this new wrapper under components as well?

components.schemas needs to change as well yea 🙂

Comment on lines 116 to 118
"payload": {
"$ref": "http://asyncapi.com/definitions/3.0.0/messagePayloadObject.json"
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be a union of the schema+format, just schema, or reference.

@smoya smoya marked this pull request as ready for review June 26, 2023 08:19
@smoya smoya requested a review from jonaslagoni June 26, 2023 08:19
@smoya
Copy link
Member Author

smoya commented Jun 26, 2023

@jonaslagoni I made the changes you suggested. Additionally, I removed the object suffix as I don't find it necessary anymore.
The multiFormatSchemas are added into the schemas.json now.

BTW, I tested the compiled schema but I expected the following to give an error because the schema is not a valid avro but it doesn't. I will check the reason why.

https://www.jsonschemavalidator.net/s/aSOFbwxD

@GreenRover
Copy link
Collaborator

@GreenRover Sorry for the delay, but I can't find where we discussed about having those under components. We already have schemas so what's the need for adding this new wrapper under components as well?

components.schemas needs to change as well yea 🙂

There is no direct need. May be reusability.
But we never discussed this fact.

"required": [
"schemaFormat"
],
"properties": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I miss a section that will allow protobuff, xsd or other string based schemas?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we need to add official support for protobuf, indeed this should be done inside this document by adding a new "if" section. But afaik Otherwise, it is still supported since the schema for the object itself allows it since the schema has no JSON Schema definition:

"properties": {
      "schemaFormat": {
        "type": "string"
      },
      "schema": {}
    },

Meaning the following should be valid:

"components": {
    "schemas": {
      "myProtobufSchema": {
        "schemaFormat": "application/vnd.google.protobuf;version=3",
        "schema": "message Point {\n    required int32 x = 1;\n    required int32 y = 2;\n    optional string label = 3;\n}\n\nmessage Line {\n    required Point start = 1;\n    required Point end = 2;\n    optional string label = 3;\n}\n"
      }
    }
  }

Copy link
Member

@jonaslagoni jonaslagoni Jun 28, 2023

Choose a reason for hiding this comment

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

the "schema": {} is the same as "schema": true, so everything is allowed. Also stringed protobuf schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

Testing, I noticed there is a bug somewhere in my current implementation where it is allowed to set a string schema even when setting the schemaFormat to be an AsyncAPI schema.

The following example demonstrates it; where the testinline schema appears as invalid (as I expect) but the test schema is valid (unexpected).

{
  "asyncapi": "3.0.0",
  "info": {
    "title": "Example",
    "version": "0.1.0"
  },
  "components": {
    "schemas": {
      "test": {
        "schemaFormat": "application/vnd.aai.asyncapi+json;version=3.0.0",
        "schema": "message Point {\n    required int32 x = 1;\n    required int32 y = 2;\n    optional string label = 3;\n}\n\nmessage Line {\n    required Point start = 1;\n    required Point end = 2;\n    optional string label = 3;\n}\n"
      },
      "testInline": "message Point {\n    required int32 x = 1;\n    required int32 y = 2;\n    optional string label = 3;\n}\n\nmessage Line {\n    required Point start = 1;\n    required Point end = 2;\n    optional string label = 3;\n}\n"
    }
  }
}

Open in JSON Schema validator

I don't get why since both are validating against schema.json. I will need to check what's the issue about.

Copy link
Member

Choose a reason for hiding this comment

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

@smoya remember to change the components to be anySchema.json not schema.json 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

oh-no-facepalm-gif

definitions/3.0.0/anySchema.json Outdated Show resolved Hide resolved
"required": [
"schemaFormat"
],
"properties": {
Copy link
Member

Choose a reason for hiding this comment

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

@smoya remember to change the components to be anySchema.json not schema.json 😄

definitions/3.0.0/schemas.json Outdated Show resolved Hide resolved
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.

One comment

definitions/3.0.0/components.json Outdated Show resolved Hide resolved
@smoya
Copy link
Member Author

smoya commented Jun 29, 2023

@jonaslagoni I had to add this change in order to allow values such as bool to be valid schemas when matched with multiFormatSchema.json. Otherwise, the following will be invalid:

"components": {
    "schemas": {
      "test": true,
      "test2": {
        "schema": true
      },
      "test3": {
        "schemaFormat": "application/vnd.aai.asyncapi;version=3.0.0",
        "schema": true
      }
    }
  }

Do you believe there is a better/smarter way of doing that?

@smoya smoya requested a review from jonaslagoni June 30, 2023 09:20
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 👌

@@ -0,0 +1,21 @@
{
"allOf": [
Copy link
Member

Choose a reason for hiding this comment

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

Is allOf needed here? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. Removed!

@smoya smoya requested a review from fmvilas July 13, 2023 14:27
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹

@sonarcloud
Copy link

sonarcloud bot commented Jul 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@smoya
Copy link
Member Author

smoya commented Jul 13, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit 6367e1d into asyncapi:next-major-spec Jul 13, 2023
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.0.0-next-major-spec.2 🎉

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
Development

Successfully merging this pull request may close these issues.

5 participants