Opened 15 years ago

Closed 7 months ago

#10808 closed Bug (invalid)

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

Reported by: mikemintz Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: shaun@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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

Attachments (6)

base.py.2.diff (847 bytes ) - added by jianhuang 15 years ago.
fixed the init bug with diamond inheritance, keeps track of pre-assigned fields with a set instead of a dict
base.py.diff (1.3 KB ) - added by jianhuang 15 years ago.
patch which fixed the diamond inheritance init bug, but with comments
model_inheritance.py.diff (1.2 KB ) - added by jianhuang 15 years ago.
added a test of this patch--very simplified example along the lines mikemintz had
10808.diff (2.5 KB ) - added by Chris Beaven 15 years ago.
Single patch with tests & changes
10808b.diff (5.1 KB ) - added by Luc Saffre 15 years ago.
contains 10808.diff + my suggestion
10808b-14404.diff (6.0 KB ) - added by Luc Saffre 13 years ago.
10808b against rev 14404 with tests converted to unittest

Download all attachments as: .zip

Change History (28)

comment:1 by mikemintz, 15 years ago

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.

comment:2 by jianhuang, 15 years ago

Owner: changed from nobody to jianhuang
Status: newassigned

comment:3 by Alex Gaynor, 15 years ago

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

comment:4 by Alex Gaynor, 15 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

by jianhuang, 15 years ago

Attachment: base.py.2.diff added

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

comment:5 by mikemintz, 15 years ago

Patch needs improvement: unset

comment:6 by Chris Beaven, 15 years ago

Triage Stage: UnreviewedAccepted

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.

by jianhuang, 15 years ago

Attachment: base.py.diff added

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

by jianhuang, 15 years ago

Attachment: model_inheritance.py.diff added

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

comment:7 by mikemintz, 15 years ago

Needs tests: unset

by Chris Beaven, 15 years ago

Attachment: 10808.diff added

Single patch with tests & changes

comment:8 by Chris Beaven, 15 years ago

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?

comment:9 by mikemintz, 15 years ago

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

comment:10 by Chris Beaven, 15 years ago

Triage Stage: AcceptedReady for checkin

Yeah, fair enough

by Luc Saffre, 15 years ago

Attachment: 10808b.diff added

contains 10808.diff + my suggestion

comment:11 by Luc Saffre, 15 years ago

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.

comment:12 by Shaun Cutts, 14 years ago

Cc: shaun@… added

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.

comment:13 by Shaun Cutts, 14 years ago

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.

comment:14 by Malcolm Tredinnick, 14 years ago

Owner: changed from jianhuang to Malcolm Tredinnick
Status: assignednew
Triage Stage: Ready for checkinAccepted

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

by Luc Saffre, 13 years ago

Attachment: 10808b-14404.diff added

10808b against rev 14404 with tests converted to unittest

comment:15 by Chris Beaven, 13 years ago

Severity: Normal
Type: Bug

comment:16 by patchhammer, 13 years ago

Easy pickings: unset
Patch needs improvement: set

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

comment:17 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:18 by Luc Saffre, 12 years ago

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.

Last edited 12 years ago by Luc Saffre (previous) (diff)

comment:19 by Tim Graham, 9 years ago

Closed #17232 as a duplicate.

comment:20 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed

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

comment:21 by Alex Es, 7 months ago

Resolution: invalid
Status: closednew

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.

comment:22 by Mariusz Felisiak, 7 months ago

Resolution: invalid
Status: newclosed

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

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