Opened 15 years ago
Closed 13 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)
Change History (28)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 15 years ago
This patch should use the set() datatype, rather than a dictionary with keys who map to nothing.
comment:4 by , 15 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
by , 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 , 15 years ago
Patch needs improvement: | unset |
---|
comment:6 by , 15 years ago
Triage Stage: | Unreviewed → 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.
by , 15 years ago
Attachment: | base.py.diff added |
---|
patch which fixed the diamond inheritance init bug, but with comments
by , 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 , 15 years ago
Needs tests: | unset |
---|
comment:8 by , 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 , 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:11 by , 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 , 14 years ago
Cc: | 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 , 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 , 14 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Triage Stage: | Ready for checkin → 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).
by , 14 years ago
Attachment: | 10808b-14404.diff added |
---|
10808b against rev 14404 with tests converted to unittest
comment:15 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:16 by , 13 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
10808b-14404.diff fails to apply cleanly on to trunk
comment:18 by , 12 years ago
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.
comment:20 by , 9 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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 , 13 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
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 , 13 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
Please don't reopen old tickets without providing new details. We cannot accept or investigate reports based on model definitions that are not supported.
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.