ModelForm inheritance logic is broken
|Reported by:||semenov||Owned by:||nobody|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||yes||Patch needs improvement:||no|
A simple example:
class TicketForm(forms.ModelForm): class Meta: model = Ticket fields = ('priority','summary') class EditTicketForm(TicketForm): pass
Supposedly, the program behaviour generally should not change if TicketForm usage is replaced with EditTicketForm. However, it changes in a very unexpected way (instance saving still works fine, but the "fields" filter is gone!). Actually it brings some more "gotchas", but they're harder to explain with a simple example.
When I looked closer, I discovered that django.newforms.models.ModelFormMetaclass is not very well thought when it comes to the inheritance.
First, regarding the simple problem mentioned above, it says:
class ModelFormMetaclass(type): ... opts = ModelFormOptions(attrs.get('Meta', None)) attrs['_meta'] = opts
It can be easily seen that for EditTicketForm, as there's no Meta in it, opts becomes "empty" ModelFormOptions with model=None, fields=None, and exclude=None. Therefore, the further save() call doesn't use proper "fields" filter, since it is lost in the parent class'es Meta.
Another related issue is that ModelFormMetaclass (according to the code and comments) is supposed to allow only one base class with the defined Meta attribute. I can see the rationale behind this limitation - it wouldn't be easy to deal with several Metas with contradictory fields= or exclude=. However, the current implementation is broken in that case, as it actually allows to have two Metas (one in the base classes, and one in the child class), with unpredictable results. We need either to disallow that completely, or come up with some logic which handles these cases accurately.
The proposed patch fixes the arisen problems. With inherited Metas, the following algorithm is imposed:
- If a child class defines a Meta class, it is taken as an argument to ModelFormOptions.
- If a child class doesn't define a Meta, it is taken from its parents. The parents are obliged to define no more than one Meta.
- If neither a child class nor its parents define any Meta class, an "empty" ModelFormOptions is created.
Also, I extracted a function from DeclarativeFieldsMetaclass to reuse it (and get rid of copy-pasted code) in ModelFormMetaclass.
The patch includes documentation describing the proposed behaviour (previously, the model form inheritance was not covered in the documentation at all).
Change History (8)
Changed 6 years ago by semenov
comment:1 Changed 6 years ago by semenov
- Has patch set
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
comment:6 Changed 6 years ago by mtredinnick
- Resolution set to fixed
- Status changed from new to closed