Opened 7 years ago

Last modified 4 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: master
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 7 years ago.
12627_r12827_with_test.diff (3.3 KB) - added by Colin Copeland 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by tin_nqn

Owner: changed from nobody to tin_nqn
Status: newassigned

comment:3 Changed 7 years ago by tin_nqn

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 Changed 7 years ago by tin_nqn

Owner: changed from tin_nqn to nobody
Status: assignednew

comment:5 Changed 7 years ago by gonzalolarralde

Owner: changed from nobody to gonzalolarralde
Status: newassigned

comment:6 Changed 7 years ago by NicoEchaniz

Owner: changed from gonzalolarralde to NicoEchaniz
Status: assignednew

Changed 7 years ago by NicoEchaniz

Attachment: 12627.diff added

comment:7 Changed 7 years ago by NicoEchaniz

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 Changed 7 years ago by Colin Copeland

Owner: changed from NicoEchaniz to Colin Copeland
Status: assignednew

comment:9 Changed 7 years ago by Colin Copeland

Status: newassigned

Changed 7 years ago by Colin Copeland

Attachment: 12627_r12827_with_test.diff added

comment:10 Changed 7 years ago by Karen Tracey

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 Changed 7 years ago by NicoEchaniz

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 Changed 7 years ago by anonymous

Owner: changed from Colin Copeland to anonymous
Status: assignednew

comment:13 Changed 7 years ago by tomevans222

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 Changed 7 years ago by Jacob

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 Changed 7 years ago by Russell Keith-Magee

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 Changed 6 years ago by Matt McClanahan

milestone: 1.31.4
Severity: Normal
Type: Bug

comment:17 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:2 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:3 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:4 Changed 4 years ago by Kit Sunde

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