Opened 2 years ago

Closed 2 years ago

#19691 closed Cleanup/optimization (invalid)

Inconsistency in instantiating/validating ModelForms: CharFields vs. with ImageField only.

Reported by: pfeiffersimon@… Owned by: nobody
Component: Forms Version: 1.4
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

Hi I found a sligth inconsistency in the instantiation/validation of the different field types. Its kinda hard to describe so here some code:

Consider the following models:

class Text(models.Model):
    """Just a basic model to store some text"""
    create_date = models.DateTimeField(auto_now_add=True)
    content = models.TextField()


class Picture(models.Model):
    """Just a basic model to store a picture"""    
    create_date = models.DateTimeField(auto_now_add=True)
    picture=models.ImageField(upload_to="pics")

And the following forms:

from django.forms import ModelForm
from notebook.models import *

class TextForm(ModelForm):
    class Meta:
        model=Text
        
class PictureForm(ModelForm):
    class Meta:
        model=Picture

When you now instantiate PictureForm and a TextForm with a bound instance, but empty request.POST / request.FILES likes this:

#get_or_none() returns a valid model instance in both cases below
#req.POST and req.FILES are both empty QueryDict instances

form_text = TextForm(req.POST,instance=get_or_none(Text,pk=primary_key))
form_pic = PictureForm(req.POST,req.FILES ,instance=get_or_none(Picture,pk=primary_key))

#The validation of form_text 
form_text.is_valid() 
# >>> FALSE
form_pic.is_valid()
# >>> True

So basically, the PictureForm ignores that it gets handed an empty req.FILES & req.POST and just uses the instance that is bound.

Whereas, the TextModel throws errors which get rendered although it has a bound a instance:

# found with pdb
>>> form_text.is_valid() 
FALSE
>>>form_text.initial
{'content': u'Some content from the database', 'id': 15}

So as you can see its just a little inconsistency. I do not even know I should have reported it. If you are wondering how I came about doing something like that:

# Compact way of writing a create/edit form
def picture_edit(req,primary_keyk=None):
    
    form = PictureForm(req.POST,req.FILES ,instance=get_or_none(Picture,pk=primary_key))
    # Works for         ^^^^^^^^^^^^^^^^req.POST={} & req.FILES={}  and instance == Model()                             
  
    if req.method == 'POST': 
        if form.is_valid():
            form.save()
            return redirect("notebook_dashboard")
    
    return render(req, 'notebook/picture_edit.html', {'form': form, })

def text_edit(req,pk=None):
    
    form = TextForm(req.POST,instance=get_or_none(Text,pk=pk))
    # this does not   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  when req.POST={} and instance == Model()
    if req.method == 'POST': # If the form has been submitted...
        if form.is_valid():
            form.save()
            return redirect("notebook_dashboard") #Redirect to dash
            #Optional else clause not explicitly used
    import pdb; pdb.set_trace()
    return render(req, 'notebook/text_edit.html', {'form': form, })


Change History (1)

comment:1 Changed 2 years ago by rafales

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

This is intentional - imagine what would happen if users had to re-upload file every time they want to edit a model instance.

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