Opened 11 years ago

Closed 11 years ago

#22346 closed Uncategorized (invalid)

ModelForm modifies the Model instance before save is called

Reported by: lehins@… Owned by: nobody
Component: Forms Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

So here is a use case that is relevant to me:
I need a model instance's primary key(pk) before I save the model, but only after the model form is validated. In particular I needed it to use in a pre_save method on a file field. So my logical solution was:

try:
   instance = CustomModel.objects.get(pk=pk)
except CustomModel.DoesNotExist:
   instance = CustomModel()
form = CustomModelForm(instance=instance, data=request.POST, files=request.FILES))
if form.is_valid():
   if not instance.pk:
       instance.save() # saves the model while generating a pk
   instance = form.save()

To my disappointment, after a few wasted hours, I figured out that binding data to a model instance modifies that instance, so when I am calling instance.save() it already contains data from the form, before even calling form.save() method.
After finding this ticket: #14885 I also noticed that it may be a partial update of the model instance. Despite closed ticket I couldn't find it in current documentation.
In my opinion it is completely illogical. Model instance should only be modified only upon form.save() call, this would also give a real use case for the existence of commit=False argument: "apply cleaned form data to model instance, but do not commit to database."
Even documentation follows the logic I am proposing: "A subclass of ModelForm can accept an existing model instance as the keyword argument instance; if this is supplied, save() will update that instance"

Overall it sounds to me like a side effect or a bug. If it truly is an unavoidable side-effect, I believe, we should get at least a really good explanation of it in the documentation.

Change History (1)

comment:1 by lehins@…, 11 years ago

Resolution: invalid
Status: newclosed

Never mind, I just found a Warning in Documentation, and it actually makes sense, since model fields need to be validate as well.

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