Add a deprecation warning to validate_for if schema doesn't exist #484

opethe1st · 2018-11-02T12:49:03Z

This addresses this issue #460
Followed your lead by using the SynchronousTestCase . Any reason or particular features that drive using it instead of just TestCase?
Please let me know if you have any comments. Can't think of any other bright ideas regarding the warnings. First time using the module :) and feels like a pretty small PR so maybe I am missing something.

codecov · 2018-11-02T13:26:21Z

Codecov Report

Merging #484 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   94.88%   94.91%   +0.02%     
==========================================
  Files          19       19              
  Lines        2309     2320      +11     
  Branches      300      301       +1     
==========================================
+ Hits         2191     2202      +11     
  Misses        104      104              
  Partials       14       14

Julian · 2018-11-04T23:21:20Z

Awesome! Well done. Will leave some notes but pretty sure this is exactly what I was after here.

Followed your lead by using the SynchronousTestCase . Any reason or particular features that drive using it instead of just TestCase?

TestCase.assertWarns is the one here, the default unittest.TestCase doesn't define that in Py2.

Julian · 2018-11-04T23:24:03Z

jsonschema/tests/test_validators.py

+            category=DeprecationWarning,
+            message=(
+                "This schema was not found but going to validate with latest draft. "
+                "This will raise a SchemaError in future. "


So, I don't think I love this becoming a SchemaError, I think SchemaError is more specific (than its name), and we use it for just the equivalent of meta schema validation.

Here it's more likely we should just define a CannotLocateMetaschema exception for this specific case.

But, even better than committing right now to a specific thing would just be to say "it will raise an error.", and leave it unspecified.

And if we can, maybe reword the first sentence here to something more like "This schema defines a $schema property for an unregistered or unknown meta-schema. This will raise an error in the future. Register a validator for use with this $schema by calling jsonschema.validates."

Sound reasonable?

What schema are you referring to by this schema in "This schema defines a $schema property for an unregistered or unknown meta-schema ... "

The one that was passed into either validator_for or transitively validates.

fixed the other comments. Good point about leaving it unspecified. And about the schema - you mean whatever is the value of this schema.get(u"$schema", u"") . I believe I get it now, but shouldn't we add the name of the schema we are saying is unregistered to the warning message?

Literally in that line schema :)

The thing that contains the $schema.

This situation we're deprecating basically happens when someone says

validator_for({"$schema", "foobarbaz", ...}) or validate(someinstance, {"$schema", "foobarbaz", ...}), where foobarbaz is supposedly the metaschema for that schema, but didn't tell jsonschema which validator should be used to validate schemas that use that.

Which actually I believe points out a bug in the implementation here.

You're calling schema.get("$schema", "") and warning any time that the resulting $schema isn't registered, but that will warn even in the case where $schema isn't present. Doing that isn't wise, but isn't per se part of what we need to deprecate here. The thing we're deprecating here is the behavior that takes a schema like {"$schema": "unknown"} and silently still uses a validator for that, rather than saying "I don't know what meta schema unknown refers to, so I can't pick a Validator class for it".

Does that make sense?

what should happen in this case that the schema doesn't exist? - use the default draft? nice catch by the way (the bug, when $schema doesn't exist

Yep exactly, this is the case the default argument is designed for -- so, we should likely add a test that this case doesn't warn.

Going to have to do that tomorrow.. it's midnight here and work tomorrow.

Julian · 2018-11-04T23:24:36Z

jsonschema/tests/test_validators.py

+
+            f=validators.validator_for,
+            schema={},
+            default={}


