Opened 6 years ago

Last modified 3 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 5 years ago.
12627_r12827_with_test.diff (3.3 KB) - added by copelco 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by tin_nqn

  • Owner changed from nobody to tin_nqn
  • Status changed from new to assigned

comment:3 Changed 5 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 5 years ago by tin_nqn

  • Owner changed from tin_nqn to nobody
  • Status changed from assigned to new

comment:5 Changed 5 years ago by gonzalolarralde

  • Owner changed from nobody to gonzalolarralde
  • Status changed from new to assigned

comment:6 Changed 5 years ago by NicoEchaniz

  • Owner changed from gonzalolarralde to NicoEchaniz
  • Status changed from assigned to new

Changed 5 years ago by NicoEchaniz

comment:7 Changed 5 years ago by NicoEchaniz

  • Has patch set
  • Needs tests set
  • Status changed from new to 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 Changed 5 years ago by copelco

  • Owner changed from NicoEchaniz to copelco
  • Status changed from assigned to new

comment:9 Changed 5 years ago by copelco

  • Status changed from new to assigned

Changed 5 years ago by copelco

comment:10 Changed 5 years ago by kmtracey

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

  • Owner changed from copelco to anonymous
  • Status changed from assigned to new

comment:13 Changed 5 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 5 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 5 years ago by russellm

  • Has patch unset
  • milestone changed from 1.2 to 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 Changed 4 years ago by mattmcc

  • milestone changed from 1.3 to 1.4
  • Severity set to Normal
  • Type set to Bug

comment:17 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:2 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:3 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:4 Changed 3 years ago by kitsunde

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