Opened 14 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)

12627.diff (965 bytes ) - added by NicoEchaniz 14 years ago.
12627_r12827_with_test.diff (3.3 KB ) - added by Colin Copeland 14 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:2 by tin_nqn, 14 years ago

Owner: changed from nobody to tin_nqn
Status: newassigned

comment:3 by tin_nqn, 14 years ago

I've just could reproduce the bug. create a project ticket and app t12627

#models.py
from django.db import models

class Musician(models.Model):
    first_name = models.CharField(max_length=50)
    last_name = models.CharField(max_length=50)
    instrument = models.CharField(max_length=100)
    
    def __unicode__(self):
		return "%s %s" % (self.first_name, self.last_name)

class Album(models.Model):
    artist = models.ForeignKey(Musician, editable=False)
    name = models.CharField(max_length=100, editable=False)
    release_date = models.DateField(editable=False)
    num_stars = models.IntegerField(editable=False)


    def __unicode__(self):
		return "%s by %s" % (self.name, self.artist)


#admin.py
from django.contrib import admin
from ticket.t12627.models import Musician, Album

class MusicianAdmin(admin.ModelAdmin):
    readonly_fields = ('first_name', 'last_name', 'instrument')
	
class AlbumAdmin(admin.ModelAdmin):
	pass    
    
admin.site.register(Musician, MusicianAdmin)
admin.site.register(Album, AlbumAdmin)


run admin and you can add blank Musicians and raise an IntegrityError if you try add an album

comment:4 by tin_nqn, 14 years ago

Owner: changed from tin_nqn to nobody
Status: assignednew

comment:5 by gonzalolarralde, 14 years ago

Owner: changed from nobody to gonzalolarralde
Status: newassigned

comment:6 by NicoEchaniz, 14 years ago

Owner: changed from gonzalolarralde to NicoEchaniz
Status: assignednew

by NicoEchaniz, 14 years ago

Attachment: 12627.diff added

comment:7 by NicoEchaniz, 14 years ago

Has patch: set
Needs tests: set
Status: newassigned

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 Colin Copeland, 14 years ago

Owner: changed from NicoEchaniz to Colin Copeland
Status: assignednew

comment:9 by Colin Copeland, 14 years ago

Status: newassigned

by Colin Copeland, 14 years ago

Attachment: 12627_r12827_with_test.diff added

comment:10 by Karen Tracey, 14 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 NicoEchaniz, 14 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 anonymous, 14 years ago

Owner: changed from Colin Copeland to anonymous
Status: assignednew

comment:13 by tomevans222, 14 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 Jacob, 14 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 Russell Keith-Magee, 14 years ago

Has patch: unset
milestone: 1.21.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 Matt McClanahan, 13 years ago

milestone: 1.31.4
Severity: Normal
Type: Bug

comment:17 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:2 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:3 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:4 by Kit Sunde, 12 years ago

Cc: kitsunde@… added
Note: See TracTickets for help on using tickets.
Back to Top