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.

Sign up for GitHub

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

Jump to bottom

Add option to validate the schema #357

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Add option to validate the schema #357

merged 1 commit into from
Mar 21, 2017

Conversation

erayd
Copy link
Contributor

@erayd erayd commented Feb 22, 2017 •
edited
Loading

Validates the schema against whatever spec is referenced in $schema (see issue #353).

This PR is based on #362; that one needs merging first.

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

Is using $schema alone to determine the spec sufficient, or would a user-override via $checkMode to specifically set draft-03 / draft-04 etc. also be useful?

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

👍 for the idea. I wish I had had this back when I was getting started; would have saved loads of development time and confusion.

I would check $schema to start, then default to draft 4.

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

@shmax Done - it now defaults to draft-04, rather than the previous behavior of throwing a RuntimeException if $schema was undefined.

@erayd erayd force-pushed the schema-validation branch from 15df8fb to 09d2fcd Compare February 22, 2017 20:00
@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

I haven't tried out the code yet, but what does the error output look like? Is it clear that any errors on the schema itself are related to the schema, and not the input data?

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

@shmax It currently brings across all schema validation errors using the standard format, and then adds another error to the stack stating that schema validation has failed. Do you think that this is sufficient, or would some kind of additional disambiguation on the errors that result from schema validation be worth having?

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

I'm not sure, I guess. I'll grab your code tonight and try it out and see how it feels...

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

@shmax Sounds good. I have no particular preference either way, but adding some disambiguation context seems like a good idea, so I've done this (prefixes the error messages with Schema validation: . The old behavior was to simply use the error message verbatim.

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

That might be good enough, but I'm wondering if there should be an even stronger separation. Does the validation proceed if schema errors are encountered, or does it stop dead?

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017 •
edited
Loading

It currently proceeds - I figured that as the current behavior seems to be to proceed despite errors, I should follow suit with the schema validation stuff as well (plus CHECK_MODE_EXCEPTIONS is a thing, so folks who want it to bail at the first hurdle can easily obtain that behavior).

[Ed: Although apparently I forgot to propagate CHECK_MODE_EXCEPTIONS through to the schema validation - that oversight has now been corrected :-).]

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

What kind of stronger separation did you have in mind?

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

I don't know, I don't have the code in front of me and I can't even really remember offhand what normal error output looks like, but I'd like to see what approach AJV takes before I send you off on any wild goose chases.

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

Fair enough :-).

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

Have just fixed a URL validation bug (non-compliance with RFC-3986 relative references) that showed up when I applied schema validation to the draft unit tests. Is that fix (b415918) OK as part of this PR, or should I split it out into a separate PR of its own?

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

If it's unrelated it's probably better to branch it off to a new PR, so we can give each issue our full attention without getting all mixed-up.

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

@shmax What do you consider unrelated? I'm not sure whether it should be considered related or not. It's not part of the feature, but some of the draft unit tests will fail when using schema validation until the bug is fixed.

Splitting them will mean rebasing this on the bugfix PR until such a time as it's merged. Quite happy to do this if you're wanting them separate.

@shmax
Copy link
Collaborator

shmax commented Feb 22, 2017

I have no strong opinion, it just sounded like something that would branch its own discussion. Makes no real difference to me.

@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

I'll split it then; if you think people will want to have a big discussion about it, then it makes sense to keep them separate :-).

@erayd erayd force-pushed the schema-validation branch 2 times, most recently from 8c57c1f to 8c61753 Compare February 22, 2017 23:03
@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

OK, split done - this PR is now based on PR #358.

@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017 •
edited
Loading

Sorry for the delay. I think I'm up to speed, now. validate.getErrors() returns an array of errors; I was hoping for an object with an errors property, but no such luck. I also read up on ajv and analyzed its handling of this kind of thing. The short version is that it doesn't mix schema validation errors with regular errors (which I kind of suspected). I've prepared a jsfiddle that you can experiment with that demonstrates the general approach:

