Multiple inheritance (model-based) broken for __init__ of common fields in diamond inheritance

[mikemintz]

I am using the latest version of Django SVN r10558.

My models are:

class Restaurant(models.Model):
    name = models.CharField(max_length=255)

class Bar(Restaurant):
    min_age = models.IntegerField()

class Pizzeria(Restaurant):
    specialty = models.CharField(max_length=255)

class PizzeriaBar(Bar, Pizzeria):
    pizza_bar_specific_field = models.CharField(max_length=255)

This yields the following inheritance diagram:

    Model
      |
  Restaurant
 /         \
Bar     Pizzeria
 \         /
 BarPizzeria

My problem is that setting the name field in PizzeriaBar.__init__ does not work.

>>> p = PizzeriaBar(name="Michaels", min_age=21, specialty="Cheese", pizza_bar_specific_field="Doodle")
>>> print (p.name, p.min_age, p.specialty, p.pizza_bar_specific_field)
('', 21, 'Cheese', 'Doodle')

>>> p = PizzeriaBar()
>>> p.name = "Michaels"
>>> p.min_age = 21
>>> p.specialty = "Cheese"
>>> p.pizza_bar_specific_field = "Doodle"
>>> print (p.name, p.min_age, p.specialty, p.pizza_bar_specific_field)
('Michaels', 21, 'Cheese', 'Doodle')

So this problem only comes up when you are using multiple inheritance, and you set a field defined in a shared superclass of the current model's superclasses (i.e., diamond inheritance). I know diamond inheritance isn't great to use, but it's useful in my application (in which most models inherit from a top-level Item model), and since it works when using setattr, it seems like it should be possible to work in __init__.

[mikemintz]

It looks like the reason this is happening is in the "for field in fields_iter:" loop around line 224 for db/models/base.py. We iterate through the fields in order, but "name" is defined twice in the fields, so the second time it's reached, the argument has been popped from kwargs, and we set it back to field.get_default().

This could be easily solved by having a local set of "set_fields" in __init__. Each time we call setattr on field.attname, we add field.attname to the set (and don't call setattr on it again if it's been set already).

If this sounds like a reasonable solution, I can write a patch.

[Alex Gaynor]

This patch should use the set() datatype, rather than a dictionary with keys who map to nothing.

[jianhuang]

fixed the init bug with diamond inheritance, keeps track of pre-assigned fields with a set instead of a dict

[Chris Beaven]

I'd suggest some inline comments explaining why the code is doing what it's doing. fields = set() is enough (rather than set([]))

Add some tests and it's ready to go in.

[jianhuang]

patch which fixed the diamond inheritance init bug, but with comments

[jianhuang]

added a test of this patch--very simplified example along the lines mikemintz had

[Chris Beaven]

Single patch with tests & changes

[Chris Beaven]

Tested and works as expected.

I have one last question before promoting: this patch always uses field.attname, but the code preceding the addition of this field to the assigned_fields set uses field.name if it is a related object. Should this affect what gets added to assigned_fields?

[mikemintz]

I think it would work either with field.name or field.attname, since there will always be a one-to-one correspondence. I.e., it shouldn't matter if our set contains "user_id" or "user", as long as we're consistent with "assigned_fields.add" and "in assigned_fields".

[Chris Beaven]

Yeah, fair enough

[Luc Saffre]

contains 10808.diff + my suggestion

[Luc Saffre]

Patch 10808.diff fixes only one symptom, not the real problem.
The real problem is that in case of diamond inheritance there are duplicate field definitions.
My patch 10808b.diff won't fix the real problem, but another symptom: when the top-level model of your diamond structure contains a ForeignKey, then you get problems when trying to create inline formsets. To show this problem I added a "Owner" class to SmileyChris's diamond inheritance test. inlineformset_factory() gets disturbed because it thinks that PizzeriaBar contains more than one foreign key to Owner. The workaround is to specify the fk_name explicitly when calling inlineformset_factory(). Unfortunately this workaround needs another patch because inlineformset_factory() just can't imagine that a model can have two fields with the same name.
Patch 10808b.diff contains 10808.diff + my suggestion.

[Shaun Cutts]

It seems to me that the "real underlying problem" is that Options (in source:trunk/django/db/models/options.py) doesn't have anything like the "method resolution order" (mro) list that python uses to linearize name resolution. The ORM should probably mimic python in resolving this problem.

This may be the underlying problem in #13781 as well. As Russellm points out, multiple inheritance support is claimed, but not necessarily thought through completely, yet.

[Shaun Cutts]

Note that this problem doesn't just come up in "diamond inheritance" ... it seems to be a problem anywhere there is field shadowing. If you inherit from A and B, they will both have "id" -- see #12002, also. This patch works for me, but I think a more thorough solution would be to change Options to collect only one copy of each field.

[Malcolm Tredinnick]

I'm going to punt this back to accepted for a little bit. Not ready to apply the patch yet. It looks correct, but I want to think about if there's an MRO-related way to do this. Currently, the meta class doesn't store any mro information in the parents list, because we don't need it and usually just need to know if something exists somewhere in one parent. However, there are some cases where MRO-ordering is starting to become important, so it's time to revisit this. It's pretty hairy in there, so this isn't going to be a simple change (necessarily).

[Luc Saffre]

10808b against rev 14404 with tests converted to unittest

[patchhammer]

10808b-14404.diff fails to apply cleanly on to trunk

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Luc Saffre]

The following workaround uses the class_prepared signal to remove any duplicate fields from _meta._field_cache:

def on_class_prepared(signal,sender=None,**kw):
    cache = []
    field_names = set()
    for f,m in model._meta._field_cache:
        if f.attname not in field_names:
            field_names.add(f.attname)
            cache.append( (f,m) )
    model._meta._field_cache = tuple(cache)
    model._meta._field_name_cache = [x for x, _ in cache]
            
models.signals.class_prepared.connect(on_class_prepared)

Works for me and has the advantage of not needing to patch Django.

[Tim Graham]

Closed #17232 as a duplicate.

[Tim Graham]

Closing as invalid because the provided models don't validate:

PizzeriaBar: (models.E005) The field 'restaurant_ptr' from parent model 'polls.bar' clashes with the field 'restaurant_ptr' from parent model 'polls.pizzeria'.

[Alex Es]

I'm experiencing the bug with trying to inherit from wagtail's Page and Collection models.

Closing the ticket about the models being invalid because provided models were invalid doesn't look like a sound decision, so reopening.

[Mariusz Felisiak]

Please don't reopen old tickets without providing new details. We cannot accept or investigate reports based on model definitions that are not supported.