Opened 2 years ago

Closed 10 months ago

#19671 closed Bug (fixed)

Model field option "validators" not working with ManyToManyField

Reported by: fabio@… Owned by: anubhav9042
Component: Forms Version: master
Severity: Normal Keywords: validators ManyToManyField
Cc: flo@…, loic84 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

# models.py

def validate_depends_on(value):
    # just raise an error, whatever the value
    raise ValidationError('Error')

class Package(models.Model):
    # ...
    depends_on = models.ManyToManyField(
        'self',
        symmetrical=False,
	related_name='required_by',
        blank=True,
        null=True,
        validators=[validate_depends_on]
        )

My custom validator is not called when saving a model instance through the admin. The same validator perfectly works with other fields. Also, removing blank and null options didn't help.

Attachments (3)

test_m2m_validator_called.diff (2.2 KB) - added by fhahn 2 years ago.
test that checks if a custom validator of a m2m field gets called during form validation
test_m2m_validator_called-2.diff (2.2 KB) - added by claudep 2 years ago.
Same test updated to master
19671_patch1.diff (2.8 KB) - added by anubhav9042 10 months ago.

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by fhahn

test that checks if a custom validator of a m2m field gets called during form validation

comment:1 Changed 2 years ago by fhahn

  • Cc flo@… added
  • Component changed from Uncategorized to Forms
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to fhahn
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Version changed from 1.4 to master

I've added a test that checks the behavior described by the author of the ticket.

I think part of the problem is, that m2m validation cannot be done before saving the object, because the object needs a pk before a m2m related object can be added (using .add()). But the validator could be used during form validation. If not, we should at least document that the validators kwarg does not work for ManyToManyField (the null and blank kwarg are ignored too).

comment:2 Changed 2 years ago by carljm

  • Triage Stage changed from Unreviewed to Accepted

Not immediately obvious to me whether this will be reasonably fixable or not, but if not then a documentation fix is still clearly required.

Changed 2 years ago by claudep

Same test updated to master

Changed 10 months ago by anubhav9042

comment:3 Changed 10 months ago by anubhav9042

  • Has patch set
  • Owner changed from fhahn to anubhav9042

comment:4 Changed 10 months ago by anubhav9042

As Florian(fhahn) says that validators could be used in case of forms, I too am of the same opinion because in case of forms, when we provide the entire data all at once, we can surely run the validators on it when full_clean() on ModelForm is called, for which I have attached a patch.
And also we should update the docs saying that validators for M2M have meaning only when ModelForm is used as otherwise they can't because the m2m validators play no role in model instance's full_clean() is called, but definitely when form's full_clean() is being called as it is part of form data and must be validated.
The patch is also available at https://github.com/coder9042/django/compare/gsoc_19671

Last edited 10 months ago by anubhav9042 (previous) (diff)

comment:5 Changed 10 months ago by anubhav9042

Regarding the kwargs null and blank. They do not seem to be meaningful in case of M2M as if either is null then there is actually no relation, so it should be mentioned in the docs.

comment:6 Changed 10 months ago by loic84

Proposed patch from Anubhav tries to reuse ManyToManyField.validators at the form level and I don't think we should do that: model field validators are for model validation only.

It was proposed on IRC to make obj.m2m.add() use the validators, but that's conflicting with the design decision that model validation should be completely optional.

Regarding null it obviously has no meaning on a M2M since there is no corresponding field on the model. For blank I'm pretty sure that model validation would completely ignore it as checking it would require an additional query; however, model forms and admin might, so it's worth double checking.

I'm leaning towards a docs patch, and possibly system checks.

comment:7 Changed 10 months ago by loic84

  • Cc loic84 added

comment:9 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 011abb7d96c75f6154c15a8a0f997d8c27698679:

Fixed #19671 -- Added warnings that null and validators are ignored for ManyToManyField.

Thanks Loic Bistuer and Tim Graham for help and review.

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