Code

Opened 3 years ago

Closed 20 months ago

Last modified 20 months ago

#14885 closed Cleanup/optimization (fixed)

is_valid for ModelForm changes instance if instance is provided

Reported by: italomaia Owned by: stumbles
Component: Documentation Version: 1.2
Severity: Normal Keywords: forms, is_valid, bad data
Cc: bronger Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

See this example:

def val_check(value):
  from django.core.exceptions import ValidationError
  raise ValidationError('Some reason')

class Model(models.Model):
  image = models.ImageField(upload_to='some/place', validators=[val_check(])

class ModelForm(forms.ModelForm):
  pass

def view(request, model_id):
  instance = Model.objects.get(pk=model_id)
  if request.method=='POST':
    form = ModelForm(request.POST, instance=instance)
  else: form = ModelForm(instance=instance)
  if form.is_bound and form.is_valid():
    form.save()
    # do something else
  return simple.direct_to_template(request, {'template':'some.html', 'extra_context':{'model':instance}})

In the example above, if i send a post request with some data for the field image, instance will be modified. If i use the value of instance in my 'some.html' i'll get invalid data in my template. This seems wrong.

Attachments (1)

trac#14885-is_valid.diff (1.1 KB) - added by stumbles 20 months ago.
Explain that is_valid() changes the model instance in place.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

It is not clear from the report what this ticket is reporting, it doesn't help at all that the example code isn't correct/complete:

  • The view isn't instanciating the ModelForm with request-FILES, something absolutely necessary when dealing with file uploads.
  • The ModelForm definition is incomplete.
  • The code seems to be using some third party simple.direct_to_template() function.
  • The validators argument value has a syntax error.

If the OP or anyone wants to reopen this, please provide a working test case and a clear description of the issue at hand.

Remember Since Django 1.2 ModelFom validation also triggers Model validation

comment:2 Changed 3 years ago by ramiro

Oops, anonymous that close the ticket was me.

comment:3 Changed 2 years ago by bronger

  • Cc bronger added
  • Component changed from Uncategorized to Forms
  • Easy pickings unset
  • Resolution invalid deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Bug
  • UI/UX unset

If necessary I will create a snippet for reproducing this, but actually the summary is complete: If you pass an "instance" to a model form constructor and later on call ".is_valid()" on that form, that given instance is changed in place. Very often you don't care about this object. But for example, if you want to compare the given instance with the new one returned by my_model_form.save(), changes are never detected.

I cannot read this behaviour from the docs. It is not about model validation here: The fields are really updated and not just normalised.

See also http://permalink.gmane.org/gmane.comp.python.django.user/132931 and http://groups.google.com/group/django-users/msg/2e31bfaefd8351cf.

comment:4 Changed 2 years ago by aaugustin

  • Component changed from Forms to Documentation
  • Easy pickings set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

Accepting as a documentation issue. We could make it clearer that calling is_valid() will update the instance passed to the ModelForm.

Changed 20 months ago by stumbles

Explain that is_valid() changes the model instance in place.

comment:5 Changed 20 months ago by stumbles

  • Has patch set
  • Owner changed from nobody to stumbles
  • Status changed from reopened to new

I've added a patch to point out both that is_valid() causes the model instance to be modified in place, and also that this modification can be partial.

comment:6 Changed 20 months ago by Tim Graham <timograham@…>

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

In [3fd89d99036696ba08dd2dd7e20a5b375f85d23b]:

Fixed #14885 - Clarified that ModelForm cleaning may not fully complete if the form is invalid.

Thanks Ben Sturmfels for the patch.

comment:7 Changed 20 months ago by Tim Graham <timograham@…>

In [42aee6ffe5f16852347e0cf069447950e9d2ef85]:

[1.4.x] Fixed #14885 - Clarified that ModelForm cleaning may not fully complete if the form is invalid.

Thanks Ben Sturmfels for the patch.

Backport of 3fd89d99036696ba08dd2dd7e20a5b375f85d23b from master.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.