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

Change JSONSchema validator from is-my-json-valid to ajv #397

Closed
wants to merge 2 commits into from

Conversation

rybaczewa
Copy link
Contributor

@rybaczewa rybaczewa commented Dec 8, 2017

This PR contains:

Potentially breaking change, few new tests, package change

Describe the problem you have without this PR

At the moment, is-my-json-valid is not validating schemas with definitions properly. It causes invalid schemas to pass validation where it shouldn't. Ajv is faster and better maintained, much better coming into the future of the project.

Extra parsing function added for backward compatibility of errors.

@pubkey
Copy link
Owner

pubkey commented Dec 11, 2017

Hi @rybaczewa thank you for the great PR.
The code looks good. But if have some problems:

  1. The build-size is much bigger now:
    (run npm run disc to verify)
before:

- disc: 408.9kB
- gzip: 156.8kB


after:

- disc: 489.6kB
- gzip: 174.3kB
  1. It looks like this is a breaking change because required: true seems to not works anymore. Is this correct?

I think we should not swap out the default validator by now. It will be better to create a new plugin ajv-validate so users can cherry-pick it if they want. Then we can swap out the default validator afterwards if wanted.

@rybaczewa
Copy link
Contributor Author

If it comes to required keyword, that's another place where is-my-json-valid wasn't respecting JSONSchema specification. If specified, required keyword must be an array of unique string values. Can't be boolean.
http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.5.3

Build size increase sounds about right looking at package sizes:

Generally speaking, the more I looked at old validator package, the less I liked it. Separate validator seems OK for now, but it might more beneficial for the future to migrate to package that does it's job. I am by no means pushing exactly ajv, just anything that works (and it's looking like the best choice for now).

@pubkey
Copy link
Owner

pubkey commented Dec 13, 2017

You are right. I currently also regret the switch to is-my-json-valid and we should move to ajv in the future.
I will now create an ajv-validate-plugin from your code. And we I do the next major-release, I will move to ajv as breaking change.

pubkey added a commit that referenced this pull request Dec 13, 2017
@pubkey
Copy link
Owner

pubkey commented Dec 13, 2017

Closed in favor of 20063b960ba87e9a7a561f45d56271aafef6a42e

@pubkey pubkey closed this Dec 13, 2017
@rybaczewa rybaczewa deleted the nested-schema branch June 5, 2018 14:49
@pubkey
Copy link
Owner

pubkey commented Jul 7, 2018

Hi @rybaczewa
I am working on RxDB v8.0.0 atm.
As described in the before-next-major-todo-list, it was planned to switch to ajv by default.
I switched to ajv and checked the result, but now I am not sure if switching to ajv is a good thing because of the following reasons:

  • Build size increased by 80kB, minified+gziped (run npm run disc to validate)
  • Spawn Database Time increased by 9% (run npm run test:performance to validate)
  • Insertion Time of documents increased by 6%

Yes, ajv is more compliant with the json-schema-standard, but I do currently not think that this is worth the disadvantages.

Any comment or argument would be welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants