fix: clarify support for Boolean JSON Schemas #550
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
|
@magicmatatjahu is this really necessary? When we refer to the Schema object, to me that implicitly say that boolean is allowed? 🤔 |
|
@jonaslagoni Do you mean union type |
magicmatatjahu
added
the
✏️ Editorial
spec/asyncapi.md
Outdated
| This object is a superset of the [JSON Schema Specification Draft 07](http://json-schema.org/). This means that Schema Object can be a boolean value. `True` means that the type can be anything, while `false` means that the given schema cannot be defined. | ||
|
|
There was a problem hiding this comment.
This is somewhat incorrect depending on how you look at it. If we are talking purely JSON Schema, then true means everything will be accepted, false means everything will be rejected. Not sure how to best describe that in this sentence though 🤔
There was a problem hiding this comment.
then true means everything will be accepted - True means that the type can be anything.
false means everything will be rejected - false means that the given schema cannot be defined.
I think that both phrases seem to have the same meaning.
The biggest problem to describe is for some parts of spec, because when we will allow making schema as boolean value (as described in spec after merging this PR), some parts, like headersin message, MUST be ofobject` type, not boolean. People may not understand exactly when they will be able to use scheme as boolean. The schema for the Channel Parameters is another example where the boolean schema doesn't make much sense, but if it's a Schema Object then they have possibility to do that...
There was a problem hiding this comment.
then true means everything will be accepted-Truemeans that the type can be anything.
false means everything will be rejected-falsemeans that the given schema cannot be defined.
Hehe, I see your point, but it is two different perspectives, the one I proposed is from a validation point of view, the other is from a definition point of view. - and as JSON Schema is validation, that is why I proposed to change the wording 🙂
The bigger problem to describe is for some parts of spec, ...
From my perspective, it is not a problem, as soon as you say it can be a Schema Object that entails that it can be a boolean. Why would you want to restrict this? 🤔 IMO, whether it is used or not, that's up to the user, not us 🙂
There was a problem hiding this comment.
I am starting to wonder if it is intentional that the schema object is not allowed to be a boolean or if that was just how it came to be? 🤔
There was a problem hiding this comment.
In spec we use usually definition point of view, in some places also validation POV, but only by purpose, we define that some parts must be in given shape or must be unique. Now I understand why you want describe true/false in this way, but as I read Schema Object section in spec I see description from definition POV, not validation 😅
There was a problem hiding this comment.
In spec we use usually definition point of view, in some places also validation POV, but only by purpose, we define that some parts must be in given shape or must be unique. Now I understand why you want describe
true/falsein this way, but as I readSchema Objectsection in spec I see description from definition POV, not validation 😅
Yeea, and I agree we should be consistent.
IMO it needs to be re-written to use the correct perspective, but it might become unclear for other people then... 🤷
There was a problem hiding this comment.
IMO it needs to be re-written to use the correct perspective, but it might become unclear for other people then...
Ok, let's wait for other comments :)
There was a problem hiding this comment.
IMO it needs to be re-written to use the correct perspective, but it might become unclear for other people then
I agree we should advocate for consistency since having two perspectives are adding confusion when reading.
In spec we use usually definition point of view, in some places also validation POV, but only by purpose, we define that some parts must be in given shape or must be unique. Now I understand why you want describe true/false in this way, but as I read Schema Object section in spec I see description from definition POV, not validation 😅
IMHO, definition/describing POV looks more easy to understand for the first reader. Specs are made for describing, tooling around does other things such as validation.
The point is, shall we rewrite the doc entirely? This is something we can discuss in another issue.
What shall we do with this current PR? I see a couple of options:
- To keep using the same POV schemaObject section had, which seems to ve a validation POV.
- To rewrite the schemaObject section entirely replacing wording to be adapted to a describe/definition POV.
There was a problem hiding this comment.
Specs are made for describing, tooling around does other things such as validation.
Then what if you describe what your data (payload) should validate against? 😆
There was a problem hiding this comment.
I used the description from https://github.com/OAI/OpenAPI-Specification/pull/2609/files PR:
The empty schema (which allows any instance to validate) MAY be represented by the
booleanvaluetrueand a schema which allows no instance to validate MAY be represented by thebooleanvaluefalse.
It's from validation pov, but for me it's a better than previous one.
|
From our co-incidentally raised PR over at OAS https://github.com/OAI/OpenAPI-Specification/pull/2609/files |
|
As I can see AsyncAPI not only copied the architecture (of spec), but also full sentences from the specification 😅 @jonaslagoni Do you agree that
is much better that current description in PR? Will you accept it? As you see, there we have desc from validation point of view. @MikeRalphson will you have any problem so that we can reuse it in the AsyncAPI specification, or do you want us to pay you? 😄 |
|
I think we share a common license, as well as having common interests! So feedback on our PR can be two-way. |
fmvilas
dismissed stale reviews from smoya and themself
The base branch was changed.
|
@fmvilas I see that you changed two times the target branch and we end to master. I thought that we will merge it to master and then rebase the |
|
Yes, I got confused at first. If it's just an editorial change and it doesn't change the behavior, it should point to master and should trigger a patch release when merged. |
|
@magicmatatjahu @fmvilas
yes, but in the future after 2.1, otherwise merge of this fix will trigger 2.1 now it should go to release candidate branch with |
why? because it already happened twice 😓 |
|
Another solution can be go with |
magicmatatjahu
changed the title
no, unless you provide in commit details
good luck 😄 |
|
@fmvilas I'm changing the target to prerelease due to problems described by Łukasz :) I need only info from you that you accept our decision, or if you have problems, please write them. |
|
That's fine. I don't think we too many options right now 😄 |
magicmatatjahu
force-pushed
the
true-false-json-schemas
branch
from
18a02bc to
9e00dfa
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Merged in #561 Thanks guys for review! |
title: "Mention about true/false JSON Schemas in Schema Object"
As Schema Object in spec is defined as superset of the JSON Schema Specification Draft 07, it would be nice to specify that the Schema Object itself can be boolean value. I know it results from the definition of draft 07, so I'd like to hear your opinion about the changes.
Description for
true/falseschemas I took from https://github.com/OAI/OpenAPI-Specification/pull/2609/files Thanks to @MikeRalphsonRelated issue(s):
asyncapi/parser-js#232
Fixes #554
Related PR(s):
asyncapi/spec-json-schemas#63 - fix bug with true/false schemas in JSON Schema of spec
asyncapi/parser-js#311 - handle true/false JSON Schema in parser
asyncapi/asyncapi-react#367 - handle Boolean JSON Schema in the
react-component