Opened 15 years ago
Last modified 12 years ago
#12627 new Bug
If all fields are readonly or editable=False, their ModelForm is always valid and can raise exceptions
Reported by: | KyleMac | Owned by: | anonymous |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | readonly_fields editable |
Cc: | kitsunde@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If all fields are marked as readonly_fields on a ModelAdmin, or they're editable=False in the actual model, then their ModelForm will always be valid and it will try to save the data. If every field has a default or allows null then a blank row will be added, if not then an IntegrityError will be raised.
Attachments (2)
Change History (22)
comment:1 by , 15 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 15 years ago
comment:4 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:5 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
by , 15 years ago
Attachment: | 12627.diff added |
---|
comment:7 by , 15 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Status: | new → assigned |
The cases described by this issue end up in an empty fields dictionary which causes is_valid() and related methods to skip validation on the form altogether. This patch checks for the existence of fields and if non are present, adds a top level validation error.
comment:8 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:9 by , 15 years ago
Status: | new → assigned |
---|
by , 15 years ago
Attachment: | 12627_r12827_with_test.diff added |
---|
comment:10 by , 15 years ago
Seems like we need to do something here, but I'm not sure what. Current patch fixes it at the form level, saying a form with no fields can't ever be valid. That seems reasonable, but it is in fact a change. What used to work:
>>> from django import forms >>> class MF(forms.Form): ... pass ... >>> mf = MF({}) >>> mf.is_valid() True
will now fail:
>>> from django import forms >>> class MF(forms.Form): ... pass ... >>> mf = MF({}) >>> mf.is_valid() False >>> mf.errors {'__all__': [u'No data was submitted.']}
I don't know why anyone would ever intentionally create such a form and expect it to be valid, but I'm a bit nervous about the fix for an admin-specific problem being made at the basic form level. Could be I'm just overly paranoid?
comment:11 by , 15 years ago
Needs tests: | unset |
---|
It's not really an admin only issue, is it?
If you were to create a ModelForm for a Model with all fields marked as editable=False, wouldn't you run into this validation problem when trying to save it?
comment:12 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:13 by , 15 years ago
Sorry, this is a bit late to the party.
Any fix should also ensure that this sort of pattern still works:
class NonEditable(models.Model): sprocket = models.BooleanField(default=False, editable=False) class NonEditableModelForm(forms.ModelForm): class Meta: model = NonEditable f=NonEditableModelForm({}) instance=f.save()
as should this:
class NonEditable2(models.Model): sprocket = models.CharField(max_length=128) class NonEditable2ModelForm(forms.ModelForm): class Meta: model = NonEditable2 exclude = ( 'sprocket', ) f=NonEditable2ModelForm({}) instance=f.save(commit=False) instance.sprocket = 'no sprocket' instance.save()
If the model is defined such that you can construct a valid object from default values, then this should create a valid object. If your model is defined in such a way that you cannot, then an integrity error on save() is the correct error.
This would change documented behaviour wouldn't it? I see no requirement in the documentation for a model form to have editable fields.
comment:14 by , 15 years ago
Yup, I agree with tomevans222 -- any fix needs to ensure that a form with no fields *is* valid, and can be saved -- the example of excluding all fields (or allowing them to be saved via defaults) is in fact one I've used in the wild (don't ask).
I'm fairly sure, then, that forms are behaving correctly here, and the fix needs to happen at the admin level.
comment:15 by , 15 years ago
Has patch: | unset |
---|---|
milestone: | 1.2 → 1.3 |
I agree with Jacob; this isn't a forms issue, it's an admin validation issue. As far as I can make out the behavior noted by @tin_nqn is completely valid; although it's an unusual edge case, saving Musician is legal, and saving Albums should be a problem because you're not providing enough information in the form.
It would be nice if the potential problem with Album was caught as an admin validation issue, but the absence of admin validation for this use case isn't going to cause real problems in the wild. If you have an unsaveable form, you're going to get errors whenever you try. Basic release testing would catch should catch this issue.
This isn't a new problem, either. Album hits this problem without ever using readonly fields. Readonly fields makes the problem slightly more obvious, because it's a lot easier to configure a truly readonly admin declaration.
So - after discussing with Karen, we're going to bump this from the 1.2 milestone. If someone presents a fully working patch with admin validation for edge case forms like this before we cut the RC (hopefully in about a week), feel free to retarget this for 1.2.
comment:16 by , 14 years ago
milestone: | 1.3 → 1.4 |
---|---|
Severity: | → Normal |
Type: | → Bug |
comment:4 by , 12 years ago
Cc: | added |
---|
I've just could reproduce the bug. create a project ticket and app t12627
run admin and you can add blank Musicians and raise an IntegrityError if you try add an album