fix: support true/false JSON Schemas by magicmatatjahu · Pull Request #311 · asyncapi/parser-js

magicmatatjahu · 2021-06-02T12:52:48Z

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 and of course in parser. More info #232

Related issue(s)
Fixes #232

magicmatatjahu · 2021-06-02T12:56:34Z

+  constructor (json) {
+    const isBoolSchema = typeof json === 'boolean';
+    super(isBoolSchema ? {} : json);
+    this.isBoolSchema = isBoolSchema;
+  }


I have the biggest problem here, how to handle it, because we don't support true/false schema from the beginning. This solution both adds boolean schema support, and is also backward compatible. Another solution is to return the schema as boolean, but then we must adjust a lot of functions in another models, which returns array of schemas or single schema. Any thoughts?

I think it makes sense to keep all schemas as Schema object types. Also, if you return pure boolean types here, checks would need always to be performed as a strict comparison (=== / !===), which sounds pretty error-prone. schema.isBoolSchema sounds like a more readable version and also less error-prone since we can make a statement on "all schemas are represented as Schema objects.

Yes, you're right :)

Hmm, yea it is a tricky problem.

In theory, I would never expect an instance of the Schema to be returned to me if the schema was a boolean. Especially since in all other aspects, the parser structure follows the exact type dictated by the specification.

However, I can see the benefits of having this isBoolSchema function 🧐 Not sure what the appropriate approach is here.

About backward compatible I thought wrong, We didn't support the true/false schemas in parser, because if someone wrote them then had the errors from ajv, because we had broken JSON Schema for spec. I can change the current behaviour and return the boolean in places where we operate on Schema model, but... handling boolean schemas as class is probably less error-prone in templates as @smoya wrote, because then we haven't to check in every place that schema is bool or not. I can only imagine the pain.

Also in current changes I passed for true schema the {} model body and for false the { not: {} } and by this we have two ways for handling Boolean Schemas.

Yep true, I can see it as a benefit as well in terms of the boolean functions I guess 👍

magicmatatjahu · 2021-06-02T12:57:14Z

-      {
-        title: '/channels/mychannel/publish/message/payload/additionalProperties should be object',
-        location: {
-          jsonPointer: '/channels/mychannel/publish/message/payload/additionalProperties',
-          startLine: 13,
-          startColumn: 38,
-          startOffset: offset(252, 13),
-          endLine: 15,
-          endColumn: 15,
-          endOffset: offset(297, 15)
-        }
-      },


When we will merge it asyncapi/spec-json-schemas#63 then this error won't be throw.

jonaslagoni

Added a few things to discuss.

jonaslagoni

Much cleaner 👍 Still got a small comment.

jonaslagoni · 2021-06-16T15:11:18Z

+    super(json === false ? {} : json);
+    this._json = json;


Would it not be possible to change the base check to check for undefined instead of booleans as well?🤔 Or would that create problems? 🤔

Setting _json twice seems like the wrong approach 😅

The base ctor has:

if (!json) throw new ParserError(`Invalid JSON to instantiate the ${this.constructor.name} object.`); this._json = json;

I don't want to change it, because handling false schemas in base class isn't a good idea. At the moment it's only needed for Schema so I pass empty object as workaround

Setting _json twice seems like the wrong approach 😅

Yep, but it's still "fine" if we make this assignment in child ctor, not in other place :)

Also checking undefined is not good idea - JSON Schema hasn't undefined type/value

To my understanding, the base class constructor check, in theory, should only make sure that the received value is not undefined/null 🤔

Therefore you could change that referenced line from a lazy check to strict:

if (json === undefined || json === null) throw new ParserError(`Invalid JSON to instantiate the ${this.constructor.name} object.`);

That way you can remove this._json = json; in this constructor.

Can't see the cases where the argument would be 0, NaN, false, ""

Perhaps I'm sitting in work too long today 😅 Thanks! Changed.

haha 😆 That or I have, lets see if others find a problem with this approach 😆

but even if you put "" or 0 what it's a value in spec? Can we pass something like this in spec itself? Only false has some "value", but only related to Schema object.

Not that I know of no, hence my suggestion.

The only dilemma I can see could be if the JSON Schema for the specification allowed such values where objects are expected, then it would no longer throw a parser error here. I just don't see the use-case 🤷

