Code

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6337 closed (fixed)

ModelForm inheritance logic is broken

Reported by: semenov Owned by: nobody
Component: Forms Version: master
Severity: Keywords: ModelForm inheritance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

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

Attachments (1)

ModelFormMetaclass-inheritance.diff (6.9 KB) - added by semenov 6 years ago.

Download all attachments as: .zip

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:2 Changed 6 years ago by brosner

  • Needs tests set

I agree with these points. In my patch on #6241 I was running into a similar
problem and just worked around it. The ModelFormMetaclass` needs to be
made more flexible with the Meta class. I also have a use-case where the meta
options are needed to be passed into the __new__ method so I would
take that into consideration with how you are building the options.

I would also suggest as opposed to using get_declared_fields that
ModelFormMetaclass just inherits from DeclarativeFieldsMetaclass.

This patch will also need tests to ensure this functionality.

Being the maintainer of the #6241 patch work that this should be looked at
and resolved. Thanks for submitting the patch semenov!

comment:3 Changed 6 years ago by semenov

The ModelFormMetaclass needs to be made more flexible with the Meta class.

I must be missing something. Do you suggest having a Meta class for a Meta class? Sounds overcomplicated. What problem does that suppose to solve?

You see, I don't think we should cover several ModelForm's problem with a single ticket. That gives smaller chance for it to get to the trunk :)

I described a specific problem with inheritence and solved it with a proper patch which doesn't bring much complexity to the code (actually I think the code became simplier).

I would also suggest as opposed to using get_declared_fields that ModelFormMetaclass just inherits from DeclarativeFieldsMetaclass.

I don't think it's possible to just inherit DeclarativeFieldsMetaclass, since __new__ method there is static (and there's no self to access the parent).

This patch will also need tests to ensure this functionality.

Sure, will add the tests.

comment:4 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

I'll trust brosner that this is a real issue.

comment:5 Changed 6 years ago by mtredinnick

#6241 really is a separate issue to this patch and maybe it will add further changes or maybe not. In any case, this is a problem now, regardless of formsets and the patch here is a reasonable holistic approach to fixing the problem. I'm going to apply a variation on this patch for now and we can thrash out formsets in the other ticket as an independent issue. It's not worth blocking this for the other ticket at the moment.

comment:6 Changed 6 years ago by mtredinnick

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

(In [7112]) Fixed #6337. Refs #3632 -- Fixed ModelForms subclassing, to the extent that it can be made to work.

This ended up being an almost complete rewrite of ModelForms.new, but
should be backwards compatible (although the text of one error message has
changed, which is only user visible and only if you pass in invalid code).

Documentation updated, also.

This started out as a patch from semenov (many thanks!), but by the time all
the problems were hammered out, little of the original was left. Still, it was
a good starting point.

comment:7 Changed 6 years ago by mtredinnick

This was further modified in [7115], but I forgot to include the ticket reference in the commit message. Inheritance now behaves "naturally" when Meta is subclassed.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.