Opened 12 years ago

Closed 9 years ago

#8999 closed Cleanup/optimization (fixed)

ModelAdmin.get_form and get_formset will overwrite self.exclude with a kwargs exclude

Reported by: Sean Legassick Owned by: jkocherhans
Component: contrib.admin Version: 1.0
Severity: Normal Keywords:
Cc: brandon.konkle@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


In ModelAdmin.get_form and ModelAdmin.get_formset the intent is to combine self.exclude and kwargs['exclude'] but the defaults.update(kwargs) will overwrite the combined values. Fix is simply changing the kwargs.get('exclude', []) to kwargs.pop('exclude', []).

Attachments (5)

modeladmin-getform-and-getformset-exclude-fix.diff (928 bytes) - added by Sean Legassick 12 years ago.
modeladmin-getform-and-getformset-exclude-fix-2.diff (1.3 KB) - added by Sean Legassick 12 years ago.
modeladmin-getform-and-getformset-exclude-fix-with-tests.diff (4.6 KB) - added by Sean Legassick 12 years ago.
modeladmin-getform-and-getformset-exclude-will-overwrite.diff (2.2 KB) - added by Brandon Konkle 10 years ago.
8999.getform-exclude-kwarg.diff (3.3 KB) - added by Julien Phalip 9 years ago.

Download all attachments as: .zip

Change History (19)

Changed 12 years ago by Sean Legassick

Changed 12 years ago by Sean Legassick

comment:1 Changed 12 years ago by Sean Legassick

Patch modified to coerce self.exclude to list before concatenation, so any iterable can be used in the ModelAdmin subclass parameters.

comment:2 Changed 12 years ago by Brian Rosner

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 12 years ago by Sean Legassick

Needs tests: unset

added regression tests for bugfixes

comment:4 Changed 11 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:5 Changed 11 years ago by jkocherhans

Owner: changed from nobody to jkocherhans
Status: newassigned

I don't think we should be combining anything here. If someone passes exclude to get_form, that value should override self.exclude. If someone wants different behavior, they can override the get_form method. I need to think about this a bit more in the context of a few other related issues though.

comment:6 Changed 11 years ago by Sean Legassick

OK, that also makes sense as behaviour, but the current code looks like it intends to combine the values, but due to a subtlety kwargs takes precedence. (that the 'exclude' kwarg if present is left in the kwargs dict, and thus overrides when you come to defaults.update(kwargs) ). If the current behaviour is what is intended, then the whole 'excludes' line in the defaults dictionary literal should be removed, and the behaviour will be the same, but with the code reflecting the intent.

comment:7 Changed 11 years ago by jkocherhans

Yeah, the apparent intention of the existing code is the wrong thing to do IMO. Just look at what a subtle bug it produced! ;)

comment:8 Changed 10 years ago by Brandon Konkle

Cc: brandon.konkle@… added

I just ran into this bug, myself. If the 'exclude' kwarg is intended to override self.exclude, then exclude.extend(kwargs.get("exclude", [])) should not be called. This implies that the kwarg will extend self.exclude.

My use case here is an ModelAdmin object where I'm overriding get_form to exclude two fields if this is a change as opposed to a create action. My ModelAdmin subclass has readonly_fields defined, which by default extend self.exclude in the ModelAdmin class's get_form method. I'm doing:

        if obj:
            kwargs['exclude'] = ['field', 'other_field']
        return super(MySubclass, self).get_form(request, obj,

The end result is that the readonly_fields I have defined on my subclass get presented twice in the rendered admin view, since defaultsexclude? is overridden by the 'exclude' kwarg.

It's easy to fix, of course, but the existing code in ModelAdmin.get_form is confusing.

comment:9 Changed 9 years ago by Julien Phalip

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: Bug

The tests would need to be rewritten using unittests since this is now Django's preferred way.

Changed 9 years ago by Julien Phalip

comment:10 Changed 9 years ago by Julien Phalip

Patch needs improvement: unset
Type: BugCleanup/optimization

I agree with jkocherhans that the exclude kwarg should completely override any other declarations rather than being combined to them. So, the current behaviour is actually correct.

The line "exclude.extend(kwargs.get("exclude", []))", which seems to suggest that the kwarg should be combined, is actually useless and it can be safely removed. The attached patch removes that line and verifies the current behaviour both for ModelAdmin.get_form() and InlineAdmin.get_formset().

comment:11 Changed 9 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

LGTM, although it needs a release note addition.

comment:12 Changed 9 years ago by Jannis Leidel

Actually, the test case could probably be split in two methods for further isolation.

comment:13 Changed 9 years ago by Julien Phalip

Thanks Jannis. I don't think it needs a release note. The removed lines were actually useless. If exclude is passed via get_form()'s kwargs, then it is first combined to ModelAdmin.exclude but a bit further it overrides it anyway via defaults.update(kwargs).

The provided tests verify the current behaviour and they pass whether those two lines are present or not. This is basically just cleaning up some stale code. No one could have possibly relied on a different behaviour.

comment:14 Changed 9 years ago by Ramiro Morales

Resolution: fixed
Status: assignedclosed

In [16422]:

Fixed #8999 -- Removed useless code in handling of exclude option in ModelAdmin and InlineModelAdmin custom form(set) hooks. Thanks goes to seanl for the report, patch and bkonkle and Julien for further work on final patch.

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