4.10.0 breaks libraries that subclass validator classes

[Wim-De-Clercq]

The failing code is here:
https://github.com/p1c2u/openapi-schema-validator/blob/4d8fd4c2bddce50931ea617bca54d526ae0535cd/openapi_schema_validator/validators.py#L78-L96

It does look very strange that the new evolve method doesn't keep the attributes of the OAS30Validator class.
There they have an @attrs class. It defines a read and write attribute. And after evolving, not only are the read and write attributes gone, but super calls as line 96 don't work either.

I have also made an issue in their project: python-openapi/openapi-schema-validator#46

I am not sure which one of the projects should update. But I feel that this new evolve method only does half of what it used to do and is not good enough to replace an attrs.evolve

jsonschema/jsonschema/validators.py

Lines 204 to 216 in ae0feea

	def evolve(self, **changes):
	schema = changes.setdefault("schema", self.schema)
	NewValidator = validator_for(schema, default=Validator)
	# Essentially reproduces attr.evolve, but may involve instantiating
	# a different class than this one.
	for field in attr.fields(Validator):
	if not field.init:
	continue
	attr_name = field.name # To deal with private attributes.
	init_name = attr_name if attr_name[0] != "_" else attr_name[1:]
	if init_name not in changes:
	changes[init_name] = getattr(self, attr_name)

[Julian]

At first glance that code seems to be subclassing validators, which isn't something that's really supported, so I guess I'm not surprised it unfortunately changed behavior. It may be that it's possible to fix easily though (both here and there). I'd have to see why they're doing what's there to really know what they should be doing instead, and really they should use a supported way of extending validator behavior (and really I should have long ago explicitly made subclassing warn and then raise an error rather than just saying it in various issues here :/, so that's my mistake.)

[Julian]

The commit I just pushed (and release which should go out in a few minutes) should fix this I believe -- though as I mentioned:

[Subclassing validators] wasn't intended to be public API, but given it didn't warn or raise an error it's of course understandable [that it was done in some places]. The next release however will make it warn (and a future one will make it error). If you need help migrating usage of inheriting from a validator class feel free to open a discussion and I'll try to give some guidance

I didn't take the longest look at asdf or what the OpenAPI repo was doing but I think both should be relatively easy to do without inheritance. If more help's needed, yeah, feel free to follow up!

[Wim-De-Clercq]

I can confirm that the commit fixes the majority of the issues related to the super(...) call for the opanapi library.

The issue of missing attributes still persists, but I suppose they can fix that with composition instead of inheritance, or however else they plan to adjust their validators.

[Julian]

Thanks! Hm, I don't think there's anything else different happening now relative to what attrs did itself -- do you have any way I can reproduce what you're saying about missing attributes without digging through OpenAPI itself?

[Wim-De-Clercq]

I think this is a simplified bit of code that does what they do

from attr import attrib
from attr import attrs
from jsonschema.validators import create

BaseValidator = create(meta_schema={})


@attrs
class Validator(BaseValidator):
    read: bool = attrib(default=None)
    write: bool = attrib(default=None)

    def bug(self):
        return self.evolve()


validator = Validator(write=True, schema={})
evolved = validator.bug()

print(validator.write)
print(evolved.write)
assert validator.write == evolved.write

[Wim-De-Clercq]

It could be as simple as changing the field iterator to self.__class__ as well.

            for field in attr.fields(self.__class__):

But I'm not sure of all the implications. Haven't really looked much into what the code truly does and needs for either of the projects.

[Julian]

Ah, indeed. OK, the second place is fixed too in the commit I just pushed, thanks.

[WilliamJamieson]

Unfortunately, even with the latest changes (a8c3b14), ASDF is still broken by jsonschema (CI report: https://github.com/asdf-format/asdf/runs/7884285535?check_suite_focus=true). I think the main error ASDF is getting is:

self = ASDFValidator(schema={}, format_checker=None, _context=<asdf.schema._ValidationContext object at 0x7f48f96dfee0>, ctx=...df.AsdfFile object at 0x7f48f96add30>, serialization_context=<asdf.asdf.SerializationContext object at 0x7f48f96df640>)
changes = {'context': <asdf.schema._ValidationContext object at 0x7f48f96dfee0>, 'ctx': <asdf.asdf.AsdfFile object at 0x7f48f96add30>, 'format_checker': None, 'resolver': <jsonschema.validators.RefResolver object at 0x7f48f96df310>, ...}
cls = <class 'asdf.schema._create_validator.<locals>.ASDFValidator'>
schema = {'$schema': 'http://stsci.edu/schemas/yaml-schema/draft-01', 'additionalProperties': True, 'definitions': {'history-1....ray'}}, 'type': 'object'}}, 'description': 'This schema contains the top-level attributes for every ASDF file.\n', ...}
NewValidator = <class 'jsonschema.validators.Draft202012Validator'>
field = Attribute(name='serialization_context', default=None, validator=None, repr=True, eq=True, eq_key=None, order=True, ord...None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False, on_setattr=None)
attr_name = 'serialization_context', init_name = 'serialization_context'

    def evolve(self, **changes):
        # Essentially reproduces attr.evolve, but may involve instantiating
        # a different class than this one.
        cls = self.__class__
    
        schema = changes.setdefault("schema", self.schema)
        NewValidator = validator_for(schema, default=cls)
    
        for field in attr.fields(cls):
            if not field.init:
                continue
            attr_name = field.name  # To deal with private attributes.
            init_name = attr_name if attr_name[0] != "_" else attr_name[1:]
            if init_name not in changes:
                changes[init_name] = getattr(self, attr_name)
    
>       return NewValidator(**changes)
E       TypeError: __init__() got an unexpected keyword argument 'context'

as the last item in the traceback. Still with the emitted warning:

DeprecationWarning: The metaschema specified by $schema was not found. Using the latest draft to validate, but this will raise an error in the future.

I am happy to discuss this in a separate issue and/or discuss how to migrate ASDF away from using the unintended API. I

[Julian]

@WilliamJamieson aw. I'll reopen the other issue you opened and we can diagnose (both how to fix now and then yeah later too)