[Django] #17232: Deadly Diamond of Death

Django

unread,

Nov 14, 2011, 8:34:00 PM11/14/11

to django-...@googlegroups.com

#17232: Deadly Diamond of Death
-------------------------------------+-------------------------------------
Reporter: rich.harkins@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.3
(models, ORM) | Keywords: multiple shared
Severity: Normal | inheritance
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I ask you to consider the following found in 1.3.1 ("1.3.1" wasn't in the
Version dropdown at time of submission, BTW):

{{{
from django.db import models
import uuid

# Create your models here.
class Tag(models.Model):
tag_name = models.CharField(max_length=32)
uuid = models.CharField(max_length=36, unique=True,
default=uuid.uuid4)

class IdentifiableTag(Tag):
html_id = models.CharField(max_length=64, null=True)

class LinkableTag(Tag):
url = models.URLField(verify_exists=False, null=True)

class AnchorTag(IdentifiableTag, LinkableTag):
name = models.CharField(max_length=32, null=True)

def save(self, **_kw):
self.tag_name = 'a'
super(AnchorTag, self).save(**_kw)
}}}

In essence, an Anchor is a linkable, identifiable tag. Let's not get too
worked up about the semantics (yes, all tags could have an id attribute)
-- I just needed a sanitary illustration.

The following test case will work:

{{{
from django.test import TestCase
from models import *

class DDDTests(TestCase):
def test_base_tag(self):
tag_uuid = uuid.uuid4()
tag = Tag(tag_name='something', uuid=tag_uuid)
self.assertEqual(tag.uuid, tag_uuid)
self.assertEqual(tag.tag_name, 'something')

def test_id_tag(self):
tag_uuid = uuid.uuid4()
tag = IdentifiableTag(tag_name='something',
uuid=tag_uuid,
html_id='blah')
self.assertEqual(tag.uuid, tag_uuid)
self.assertEqual(tag.tag_name, 'something')
self.assertEqual(tag.html_id, 'blah')

def test_link_tag(self):
tag_uuid = uuid.uuid4()
tag = LinkableTag(tag_name='something',
uuid=tag_uuid,
url='http://blah.com/')
self.assertEqual(tag.uuid, tag_uuid)
self.assertEqual(tag.tag_name, 'something')
self.assertEqual(tag.url, 'http://blah.com/')
}}}

Everything is hunky-dory. No worries here, everything as you'd expect.
Now let's look at the following extra test method:

{{{
def test_anchor_tag(self):
tag_uuid = uuid.uuid4()
tag = AnchorTag(name='hello',
uuid=tag_uuid,
html_id='world',
url='http://blah.com/')
self.assertEqual(tag.uuid, tag_uuid)
self.assertEqual(tag.name, 'a')
self.assertEqual(tag.html_id, 'world')
self.assertEqual(tag.url, 'http://blah.com/')
}}}

If you think this should pass, I'd agree with you. However, if run:

