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__().)

Change History (1)

comment:1 by Simon Charette, 9 hours ago

Triage Stage: UnreviewedAccepted

I wonder if we should compute the identity derived from resolving _constructor_args against __init__ (thanks to @deconstructible) like we do with Expression.identity to prevent these from slipping through.

Note: See TracTickets for help on using tickets.
Back to Top