Code

Opened 5 years ago

Last modified 23 months ago

#10808 new Bug

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

Reported by: mikemintz Owned by: mtredinnick
Component: Database layer (models, ORM) Version: master
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 5 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 5 years ago.
patch which fixed the diamond inheritance init bug, but with comments
model_inheritance.py.diff (1.2 KB) - added by jianhuang 5 years ago.
added a test of this patch--very simplified example along the lines mikemintz had
10808.diff (2.5 KB) - added by SmileyChris 5 years ago.
Single patch with tests & changes
10808b.diff (5.1 KB) - added by lsaffre 5 years ago.
contains 10808.diff + my suggestion
10808b-14404.diff (6.0 KB) - added by lsaffre 4 years ago.
10808b against rev 14404 with tests converted to unittest

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by mikemintz

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 5 years ago by jianhuang

  • Owner changed from nobody to jianhuang
  • Status changed from new to assigned

comment:3 Changed 5 years ago by Alex

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

comment:4 Changed 5 years ago by Alex

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

Changed 5 years ago by jianhuang

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

comment:5 Changed 5 years ago by mikemintz

  • Patch needs improvement unset

comment:6 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 5 years ago by jianhuang

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

Changed 5 years ago by jianhuang

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

comment:7 Changed 5 years ago by mikemintz

  • Needs tests unset

Changed 5 years ago by SmileyChris

Single patch with tests & changes

comment:8 Changed 5 years ago by SmileyChris

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 Changed 5 years ago by 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".

comment:10 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

Yeah, fair enough

Changed 5 years ago by lsaffre

contains 10808.diff + my suggestion

comment:11 Changed 5 years ago by lsaffre

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 Changed 4 years ago by shauncutts

  • 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 Changed 4 years ago by shauncutts

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 Changed 4 years ago by mtredinnick

  • Owner changed from jianhuang to mtredinnick
  • Status changed from assigned to new
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 4 years ago by lsaffre

10808b against rev 14404 with tests converted to unittest

comment:15 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:16 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

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

comment:17 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:18 Changed 23 months ago by lsaffre

The following workaround uses the class_prepared signal to removes 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.

Version 0, edited 23 months ago by lsaffre (next)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from mtredinnick to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.