Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#27039 closed Bug (fixed)

ModelFields with 'default' value set and 'required'=False in form does not use default value

Reported by: Ivan Belokobylskiy Owned by: nobody
Component: Forms Version: 1.10
Severity: Release blocker Keywords: default non-required modelform
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have Model M with corresponding ModelForm

class M(models.Model):
    f = models.CharField(max_length=255, default=u'default_value')

class MF(forms.ModelForm):
    class Meta:
        model = M
        fields = ['f']

    f = forms.CharField(required=False)

I save form with empty data, expecting receive default value in field

mf = MF({})

if mf.is_valid():
    m = mf.save(commit=False)

m.f 
>>> u''

expected behavior

m.f 
>>> u'default_value'

Commits, that broke previous behavior:
https://github.com/django/django/commit/375e1cfe2b2e1c3c57f882147c34902c6e8189ac

Offered patch:

--- django/forms/models.py	(revision 35225e2ade08ea32e36a994cd4ff90842c599e20)
+++ django/forms/models.py	(revision )
@@ -385,7 +385,7 @@
                 exclude.append(name)
 
         try:
-            self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
+            self.instance = construct_instance(self, self.instance, opts.fields, exclude)
         except ValidationError as e:
             self._update_errors(e)

Attachments (1)

27039-test.diff (850 bytes) - added by Tim Graham 3 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by Claude Paroz

Needs tests: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks, but a test is still missing in the patch.

comment:2 Changed 3 years ago by Tim Graham

I'd like to hear more about the use case. It might be argued that defaults should only apply to the population of initial forms and that if a user removes that value and submits a blank form, Django shouldn't transform it back to the default. Of course if we decide on that resolution, we'll have to document the change. If we decide to keep the old behavior, I'm attaching a regression test.

Changed 3 years ago by Tim Graham

Attachment: 27039-test.diff added

comment:3 Changed 3 years ago by Claude Paroz

If a user removes the value in a form, the form should receive {'f': ''}, not an empty dict. I still think we should fix the regression.

comment:4 Changed 3 years ago by Tim Graham

Makes sense to me, however, on older Django versions, {'f': ''} as the data also results in an instance with the model field default rather than an empty string. Do you feel the behavior should be restored for that case as well?

comment:5 Changed 3 years ago by Claude Paroz

No, IMHO {'f': ''} should really end up with the field being assigned the empty string, if possible. Quoting (and agreeing with) you: if a user removes that value and submits a blank form, Django shouldn't transform it back to the default.

comment:6 Changed 3 years ago by Tim Graham

Needs tests: unset
Patch needs improvement: set

I drafted a pull request, however, it demonstrates a small problem with the idea that values not present in POST data should fallback to their default. For HTML widgets like checkboxes, if the element isn't checked, it won't appear in POST data. In such a case, it's inappropriate to fallback to the model field's default if it's True. The current patch special cases this with isinstance(form[f.name].field.widget, CheckboxInput) to fix some test failures but that hardly seems ideal.

comment:7 Changed 3 years ago by Claude Paroz

I see. So maybe another approach could be documenting that default values are used to populate initial blank forms, but not to fill missing data from the form input.

comment:8 Changed 3 years ago by Tim Graham

I asked on django-developers to try to get a consensus on how to proceed.

comment:9 Changed 3 years ago by Tim Graham

Patch needs improvement: unset

The patch might be ready now.

comment:10 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 4bc6b93:

Fixed #27039 -- Fixed empty data fallback to model field default in model forms.

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

In 325dd0b:

[1.10.x] Fixed #27039 -- Fixed empty data fallback to model field default in model forms.

Backport of 4bc6b939944183533ae74791d21282e613f63a96 from master

comment:12 Changed 3 years ago by Tim Graham

Has patch: unset
Resolution: fixed
Status: closednew

Reopening as Alex Hill reported that this isn't working with prefixed forms.

comment:13 Changed 3 years ago by Alex Hill

The issue was that when a field on a prefixed form had a default, its (unprefixed) name wasn't found in form.data, so the instance would always be populated with the default value. Fixed with form.add_prefix().

PR at https://github.com/django/django/pull/7195

comment:14 Changed 3 years ago by Tim Graham <timograham@…>

In d9c083cf:

Refs #27039 -- Fixed regression with field defaults in prefixed forms.

comment:15 Changed 3 years ago by Tim Graham <timograham@…>

In db3eabf:

[1.10.x] Refs #27039 -- Fixed regression with field defaults in prefixed forms.

Backport of d9c083cfee853272ded14c6c87623e910c9e81c4 from master

comment:16 Changed 3 years ago by Tim Graham

Resolution: fixed
Status: newclosed

comment:17 Changed 2 years ago by Matt Davis

I am noticing an odd behavior upgrading a Django app and I have traced it to this ticket. I have upgraded the app from 1.8 to 1.10 and all tests are passing, however if I upgrade from 1.10 to 1.10.1 one of my tests fail. I have determined the reason why to be its a ModelForm and the model has a field defined with a default value:

field_x = models.CharField(max_length=100, db_index=True, blank=True, default='')

The model form also has a clean method that if a value isn't supplied for field_x that it will generate one:

def clean_field_x(self):
    token = self.cleaned_data.get('field_x')
    if not token:
        token = utils.generate_token()
    return token

It would appear that in 1.10.1 it changed from using the generated token in the clean method, to then preferring the model field's default value. I think this behavior is incorrect, shouldn't it prefer the form's cleaned data over the model defaults?

Last edited 2 years ago by Matt Davis (previous) (diff)

comment:18 Changed 2 years ago by Tim Graham

The behavior you describe isn't what's implemented. Hopefully it's not documented anywhere. It should be possible to adapt your code. For example, this might work:

if not token:
    token = utils.generate_token()
    self.data['field_x'] = token

by inserting a value in self.data, the model fallback logic won't be triggered, I think. If you think we could improve the behavior and have a patch to propose, feel free to open a new ticket.

comment:19 Changed 2 years ago by ljsjl

I've run into what I would call a bug (or at least unexpected - to me - behaviour) which is related to this, running Django 1.11.1. I'm happy to break this out into another ticket if you'd prefer.

While the argument can be made for a user submitted blank/empty string overriding the field default, does this make sense in the case of an IntegerField (or any number field really)? If I have e.g. models.IntegerField(blank=True, default=0) in a model then currently a user can submit a blank value to a ModelForm for this model by deleting the field initial value, and the field value will travel through form/model cleaning as None.

If I then have couple of IntegerFields like the above and model.clean() does a check on their sum, it will fail when attempting to add NoneType. I would say it is reasonable to expect a number based field to contain a number by the time it hits model.clean() via a form, either a valid user submitted value or default if the submitted field is empty. Obviously I can guard the arithmetic statement with a test or try: clause, but in my mind I set a default value so I expect to see something valid. Maybe others expect otherwise :)

comment:20 Changed 2 years ago by Tim Graham

I'm doubtful that further complexity in this area is a good idea but this isn't a good place to discuss it. Design decisions are discussed on the DevelopersMailingList. Providing a patch to show your idea is feasible and wouldn't break backwards compatibility would help the discussion greatly.

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