Add option to validate the schema #357

erayd · 2017-02-22T14:06:59Z

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 · 2017-02-22T14:15:22Z

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 · 2017-02-22T16:50:00Z

👍 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 · 2017-02-22T19:58:09Z

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

shmax · 2017-02-22T20:31:19Z

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 · 2017-02-22T20:35:31Z

@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 · 2017-02-22T20:56:35Z

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

erayd · 2017-02-22T21:04:23Z

@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 · 2017-02-22T21:08:36Z

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 · 2017-02-22T21:10:41Z

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 · 2017-02-22T21:11:21Z

What kind of stronger separation did you have in mind?

shmax · 2017-02-22T21:18:50Z

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 · 2017-02-22T21:20:30Z

Fair enough :-).

erayd · 2017-02-22T22:32:39Z

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 · 2017-02-22T22:46:40Z

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 · 2017-02-22T22:50:20Z

@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 · 2017-02-22T22:56:32Z

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

erayd · 2017-02-22T22:59:37Z

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 · 2017-02-22T23:06:11Z

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

shmax · 2017-02-23T05:30:50Z

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 · 2017-02-23T05:42:21Z

...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 · 2017-02-23T05:46:55Z

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 · 2017-02-23T05:48:31Z

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 · 2017-02-23T05:56:29Z

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 · 2017-02-23T18:42:37Z

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

bighappyface · 2017-02-23T18:52:07Z

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 · 2017-02-23T19:01:08Z

#358 is merged. Please rebase.

shmax · 2017-03-03T04:01:15Z

LGTM very nice

erayd · 2017-03-03T07:06:30Z

@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 · 2017-03-03T08:54:15Z

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

erayd · 2017-03-07T18:49:14Z

Rebased.

erayd · 2017-03-17T04:29:19Z

Rebased.

erayd · 2017-03-21T18:11:14Z

Rebased.

bighappyface

+1

…jsonrainbow#400)

* 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

…jsonrainbow#400)

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

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

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

erayd changed the title ~~[WIP] Add option to validate the schema (addresses issue #353)~~ Add option to validate the schema Mar 3, 2017

erayd force-pushed the schema-validation branch from a601f7d to a41f62b Compare March 3, 2017 08:52

bighappyface added the Needs rebase on target branch label Mar 7, 2017

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

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

bighappyface added Needs rebase on target branch and removed Needs rebase on target branch labels Mar 17, 2017

Add option to also validate the schema

75f7fa8

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

bighappyface removed the Needs rebase on target branch label Mar 21, 2017

bighappyface approved these changes Mar 21, 2017

View reviewed changes

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

Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362)

d462ae6

erayd mentioned this pull request Mar 21, 2017

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

Merged

bighappyface pushed a commit that referenced this pull request Mar 21, 2017

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

2553ebd

erayd mentioned this pull request Mar 21, 2017

Backports for 5.2.0 #368

Merged

erayd added a commit to erayd/json-schema that referenced this pull request Mar 21, 2017

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

Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362) (…

fbeb472

…jsonrainbow#400)

erayd deleted the schema-validation branch March 22, 2017 21:36

erayd added a commit to erayd/json-schema that referenced this pull request Mar 22, 2017

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

Remove stale files from jsonrainbow#357 (obviated by jsonrainbow#362) (…

2bd5a39

…jsonrainbow#400)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to validate the schema #357

Add option to validate the schema #357

erayd commented Feb 22, 2017 •

edited

Loading

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017 •

edited

Loading

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 23, 2017 •

edited

Loading

erayd commented Feb 23, 2017

shmax commented Feb 23, 2017

shmax commented Feb 23, 2017

erayd commented Feb 23, 2017 •

edited

Loading

erayd commented Feb 23, 2017

bighappyface commented Feb 23, 2017

bighappyface commented Feb 23, 2017

shmax commented Mar 3, 2017

erayd commented Mar 3, 2017

erayd commented Mar 3, 2017

erayd commented Mar 7, 2017

erayd commented Mar 17, 2017

erayd commented Mar 21, 2017

bighappyface left a comment

Add option to validate the schema #357

Add option to validate the schema #357

Conversation

erayd commented Feb 22, 2017 • edited Loading

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017 • edited Loading

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 22, 2017

erayd commented Feb 22, 2017

erayd commented Feb 22, 2017

shmax commented Feb 23, 2017 • edited Loading

erayd commented Feb 23, 2017

shmax commented Feb 23, 2017

shmax commented Feb 23, 2017

erayd commented Feb 23, 2017 • edited Loading

erayd commented Feb 23, 2017

bighappyface commented Feb 23, 2017

bighappyface commented Feb 23, 2017

shmax commented Mar 3, 2017

erayd commented Mar 3, 2017

erayd commented Mar 3, 2017

erayd commented Mar 7, 2017

erayd commented Mar 17, 2017

erayd commented Mar 21, 2017

bighappyface left a comment

Choose a reason for hiding this comment

erayd commented Feb 22, 2017 •

edited

Loading

erayd commented Feb 22, 2017 •

edited

Loading

shmax commented Feb 23, 2017 •

edited

Loading

erayd commented Feb 23, 2017 •

edited

Loading