Opened 3 years ago

Closed 2 years ago

#19445 closed Bug (fixed)

Fieldsets' validation doesn't call get_form

Reported by: KJ Owned by: slurms
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In django.contrib.admin.validation, function check_formfield checks presence of a field based on "form" attribute on admin class. If get_form method is overriden and it returns form class which adds some additional fields, those cannot be used in "fieldsets" attribute because ImproperlyConfigured exception is raised.

The workaround is to use undocumented get_fieldsets method to specify additional fields instead of "fieldsets" attribute.

The easiest way to fix this would be probably to disable fieldsets' validation entirely (as we are not able to anticipate what will be the result of calling get_form) or to make get_fieldsets method a documented one and make a note that if we add fields in get_form, then we should not set "fieldsets" attribute.

Change History (10)

comment:1 Changed 3 years ago by russellm

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

comment:2 Changed 3 years ago by russellm

To be clear -- both the proposed solutions (documenting get_fieldsets(), and removing validation if get_form is defined) are worth implementing.

comment:3 Changed 3 years ago by slurms

  • Has patch set
  • Owner changed from nobody to slurms
  • Status changed from new to assigned

There was a pull request for this, but it was missing disabling validation when get_form is defined on the sub class of ModelAdmin. I've added a small fix for this and a test and opened a pull request here: https://github.com/django/django/pull/636.

comment:4 Changed 2 years ago by Julien Phalip <jphalip@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 0694d2196f0fadde37ff2d002a9a4a8edb3ca504:

Fixed #19445 -- Skip admin fieldsets validation when the ModelAdmin.get_form() method is overridden.

comment:5 Changed 2 years ago by Julien Phalip <jphalip@…>

In e18bd68dbc64dfe08ae0fbb54591d5c8f7e456eb:

[1.5.x] Fixed #19445 -- Skip admin fieldsets validation when the ModelAdmin.get_form() method is overridden.
Backport of 0694d2196f0fad

comment:6 Changed 2 years ago by Julien Phalip <jphalip@…>

In e18bd68dbc64dfe08ae0fbb54591d5c8f7e456eb:

[1.5.x] Fixed #19445 -- Skip admin fieldsets validation when the ModelAdmin.get_form() method is overridden.
Backport of 0694d2196f0fad

comment:7 Changed 2 years ago by lukeplant

  • Resolution fixed deleted
  • Status changed from closed to new

This fix is, unfortunately, inadequate.

Even if it isn't overridden, get_form will return a ModelForm class that can be significantly different from the ModelAdmin.form attribute.

For example, the following code will work fine if you disable validation, but validation will fail:

class MyModel(models.Model):
    field1 = models.BooleanField()

class MyForm(forms.ModelForm):
    my_additional_field = forms.BooleanField()

class MyAdmin(admin.ModelAdmin):
    form = MyForm
    fieldsets = (
        ('Group', {
              'fields': ['field1', 'my_additional_field']
          }),
    )

This is because get_form creates a class based on MyForm, but not the same - it adds an inner Meta class, so it has a base_fields attribute that will work, while the statically defined one won't pass validation.

We can't even call get_form with a dummy request object in the validation method, because it could call an overridden get_readonly_fields which assumes a real request object.

And there are more bugs with the validation routine - for example if get_readonly_fields dynamically returns fields that don't exist, the validate routine won't catch it.

Basically, I don't think we can do any static validation of ModelAdmin.fieldsets etc. against the form, because we don't know what form will actually be used, because it is defined and generated dynamically, not statically. I can't see a way of fixing this.

BTW, I'm hitting this for my work on #19733.

comment:9 Changed 2 years ago by Luke Plant <L.Plant.98@…>

In 1556b1c3b7a18cadf4d66d2ab6301d7fe8646ba5:

Removed fragile admin validation of fields on ModelForm

Refs #19445

comment:10 Changed 2 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

This ticket should probably be considered closed now, although it is 'fixed' by skipping the validation, and just providing a more helpful error message later down the line if necessary.

Note: See TracTickets for help on using tickets.
Back to Top