I also 😅

jonaslagoni

Is there a reason why you have updated dist/bundle.js?

Other than that it looks good from my perspective 🙂

magicmatatjahu · 2021-06-16T15:39:25Z

@jonaslagoni Probably when I run the integration tests I didn't check the changes for dist... but it has change on every release :) Could you approve again?

jonaslagoni

It still detects bundle changes 🙂

jonaslagoni

Last change I think 😆

sonarqubecloud · 2021-06-16T15:57:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

jonaslagoni

I think we can accept that code smell as we already have a similar one and I dont see any other way of shorting that check 👍

magicmatatjahu · 2021-06-16T16:02:03Z

@jonaslagoni Thanks!

asyncapi-bot · 2021-06-16T16:05:27Z

🎉 This PR is included in version 1.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: support true/false JSON Schemas

a8bc02e

magicmatatjahu added the bug Something isn't working label Jun 2, 2021

magicmatatjahu commented Jun 2, 2021

View reviewed changes

magicmatatjahu added 2 commits June 2, 2021 18:50

fix sonarcloud errors

91ae158

add booleanValue function

ae5575f

magicmatatjahu mentioned this pull request Jun 2, 2021

fix: clarify support for Boolean JSON Schemas asyncapi/spec#550

Closed

This was referenced Jun 11, 2021

Clarify support for Boolean JSON Schemas asyncapi/spec#554

Closed

fix: support true/false JSON schemas asyncapi/asyncapi-react#367

Merged

fix: clarify support for Boolean JSON Schemas asyncapi/spec#561

Merged

magicmatatjahu added 2 commits June 15, 2021 16:31

update specs dep to latest

dec8bc4

Merge branch 'master' into true-false-schemas

9ac2acf

magicmatatjahu mentioned this pull request Jun 15, 2021

fix: update @asyncapi/specs to 2.7.8 version #318

Closed

magicmatatjahu requested a review from jonaslagoni June 15, 2021 14:45

changes

9342454

magicmatatjahu requested review from derberg and smoya June 16, 2021 10:29

jonaslagoni suggested changes Jun 16, 2021

View reviewed changes

Comment thread lib/models/schema.js Outdated

Comment thread test/asyncapiSchemaFormatParser_test.js

magicmatatjahu requested a review from jonaslagoni June 16, 2021 15:07

jonaslagoni suggested changes Jun 16, 2021

View reviewed changes

magicmatatjahu requested a review from jonaslagoni June 16, 2021 15:20

jonaslagoni previously approved these changes Jun 16, 2021

View reviewed changes

magicmatatjahu dismissed jonaslagoni’s stale review via 5e6b21f June 16, 2021 15:37

jonaslagoni suggested changes Jun 16, 2021

View reviewed changes

Comment thread types.d.ts

changes

5ce70ae

magicmatatjahu force-pushed the true-false-schemas branch from 5e6b21f to 5ce70ae Compare June 16, 2021 15:44

magicmatatjahu requested a review from jonaslagoni June 16, 2021 15:51

jonaslagoni suggested changes Jun 16, 2021

View reviewed changes

Comment thread test/asyncapiSchemaFormatParser_test.js Outdated

after review

8509ff4

magicmatatjahu requested a review from jonaslagoni June 16, 2021 15:56

jonaslagoni approved these changes Jun 16, 2021

View reviewed changes

magicmatatjahu merged commit 5cc631b into asyncapi:master Jun 16, 2021

magicmatatjahu deleted the true-false-schemas branch June 16, 2021 16:01

asyncapi-bot added the released label Jun 16, 2021

github-actions Bot approved these changes Jun 16, 2021

View reviewed changes

Uh oh!

Uh oh!

Conversation

magicmatatjahu commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaslagoni Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicmatatjahu Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

magicmatatjahu commented Jun 16, 2021

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 16, 2021

Uh oh!

jonaslagoni left a comment

Choose a reason for hiding this comment

Uh oh!

magicmatatjahu commented Jun 16, 2021

Uh oh!

asyncapi-bot commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

magicmatatjahu commented Jun 2, 2021 •

edited

Loading

jonaslagoni Jun 16, 2021 •

edited

Loading

magicmatatjahu Jun 16, 2021 •

edited

Loading