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

Handling schema true and false #232

Closed
jonaslagoni opened this issue Jan 18, 2021 · 9 comments · Fixed by #311
Closed

Handling schema true and false #232

jonaslagoni opened this issue Jan 18, 2021 · 9 comments · Fixed by #311
Labels
bug Something isn't working

Comments

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 18, 2021

Describe the bug

You can define a schema as true or false to accept or reject all such as

...
"properties":
   "age": true,
...

Now the problem is that our anonymous uid assignment tries to assign a uid to such schemas when its not possible.

This problem triggered the tests to fail for asyncapi/raml-dt-schema-parser#25.

Expected behavior

There are a couple of ways we can handle this.

  1. We dont assign schemas which are true or false uids.
  2. We convert true or false to its sugarcoated values {} and {"not": {}}
@jonaslagoni jonaslagoni added the bug Something isn't working label Jan 18, 2021
@jonaslagoni
Copy link
Member Author

After a lengthy discussion on Slack with the json-schema community my perspective of the true and false schema's has changed. I saw it more as a type then the actual representation of absence and presence of types which is why it is defined as is.

@jonaslagoni
Copy link
Member Author

@magicmatatjahu @derberg thoughts? 🤔

@derberg
Copy link
Member

derberg commented Jan 18, 2021

@jonaslagoni I saw your discussion in slack. If this doesn't represent the data but rather indicates if data can exist, then I guess we should not assign id to it. On the other hand, you would generate a data type for it right, even if it is just false/true?

@jonaslagoni
Copy link
Member Author

On the other hand, you would generate a data type for it right, even if it is just false/true?

Yes, with for example in typescript I would want to generate any type for that property.

I think we shouldn't assign uid to those schemas and just ignore them as you said 😄

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label Mar 21, 2021
@derberg derberg removed the stale label Mar 22, 2021
@magicmatatjahu
Copy link
Member

I also ran into this problem 😅 However ids are not the biggest problem at the moment, but the fact that the Base model is not adapted to use the true/false schema (see code - we cannot assign to schema false value) and also that we use draft-04 in the parser and the specification itself states that it supports draft 07 - true/false schemas are supported from draft-07.

If you write true/false schemas you should have error like:

[
  "/components/schemas/lightMeasuredPayload/properties/lol should be object at line 156, column 9"
]

You can check it in https://playground.asyncapi.io/

@derberg
Copy link
Member

derberg commented Mar 29, 2021

but then just Base model needs to be fixed right? the problem that is there is not intentional imho. It is a typical check you do there, to throw an error if an argument is not passed to the function. We just need a better check, if json is undefined, or am I missing something?

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Mar 29, 2021

@derberg You're right, it's check not only for Schema but only for other models... so we should override constructor for Schema model like:

class Schema extends Base {
  constructor (json) {
    if (!json && json !== false) throw new ParserError(`Invalid JSON to instantiate the ${this.constructor.name} object.`);
    this._json = json;
  }
  // rest of class
}

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants