Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#16168 closed Cleanup/optimization (fixed)

Form validation is bypassed on Admin Inline ModelForms that define extra fields

Reported by: izzaddin.ruhulessin@… Owned by: nobody
Component: Documentation Version: dev
Severity: Normal Keywords: validation, modelforms
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When defining extra/overridden fields on modelforms, validation is bypassed. This can lead to errors under some circumstances.

For example, you have a model 'Book' that defines a ForeignKey to 'Author' named 'author'. On your ModelForm you replace this field by a CharField (or some other type) for whatever reason.

On submitting an invalid form, a ValueError will be raised complaining that the datatype is incorrect.

My proposed solution is overriding ModelForm._post_clean():

class ModelForm(BaseModelForm):

    def _post_clean(self):
        if len(self._errors) == 0:
            super(ModelForm, self)._post_clean()
        

Attachments (2)

16168.diff (673 bytes ) - added by Pieter Swinkels 13 years ago.
changes to the documentation
16168_v2.diff (694 bytes ) - added by Christopher Medrela 13 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Stephen Burrows, 13 years ago

Component: FormsDocumentation
Needs documentation: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.2SVN

This does happen in svn as well. However, the problem is not that validation is being bypassed; because of the invalid input type from the form field, the attempt to construct an instance fails loudly. There is no way to get around this cleanly - it's a programming error, not a problem with django. The most we could do would be to add a note to the docs about the dangers of not mimicking data type conversion when overriding fields.

comment:2 by Pieter Swinkels, 13 years ago

UI/UX: unset

DjangoCon Europe 2011 sprint: commenced work on this ticket

by Pieter Swinkels, 13 years ago

Attachment: 16168.diff added

changes to the documentation

comment:3 by Pieter Swinkels, 13 years ago

I have attached a diff with (I hope) the requested documentation changes.

comment:4 by Daniel Watkins, 13 years ago

Has patch: set

comment:5 by Stephen Burrows, 13 years ago

Patch needs improvement: set

Looks okay to me, but it needs appropriate rst markup (backticks around ValueError is all that I see missing.)

by Christopher Medrela, 13 years ago

Attachment: 16168_v2.diff added

comment:6 by Christopher Medrela, 13 years ago

Needs documentation: unset
Patch needs improvement: unset

I added backticks around ValueError in PieterSwinkels patch.

comment:7 by Tim Graham <timograham@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [ebbc414d1729f24e395ee050a62276229c59d75d]:

Fixed #16168 - Added note regarding type requirements when overridng ModelForm fields.

Thanks Pieter Swinkels for the patch.

comment:8 by Tim Graham <timograham@…>, 12 years ago

In [a276bdebb2e8ef4619d0cb9a02b0b9f4f73ee998]:

[1.4.X] Fixed #16168 - Added note regarding type requirements when overridng ModelForm fields.

Thanks Pieter Swinkels for the patch.

Backport of ebbc414d17 from master

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