If all fields are readonly or editable=False, their ModelForm is always valid and can raise exceptions

[KyleMac]

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.

[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

[NicoEchaniz]

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.

[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?

[NicoEchaniz]

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?

[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.

[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.

[Russell Keith-Magee]

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.

[Jacob]

Milestone 1.4 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.