https://jsfiddle.net/hahmvsfj/3/

I don't want to force any ideas down your throat or anything just yet, but I thought it might give you some ideas of your own. Let me know what you think.

@erayd
Copy link
Contributor Author

erayd commented Feb 23, 2017

...would you be keen for me to overhaul the way errors work across the whole library? I don't particularly like the way they are done currently, and would love to tackle the issue, provided people are open to the idea - I don't want to spend ages working on something if nobody wants it.

@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017

Well, I for one am a big fan of your tinkerings and would love to see what you come up with. Have anything in particular in mind right off the bat?

@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017

Oh, and if you think this is going to be a really big project and you want to get some buy-in ahead of time, you might want to start with an issue and see if you can get some thumbs (up) on it.

@erayd
Copy link
Contributor Author

erayd commented Feb 23, 2017 •
edited
Loading

I don't have any really solid ideas yet (although I have a few inklings), but I do know that I dislike the current model and want to change it. I might have a bit of a play, then open a PR with 'these are my ideas' code so others can comment, before I sink large amounts of time into it. Then once we have agreement on an approach, I can polish it up. I think the defaults thing / checkmode refactor benefited greatly from input other than mine, particularly yours, and ended up much better as a result.

[Ed: Beaten to it by @shmax on this one - this bit is yours now ;-)]

@erayd
Copy link
Contributor Author

erayd commented Feb 23, 2017

@mathroc - do you have an opinion on whether or not this schema-validation PR should be a thing? Saw your comment on #353.

@bighappyface
Copy link
Collaborator

Thoughts:

  • Spec validation is valuable
  • Spec validation and constraint validation make sense as separate operations
  • Overhauling errors has happened before and probably didn't go far enough; however, it would be best to handle in a dedicated PR. See Custom error message  #239 for more on this topic.

With another review I will merge #358 so you can rebase this work.

@bighappyface
Copy link
Collaborator

#358 is merged. Please rebase.

@erayd erayd force-pushed the schema-validation branch from 8c61753 to 4dd3cd0 Compare February 23, 2017 19:02
@shmax
Copy link
Collaborator

shmax commented Mar 3, 2017

LGTM very nice

@erayd
Copy link
Contributor Author

erayd commented Mar 3, 2017

@shmax Thanks :-). Time to revisit #362 now we've sat on it for a while? This PR (and one other) still depends on it.