{{{
Creating test database for alias 'default'...
F...
======================================================================
FAIL: test_anchor_tag (ddd.tests.DDDTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/tmp/show_ddd/ddd/tests.py", line 42, in test_anchor_tag
self.assertEqual(tag.uuid, tag_uuid)
AssertionError: UUID('fff54d5b-ee37-48d8-a091-dc4aaf88770a') !=
UUID('beff99f9-f9b7-4768-b8b2-259f9b0922c5')

----------------------------------------------------------------------
Ran 4 tests in 0.001s

FAILED (failures=1)
}}}

So why is this? Well, I traced the Django code and found something
peculiar with the _meta.fields of AnchorTag:

{{{
In [1]: from ddd.models import *

In [2]: [field.name for field in Tag._meta.fields]
Out[2]: ['id', 'tag_name', 'uuid']

In [3]: [field.name for field in IdentifiableTag._meta.fields]
Out[3]: ['id', 'tag_name', 'uuid', 'tag_ptr', 'html_id']

In [4]: [field.name for field in LinkableTag._meta.fields]
Out[4]: ['id', 'tag_name', 'uuid', 'tag_ptr', 'url']

In [5]: [field.name for field in AnchorTag._meta.fields]
Out[5]:
['id',
'tag_name',
'uuid',
'tag_ptr',
'html_id',
'id',
'tag_name',
'uuid',
'tag_ptr',
'url',
'linkabletag_ptr',
'identifiabletag_ptr',
'name']
}}}

The issue seems to be that 'uuid', 'id', and 'tag_name' are represented
twice due to the shared inheritance path. This causes Model.__init__ to
process uuid twice (popping it off the kwargs the first time and thinking
it should use the default the second, triggering a new uuid.uuid4() call).
This stems from the logic in Options._fill_fields_cache() not checking for
duplicate fields from the parents. My feeble attempt at a correction is:

{{{
def _fill_fields_cache(self):
cache = []
for parent in self.parents:
for field, model in parent._meta.get_fields_with_model():
if model:
cache.append((field, model))
else:
cache.append((field, parent))
cache.extend([(f, None) for f in self.local_fields])
self._field_cache = tuple(cache)
# My feeble attempt
name_cache = []
for field, _ in cache:
if field not in name_cache:
name_cache.append(field)
self._field_name_cache = name_cache
# Old line
#self._field_name_cache = [x for x, _ in cache]
}}}

This seems to work and passes all the unit tests above. However, I could
just be "Doing it wrong" (TM), so I would also appreciate advice in
finding another alternative if that is the case.

--
Ticket URL: <https://code.djangoproject.com/ticket/17232>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Nov 15, 2011, 3:14:38 AM11/15/11

to django-...@googlegroups.com

#17232: Multitable multi-inheritance: Deadly Diamond of Death

-------------------------------------+-------------------------------------
Reporter: rich.harkins@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.3

* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted

Comment:

A quick question: Wouldn't this result in self._field_cache still having
the duplicate fields, it is just the _field_name_cache which is correct
now? You could also assert there aren't duplicate field attnames from
_different_ models. In multitable multi-inheritance having id-field in
more than one parent is an error, for example. Or maybe that should be
done in model validation?

--
Ticket URL: <https://code.djangoproject.com/ticket/17232#comment:1>

Django

unread,

Nov 15, 2011, 10:20:45 AM11/15/11

to django-...@googlegroups.com

#17232: Multitable multi-inheritance: Deadly Diamond of Death

-------------------------------------+-------------------------------------
Reporter: rich.harkins@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.3

Comment (by hrharkins):

I'm not sure about the assertion -- other than the "id" field, which is
true should be duplicated for each level, the other "naked" fields would
seem to imply only one representation in a shared base class. I'm not
sure exactly how AutoFields deal with single inheritance otherwise -- they
are potentially multiple "id" fields stacked on different classes.

This raises a different question though (not a problem in my example or my
current project) -- suppose an external agent claims an ID of a subclass
that the base class would have wanted? Could this not cause a consistency
issue?

The correction suggested was purely out of nativity and trying to
understand the nature of the problem and a possible fix, so I am
definitely not implying that the above is likely to be a complete fix (it
is a feeble attempt after all :D ). Perhaps a better one would be:

{{{#!python

def _fill_fields_cache(self):
cache = []
for parent in self.parents:
for field, model in parent._meta.get_fields_with_model():
if model:
cache.append((field, model))
else:
cache.append((field, parent))
cache.extend([(f, None) for f in self.local_fields])

# Original line
#self._field_cache = tuple(cache)

# Second feeble attempt to avoid duplicates
recache = []
for field in cache:
if field not in recache:
recache.append(field)
self._field_cache = tuple(recache)

self._field_name_cache = [x for x, _ in cache]
}}}

This passes the unit tests above again, but I suspect there are other
subtleties out there to be dealt with too.

--
Ticket URL: <https://code.djangoproject.com/ticket/17232#comment:2>

Django

unread,

Nov 15, 2011, 10:37:35 AM11/15/11

to django-...@googlegroups.com

#17232: Multitable multi-inheritance: Deadly Diamond of Death

-------------------------------------+-------------------------------------
Reporter: rich.harkins@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.3

Comment (by hrharkins):

(Sorry about the cut-and-paste issues on the last comment -- the version
shown is consistent with my version that passes tests)

--
Ticket URL: <https://code.djangoproject.com/ticket/17232#comment:3>

Django

unread,

Aug 6, 2015, 1:03:02 PM8/6/15

to django-...@googlegroups.com

#17232: Multitable multi-inheritance: Deadly Diamond of Death

-------------------------------------+-------------------------------------
Reporter: rich.harkins@… | Owner: nobody

Type: Bug | Status: closed

Component: Database layer | Version: 1.3
(models, ORM) |

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------