Stop subclassing JSONSchema validator

[WilliamJamieson]

Potentential fix for issue #1172

[WilliamJamieson]

@Julian this is my initial attempt to stop subclassing Validator for use in ASDF. I found your suggestion in python-jsonschema/jsonschema#981 (comment) a bit hard to implement because it fails to have several methods we use from the Validator class, rather than re-implement (copy from jsonschema) all the missing parts, I am simply patching the Validator class.

Please feel free to leave comments, alternate suggestions, or a review. I would welcome your thoughts.

[WilliamJamieson]

I believe the failing tests are due to a bug in jsonschema, see python-jsonschema/jsonschema#994 for details.

[Julian]

Hi! Monkeypatching an object you don't "own" (i.e. a validator class) is definitely even more fragile than subclassing from it, so this definitely could be improved somehow. I'll have to spend some time with it to see what to recommend though, unless someone else sees it first.

[WilliamJamieson]

@Julian I completely agree that Monkeypatching is less than ideal. Essentially, I am just "replicating" the changes the previous subclassing would have accomplished. This "avoids" the warning that has been added as that breaks the required testing in downstream packages that rely on asdf (they turn all warnings into raised errors); moreover, the warning does mention that subclassing will be removed at a later date.

The alternate method to this is to start with what you suggested in python-jsonschema/jsonschema#981 (comment); however, as far as I can tell this then requires me to then replicate much of the jsonschema Validator class's interface through copying the necessary parts from the jsonschema source. This also seems fragile and less than ideal because it requires me to watch for all changes to the jsonschema Validator and adjust asdf accordingly.

As far as I can tell there isn't an interface provided by jsonschema which is rich enough to enable asdf to add its additional logic on top of the jsonschema Validator's iter_errors method. As far as I can tell, asdf truly needs to modify the iter_errors method in order to handle validation with respect to yaml tags (which are not part of the json specification and so are outside the jsonschema specification), which are used as a core part of asdf's functionality.

[WilliamJamieson]

Tagging @eslavich to weigh in, as they understand how asdf leverages jsonschema better than I do.

[eslavich]

I agree that we won't be able to support descending into tag schemas by composing jsonschema's validator or modifying the evolve-able attributes.

Should asdf simply vendorize jsonschema at a version that works for us? Our use case is bizarre and I don't think it's reasonable to expect jsonschema to officially support the level of customization that we need. Since we're stuck on draft 4 anyway, we won't miss the new features, and that draft is nearly a decade old so I don't expect that there will be much to maintain.

[Julian]

As far as I can tell there isn't an interface provided by jsonschema which is rich enough to enable asdf to add its additional logic on top of the jsonschema Validator's iter_errors method. As far as I can tell, asdf truly needs to modify the iter_errors method in order to handle validation with respect to yaml tags (which are not part of the json specification and so are outside the jsonschema specification), which are used as a core part of asdf's functionality.

If there's any detail you can share about what's missing that'd definitely be useful to hear, though I still haven't dug in in detail yet, apologies.

[WilliamJamieson]

If there's any detail you can share about what's missing that'd definitely be useful to hear, though I still haven't dug in in detail yet, apologies.

Currently the iter_errors that asdf uses is:

asdf/asdf/schema.py

Lines 266 to 311 in ebac36e

	def iter_errors(self, instance, *args, **kwargs):
	# We can't validate anything that looks like an external reference,
	# since we don't have the actual content, so we just have to defer
	# it for now. If the user cares about complete validation, they
	# can call `AsdfFile.resolve_references`.
	with self._context:
	if self._context.seen(instance, self.schema):
	# We've already validated this instance against this schema,
	# no need to do it again.
	return
	if not visit_repeat_nodes:
	self._context.add(instance, self.schema)
	if (isinstance(instance, dict) and "$ref" in instance) or isinstance(instance, reference.Reference):
	return
	if not self.schema:
	tag = getattr(instance, "_tag", None)
	if tag is not None:
	if self.serialization_context.extension_manager.handles_tag_definition(tag):
	tag_def = self.serialization_context.extension_manager.get_tag_definition(tag)
	schema_uris = tag_def.schema_uris
	else:
	schema_uris = [self.ctx.tag_mapping(tag)]
	if schema_uris[0] == tag:
	schema_uris = []
	# Must validate against all schema_uris
	for schema_uri in schema_uris:
	try:
	with self.resolver.resolving(schema_uri) as resolved:
	yield from self.descend(instance, resolved)
	except RefResolutionError:
	msg = "Unable to locate schema file for '{}': '{}'"
	warnings.warn(msg.format(tag, schema_uri), AsdfWarning)
	if isinstance(instance, dict):
	for val in instance.values():
	yield from self.iter_errors(val)
	elif isinstance(instance, list):
	for val in instance:
	yield from self.iter_errors(val)
	else:
	yield from super().iter_errors(instance)

It provides two functions:

1. It prevents the validator from getting caught in "reference cycles" as yaml provides anchors which allow one to reference already stated information. See

   asdf/asdf/schema.py

   Lines 272 to 278 in ebac36e

   	if self._context.seen(instance, self.schema):
   	# We've already validated this instance against this schema,
   	# no need to do it again.
   	return
   	if not visit_repeat_nodes:
   	self._context.add(instance, self.schema)

2. More importantly for asdf it handles the yaml tags, which asdf uses in a similar manner to $ref but instead asdf needs to pull a specific schema and use it to validate only the subtree in question against this new schema. This includes validating that the yaml tag from the data matches the one listed in the schema. See

   asdf/asdf/schema.py

   Lines 284 to 301 in ebac36e

   	tag = getattr(instance, "_tag", None)
   	if tag is not None:
   	if self.serialization_context.extension_manager.handles_tag_definition(tag):
   	tag_def = self.serialization_context.extension_manager.get_tag_definition(tag)
   	schema_uris = tag_def.schema_uris
   	else:
   	schema_uris = [self.ctx.tag_mapping(tag)]
   	if schema_uris[0] == tag:
   	schema_uris = []
   	# Must validate against all schema_uris
   	for schema_uri in schema_uris:
   	try:
   	with self.resolver.resolving(schema_uri) as resolved:
   	yield from self.descend(instance, resolved)
   	except RefResolutionError:
   	msg = "Unable to locate schema file for '{}': '{}'"
   	warnings.warn(msg.format(tag, schema_uri), AsdfWarning)

The remaining components of the iter_errors we use just set it up so that it operates like the jsonschema iter_errors or directly call the jsonschema iter_errors itself.

The main point is that asdf has to do some work to handle yaml/asdf specific things outside the scope of the original iter_errors method (checking/handling them prior to calling down into the original iter_errors). The natural way (to me) we can support this is via subclassing, but I am open to suggestions.

[braingram]

Looks good to me.