@erayd erayd changed the title [WIP] Add option to validate the schema (addresses issue #353) Add option to validate the schema Mar 3, 2017
@erayd erayd force-pushed the schema-validation branch from a601f7d to a41f62b Compare March 3, 2017 08:52
@erayd
Copy link
Contributor Author

erayd commented Mar 3, 2017

Junk commits squashed, ready to merge after #362 (URI translation & local spec bundling).

@erayd erayd force-pushed the schema-validation branch from a41f62b to 020b367 Compare March 7, 2017 18:48
@erayd
Copy link
Contributor Author

erayd commented Mar 7, 2017

Rebased.

@erayd erayd force-pushed the schema-validation branch from 020b367 to 33d549d Compare March 17, 2017 04:29
@erayd
Copy link
Contributor Author

erayd commented Mar 17, 2017

Rebased.

@erayd erayd force-pushed the schema-validation branch from 33d549d to 75f7fa8 Compare March 21, 2017 18:11
@erayd
Copy link
Contributor Author

erayd commented Mar 21, 2017

Rebased.

bighappyface
bighappyface approved these changes Mar 21, 2017
View reviewed changes
Copy link
Collaborator

@bighappyface bighappyface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@bighappyface bighappyface merged commit e8259a0 into jsonrainbow:6.0.0-dev Mar 21, 2017
erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017
@erayd
Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362)
d462ae6
@erayd erayd mentioned this pull request Mar 21, 2017
Merged
bighappyface pushed a commit that referenced this pull request Mar 21, 2017
@erayd @bighappyface
Remove stale files from #357 (obviated by #362) (#400)
2553ebd
@erayd erayd mentioned this pull request Mar 21, 2017
Merged
erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017
@erayd
Add option to also validate the schema (jsonrainbow#357)
d2ba534
erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017
@erayd
Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362) (…
fbeb472 
…jsonrainbow#400)
@erayd erayd deleted the schema-validation branch March 22, 2017 21:36
bighappyface pushed a commit that referenced this pull request Mar 22, 2017
@erayd @bighappyface
Backports for 5.2.0 (#368)
e3c9bcc 
* Add URI translation for retrieval & add local copies of spec schema

* Add use line for InvalidArgumentException & adjust scope (#372)

Fixes issue #371

* add quiet option (#382)

* add quiet option

* use verbose instead of quiet

* add quiet option

* always output dump-schema

* always output dump-schema-url

* fix typo and ws

* [BUGFIX] Add provided schema under a dummy / internal URI (fixes #376) (#378)

* Add provided schema under a dummy / internal URI (fixes #376)

In order to resolve internal $ref references within a user-provided
schema, SchemaStorage needs to know about the schema. As user-supplied
schemas do not have an associated URI, use a dummy / internal one instead.

* Remove dangling use

* Change URI to class constant on SchemaStorage

* Add option to disable validation of "format" constraint (#383)

* Add more unit tests (#366)

* Add test coverage for coercion API

* Complete test coverage for SchemaStorage

* Add test coverage for ObjectIterator

* Add exception test for JsonPointer

* MabeEnum\Enum appears to use singletons - add testing const

* Don't check this line for coverage

mbstring is on all test platforms, so this line will never be reached.

* Add test for TypeConstraint::validateTypeNameWording()

* Add test for exception on TypeConstraint::validateType()

* PHPunit doesn't like an explanation with its @codeCoverageIgnore...

* Add various tests for UriRetriever

* Add tests for FileGetContents

* Add tests for JsonSchema\Uri\Retrievers\Curl

* Add missing bad-syntax test file

* Restrict ignore to the exception line only

* Fix exception scope

* Allow the schema to be an associative array (#389)

* Allow the schema to be an associative array

Implements #388.

* Use json_decode(json_encode()) for array -> object cast

* Skip exception check on PHP versions < 5.5.0

* Skip test on HHVM, as it's happy to encode resources

* Enable FILTER_FLAG_EMAIL_UNICODE for email format if present (#398)

* Don't throw exceptions until after checking anyOf / oneOf (#394)

Fixes #393

* Fix infinite recursion on some schemas when setting defaults (#359) (#365)

* Don't try to fetch files that don't exist

Throws an exception when the ref can't be resolved to a useful file URI,
rather than waiting for something further down the line to fail after
the fact.

* Refactor defaults code to use LooseTypeCheck where appropriate

* Test for not treating non-containers like arrays

* Update comments

* Rename variable for clarity

* Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS

If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults
if they are marked as required.

* Workaround for $this scope issue on PHP-5.3

* Fix infinite recursion via $ref when applying defaults

* Add missing second test for array case

* Add test for setting a default value for null

* Also fix infinite recursion via $ref for array defaults

* Move nested closure into separate method

* $parentSchema will always be set when $name is, so don't check it

* Handle nulls properly - fixes issue #377

* Add option to also validate the schema (#357)

* Remove stale files from #357 (obviated by #362) (#400)

* Stop #386 sneaking in alongside another PR backport
erayd added a commit to erayd/json-schema that referenced this pull request Mar 22, 2017
@erayd
Add option to also validate the schema (jsonrainbow#357)
b5bc719
erayd added a commit to erayd/json-schema that referenced this pull request Mar 22, 2017
@erayd
Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362) (…
2bd5a39 
…jsonrainbow#400)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shmax shmax shmax left review comments

@mathroc mathroc mathroc left review comments

@bighappyface bighappyface bighappyface approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erayd @shmax @bighappyface @mathroc