Can you add a trailing comma here? (Not sure why ebb-lint didn't complain it was missing)

Julian · 2018-11-04T23:26:59Z

I'd be remiss also not to lament every time this comes up that the warnings module is crippled and terrible :( (because it hides DeprecationWarnings by default), but that's not your fault certainly, and not a thing that can be easily fixed. Some day someone will write a decent library for deprecations...

…n future

opethe1st · 2018-11-10T16:49:04Z

@Julian updated the pr please take a look

Julian · 2018-11-10T18:13:35Z

jsonschema/tests/test_validators.py

+        self.assertWarns(
+            category=DeprecationWarning,
+            message=(
+                "This schema was not found but going to validate with latest draft. "


This error message still confuses me. What is it you're referring to here by "schema"? It's not a schema that wasn't found here really.

you are correct, its the metaschema that's missing

Julian · 2018-11-18T13:15:40Z

Fixed the failing test on merge.

Really appreciated!

0aefbb3d Merge pull request #491 from jdesrosiers/object-contains-tests 336ef8d2 Merge pull request #452 from LeifRilbe/rilbe/propertyNames-with-pattern 2dfbc79c Simplify the test case names as well. b6d0649d Add tests for contains with objects da687ca5 Enforce a consistent code style for contains tests b163efcf Merge pull request #490 from jdesrosiers/draft-future 7c8cb488 Initialize draft-future with 2020-12 tests 4d65d2df Merge pull request #483 from kylef/kylef/date ee9dcaa7 Merge pull request #485 from marksparkza/contains-with-false-if eaa5bffc Merge pull request #489 from json-schema-org/ether/more-recursiveRef 7c33b533 dynamic $recursiveRef test with cousin $recursiveAnchors 8a3a542b Fix invalid JSON error 8a89f58e Add tests combining remote refs and defs 3aec0d14 Add tests combining relative refs and defs a107d196 fix: $defs -> definitions in draft 6,7 tests 0c223de2 Remove a test for undefined $id behavior 4efec180 Test that "contains" does not fail due to false "if" subschema bf383b4c fix: make identifiers unique across tests 812f1f08 Merge pull request #484 from json-schema-org/ether/schemas-under-unknown-keywords 64f6b850 Test that identifiers are not found inside unrecognized keywords c69a89c6 Stricter date format constraints 93193442 Test cases for propertyNames with pattern - update after PR feedback. 8e4aad95 Test cases for propertyNames with pattern. git-subtree-dir: json git-subtree-split: 0aefbb3d80e0caa22f3782677cf09c61b2205aa7

opethe1st added 7 commits November 1, 2018 01:01

Raise an Error if schema doesn't exist

69f1294

Add test to validate warning is added if schema is not found

c9fc804

.

8e47abc

remove unused warnings import

e628b7a

Remove Raise SchemaError from validator_for doc string

d4ce700

reset gitignore

ae4c2cc

remove line

0554095

opethe1st changed the title ~~Add a warning if schema doesn't exist~~ Add a deprecation warning to validate_for if schema doesn't exist Nov 2, 2018

Julian reviewed Nov 4, 2018

View reviewed changes

opethe1st added 3 commits November 4, 2018 23:48

Add trailing comma

34974d0

Change warning message from raise SchemaError to raise an error i…

cff7cf1

…n future

Update tests and use default when schema not specified

f9cf860

Julian reviewed Nov 14, 2018

View reviewed changes

opethe1st added 3 commits November 14, 2018 22:06

Update the warning mesage

8b7095f

Fix the warning message

7b41e5a

Fix - lines too long

91c4a49

Julian merged commit 91c4a49 into python-jsonschema:master Nov 18, 2018

Uh oh!

Add a deprecation warning to validate_for if schema doesn't exist #484

Add a deprecation warning to validate_for if schema doesn't exist #484

Uh oh!

Conversation

opethe1st commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Julian commented Nov 4, 2018

Uh oh!

Julian Nov 4, 2018 • 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!

opethe1st Nov 4, 2018 • 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!

opethe1st Nov 5, 2018 • 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!

Julian commented Nov 4, 2018

Uh oh!

opethe1st commented Nov 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julian commented Nov 18, 2018

Uh oh!

Uh oh!

opethe1st commented Nov 2, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading

Julian Nov 4, 2018 •

edited

Loading

opethe1st Nov 4, 2018 •

edited

Loading

opethe1st Nov 5, 2018 •

edited

Loading