Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#13138 closed (fixed)

Precedence of "blank" model attribute and "required" form attribute is unclear

Reported by: saxon75@… Owned by: nobody
Component: Forms Version: 1.2-beta
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

There appears to be a discrepancy between model/form validation in 1.2 and form validation in 1.1.1. Previously, a ModelForm field defined in the form subclass with the "required=False" option would not throw a ValidationError if the field was submitted blank, regardless of whether or not the model field was defined with "blank=True." This appears not to be the case in 1.2--if you submit a blank ModelForm field without the model field being defined with "blank=True," it will still throw a ValidationError even if you have the ModelForm field with "required=False." It's unclear which setting should take precedence when the two conflict (especially in cases where one is set explicitly and the other is left as the default, e.g. required=False in the form and no explicit declaration in the model) and the documentation doesn't appear to note the necessity of explicitly setting both options the same way.

Attachments (1)

13138_test.diff (625 bytes) - added by kmtracey 5 years ago.

Download all attachments as: .zip

Change History (6)

Changed 5 years ago by kmtracey

comment:1 Changed 5 years ago by kmtracey

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Attached test passes on the 1.1.X branch. On trunk it fails:

======================================================================
FAIL: Doctest: modeltests.model_forms.models.__test__.API_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kmt/tmp/django/trunk/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for modeltests.model_forms.models.__test__.API_TESTS
  File "/home/kmt/tmp/django/trunk/tests/modeltests/model_forms/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/kmt/tmp/django/trunk/tests/modeltests/model_forms/models.py", line ?, in modeltests.model_forms.models.__test__.API_TESTS
Failed example:
    wf2.is_valid()
Expected:
    True
Got:
    False
----------------------------------------------------------------------
File "/home/kmt/tmp/django/trunk/tests/modeltests/model_forms/models.py", line ?, in modeltests.model_forms.models.__test__.API_TESTS
Failed example:
    wf2.errors
Expected:
    {}
Got:
    {'name': [u'This field cannot be blank.']}


----------------------------------------------------------------------
Ran 14 tests in 1.377s

FAILED (failures=1)

Unless I'm missing something, this seems to be a valid use case: have the field be non-required on the form but fill in a value in between the form.is_valid() call and the actual model save?

comment:2 Changed 5 years ago by saxon75@…

Yes, that is a valid use case. However, at least in r12794, calling form.is_valid() runs both form and model validation, and will return you to the form with a ValidationError before you can do an actual model save. This becomes problematic in cases where you want to autocomplete certain form values if they are submitted blank. You might use code like so:

if form.is_valid():

model = form.save(commit=False)
if not form.cleaned_datafield?:

model.field = model.other_field.lower()

model.save()

In this case, if you have form.field declared with "required=False" but model.field declared with "blank=False" (or with "blank" left as default), you will get a ValidationError, the autocomplete value will not be applied to the model, and the model will not be saved. Having blank=False required for this sort of behavior seems potentially problematic because you may not want the model to ever actually be saved with model.field blank, you may only want the form to accommodate a blank field.

comment:3 Changed 5 years ago by kmtracey

Actually, it looks like the model form code is intending to exclude fields like this from the model-level validation, see: http://code.djangoproject.com/browser/django/trunk/django/forms/models.py#L295

Something is not quite working right there, it seems.

comment:4 Changed 5 years ago by kmtracey

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

(In [12802]) Fixed #13138: Ensure required=False on a model form field overrides
blank=False on the underlying model field during model form clean,
in order to maintain backward compatibility. Thanks to saxon75 for
the report.

comment:5 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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