-
-
Notifications
You must be signed in to change notification settings - Fork 53
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: support for Boolean JSON Schema #63
fix: support for Boolean JSON Schema #63
Conversation
There was a problem hiding this 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 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.
@@ -362,7 +362,6 @@ | |||
"$ref": "http://json-schema.org/draft-07/schema#" | |||
}, | |||
{ | |||
"type": "object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd not remove this line but instead, I'd do the following:
"schema": { // Current line 359
"oneOf": [
{ "type": "boolean" },
{ "allOf": [ // Current line 360
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's one the solution, but when we will remove the type
from second schema, then we infer the type from http://json-schema.org/draft-07/schema
and it is:
"type": ["object", "boolean"],
https://github.com/json-schema-org/json-schema-spec/blob/draft-07/schema.json#L39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that oneOf
can only complicate (already complex) our JSON Schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Forgot that type
can take an array. Go for it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fore sure not removing the type: object
line because that would mean we accept anything and that's not true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the "type": ["object", "boolean"]
is inferred from the http://json-schema.org/draft-07/schema
allOf
combined schema means that for every sub schema input data must be valid, so if I removed the type
from the second schema then I have allowed types for first schema object
and boolean
and for second I have allowed of course everything, but then, as I wrote, the http://json-schema.org/draft-07/schema schema only allows the [object, boolean], so at the end if someone put as input the number type it will be invalid:
- first schema - invalid
- second schema - valid
If someone put boolean or object it will be always valid :)
You can test it with this simple schema:
{
allOf: [
{
type: ['object', 'boolean']
},
{}
],
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for you I will write in the second schema the object, boolean
type 😘
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh sorry, I didn't notice. Nevermind, it's fine as it is 👍
Kudos, SonarCloud Quality Gate passed! |
@fmvilas Thanks! |
🎉 This PR is included in version 2.7.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
As in title. As we support JSON Schema draft-07 as schema, then we should also support
true/false
schemas in JSON Schema of our spec. More info asyncapi/parser-js#232I removed
"type": "object",
from schema, because we should infertype
field from"http://json-schema.org/draft-07/schema#"
. Currently we override it.Related issue(s)
See more asyncapi/parser-js#232
Blocked by asyncapi/spec#550