Multitable multi-inheritance: Deadly Diamond of Death

[rich.harkins@…]

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.

[Anssi Kääriäinen]

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?

[hrharkins]

[Edit: My paste buffer had the wrong version of the second feeble attempt -- I've updated it]

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:

    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 lines
        #self._field_cache = tuple(cache)
        #self._field_name_cache = [x for x, _ in cache]

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

        self._field_name_cache = [x for x, _ in recache]

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

[hrharkins]

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

[Tim Graham]

Looks like a duplicate of #10808.