Opened 12 hours ago
Last modified 9 hours ago
#37221 new Uncategorized
Validator equality checks miss unequal configurations
| Reported by: | Mike Edmunds | Owned by: | |
|---|---|---|---|
| Component: | Core (Other) | Version: | 6.0 |
| Severity: | Normal | Keywords: | validators |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The validator __eq__() methods added in #21638 (a68f32579145dfbd51d87e14f9b5e04c770f9081) miss some cases where the validators are not actually equal. This could lead to missing migrations when changing a field's validators.
1. EmailValidator.__eq__() does not check its regex attrs
This test should fail, but does not:
def test_email_validator_regex_equality(self): validate_email_no_idna = EmailValidator() validate_email_no_idna.hostname_re = DomainNameValidator.ascii_only_domain_re validate_email_no_idna.domain_re = DomainNameValidator.ascii_only_domain_re validate_email_no_idna.tld_no_fqdn_re = DomainNameValidator.ascii_only_tld_re self.assertNotEqual(validate_email_no_idna, validate_email)
Compare to the RegexValidator-based validators, which do consider their regex attr for __eq__(). (E.g., DomainNameValidator(accept_idna=True) != DomainNameValidator(accept_idna=False) correctly, even though DomainNameValidator doesn't implement its own __eq__() method to check accept_idna.)
2. No validator __eq__() methods check the class
Custom validator subclasses may implement different logic, so should probably not be considered equal by default. I would expect this test to fail but it does not:
def test_custom_domain_name_validator_equality(self): class CustomDomainNameValidator(DomainNameValidator): def __call__(self, value): super().__call__(value) if value.lower().endswith((".info", ".xyz", ".biz")): raise ValidationError( self.message, code=self.code, params={"value": value} ) self.assertNotEqual(CustomDomainNameValidator(), DomainNameValidator())
This might be considered a programming error that CustomDomainNameValidator doesn't override the base __eq__() method. But—particularly if we're going to encourage custom subclasses for tweaking validator functionality—it would probably be safer for the base equality check to require the same class. (And subclasses that do want to claim equality with base could override __eq__().)
I wonder if we should compute the identity derived from resolving
_constructor_argsagainst__init__(thanks to@deconstructible) like we do withExpression.identityto prevent these from slipping through.