@jarekwg @carltongibson

Any updates related to this issue? As far as I see has Jarek updated his PR.

jazzband/django-model-utils#382 is e.g. waiting for a fix.

Thanks!

Awesome. Thanks for the detailed review!

• I agree that always setting the descriptor is arguably a better way to go. I believe additional tests may fail as a result, and obviously the change may affect more users too, but I'm really not a fan of the is_attname_settable method I've currently got in this PR when we don't have a solid case for why we desire such specific behaviour.
• I will do a bit of my own investigation into with why mro is affected by this; the metaclasses probably have a hand in it. If the rabbithole gets too deep, it's nice you're saying we might be happy to concede that we won't support pure python MRO. The "diamond" case is kind of unusual anyway so usage of that would be pretty low.
• I'll read through the other tickets you've linked and see if I this change helps them; if yes, will add tests.
• Yes, I'm definitely interested in seeing this to completion, and your guidance will certainly help. Give me a few weeks to come back with the next iteration, and we'll discuss more then.

Cheers,

• This ticket (you have one). -- yep
• FieldTracker for AbstractModel children is still not stable jazzband/django-model-utils#382 -- covered by my initial test_shadow_parent_attribute_with_field test:

> from django.contrib.auth.models import AbstractUser

# Prior to change in this PR:
> AbstractUser.is_active
True  # True is defined on AbstractBaseUser and was preventing AbstractUser from overriding it.

# After the change in this PR:
> AbstractUser.is_active
<django.db.models.query_utils.DeferredAttribute object at 0x7fa1d01a1b20>

• ticket-16176 -- covered in test_shadow_parent_property_with_field
• ticket-28198 -- arguably already covered by test_shadow_parent_attribute_with_field ensuring we've got an instance of DeferredAttribute, but I've added ShadowDeferTests to defer/test.py to cover more explicitly.
• ticket-27807 -- not relevant I think. username_validator does not come through the contribute_to_class method. I'd assume it inherits just fine, but the point at which the field is created happens prior to any inheritance behaviour. I've added a test for this as well (for your reference), though I'd be inclined to back it out and have it addressed separate to this work.

I've stuck to the wording 'shadow', since that's what was in there already, though i think shadowing refers more to cross-scope variable overrides, and might not entirely be correct in this case.. lemme know if you think it should be changed.

As for the diamond case, I can see what's going on.
This is the line that adds fields for abstract base models: https://github.com/jarekwg/django/blame/ticket_30427/django/db/models/base.py#L274
We iterate over them in mro order, however when we ask them what fields they have to offer, we don't ask them if the fields were defined explicitly on them, or on one of their ancestors.
So in this case, DescendantOne comes in as the first abstract base, and even though it doesn't define name directly, it inherits it from AbstractBaseTwo, basically pulling it up and causing it to get looked at before the name defined directly on DescendantTwo. Once name is looked at once, all future abstract bases that try to bring it in are ignored.
The fix would be to have abstract bases only add_to_class fields that they define explicitly (not inherit from a parent), however this is actually nontrivial to check for (I believe). If we want to fix this, we'll have to get a little crafty, and I'm not sure it's worth the effort. Lemme know what you think!
Alternatively, we don't iterate over the base classes at all and only staple on fields at the top level (where python would've already ensured correct mro behaviour), however that's a much more drastic change/simplification, and there must be reasons it's been done the way it is now.

Hi @jarekwg. Good stuff. I had a quick look and rebased and squashed.

...I'd be inclined to back it out ...

OK, do that. I agree. I was just being optimistic I think 🙂

I need to go through your comments and the failures one by one to determine if they're acceptable to change.

Removed username_validator tests. Left a diff for them on https://code.djangoproject.com/ticket/27807#comment:15

Leaving the other failing tests as they are until you get a look at them first.
I'd say:

• remove check_framework.tests.CheckFrameworkReservedNamesTests.test_model_check_method_not_shadowed
• remove invalid_models_tests.test_models.OtherModelTests.test_property_and_related_field_accessor_clash
• mark as expected failure model_inheritance.test_abstract_inheritance.AbstractInheritanceTests.test_multiple_parents_mro

OK. Great stuff @jarekwg. I’ll look at this again next week now.

But we’ll definitely need a release note for the breaking change.

Then we’ll need to approach the list and ask for extra feedback. I think the bug we’re fixing (the User case being the clearest in my mind) is probably worth it but it’s not something I’d take in without consultation first. (So, release notes, my review, edit tests, mailing list...)

Let me know if you'd rather have the commit squashed into previous. I'm leaving it separate to make it more obvious where I've made changes.

e: spellcheck doesn't approve of "truthy"! :o

Hey @jarekwg. Thanks for picking this up. Please do leave separate commits for now. (Exactly makes it easier to review.)

Right, "truthy" is a word, so I added it to the spelling wordlist.

I added an example to the release notes. It's a bit long but I don't see that it can be much shorter and still get the point across? (@felixxm @adamchainz?)

If we're happy, I think we're there... 🤞

Thanks for your work on this one @jarekwg. It's a monster. 👹

It's a bit long but I don't see that it can be much shorter and still get the point across? (@felixxm @adamchainz?)

I can't see anything to cut out. I'd even consider adding a sentence that makemigrations will detect changes if they exist (they will right?).

Ah, didn't actually run against the new test from 4bbe826 ... Let me work out what it did change here.

(Sigh 🙂)

Right, this is the pull request that never dies.

Putting the old test back in, multiply inheriting fields is enabled by this change.

The test added in 4bbe826 fails, and the old test needs adjusting, as we had before, since normal Python MRO is not respected.

@jarekwg Do you have capacity to play with this and work out exactly what the ordering is etc?

• This is the relevant bit of ModelBase.__new__() It's looking at new_class._meta.local_fields at each point there and making sure it's sane. (I'll try and fire up the debugger for that again, but it keeps eating the time I allocate for it...)
• Adjusting the failing test here.
• (Re-)Adding a diamond-like example, like the old test, and adjusting that as we had it, with the comment
• Then documenting exactly what we can say about the behaviour. (Docs & release notes.)

On master you can't multiply inherit a field (you've never been able to) so we're not breaking anything (AFAICS). We should be able to get a fix in (the new test cases are compelling). If not we should have a definite idea of what the blocker is.

@carltongibson sure!
Just to clarify, what further insights (beyond what I summarised here) are you after when you ask:

... work out exactly what the ordering is etc?

Resummarising my previous comment in short: abstract base classes are traversed in mro() order, but the way we then add fields from them onto the new class is reckless -- we don't check whether the fields are defined directly on the classes we're traversing, or on one of their ancestors.

I'll:

• address the new failing test from master.
• re-add the diamond test.
• have another read of the ModelBase.__new__() code to see if we can get diamond inheritance working without making any scary changes to this part of the code.
• update docs accordingly, depending on the changes.

Will try to have a crack this long easter weekend.

Hi Jared.

I want to be 100% clear on the state of the model at each point.

The old test looked like it was working. Except whilst it had a single Deferred descriptor, there were two identically named fields added (to local_fields) which is why, when we added the model check() call the error was raised.

It wasn’t really the case that the fields were overridden in the sense of normal inheritance. Rather when you fetch the field you’d get the one from the base class, not the DescendantTwo.

With the change, I want to make sure that the resultant model is sane. Maybe an added assert or two on the state of the _meta object makes sense.

(As I say, I want to fire it up in the debugger myself as well.)

This is such a central part of the framework that we must tread very carefully.

Unless we have no option, I don’t think further changes to ModelBase.__new__() is a good idea here.

With my latest read-through of ModelBase.__new__(), I just noticed that it doesn't even do a full mro traversal of ancestors as i'd initially thought; it only looks at direct ancestors due to the base not in parents condition here:

So there really is absolutely no way of supporting diamond inheritance in the current state, because even if we did tell DescendantOne to only provide fields that were directly defined on it, we'd never end up looping through to its ancestors (ie AbstractBase) to find the other fields..

It desperately needs a rewrite; dancing around it is just going to continue causing pain.
But for that to happen safely, we need beefier tests.

Soooo, I went a little more aggressive on these abstract inheritance tests, adding some utils for quick class creation, then applying a more rigorous covering of test cases. The PRO of this approach is we can write a tonne of complex inheritance cases in a readable way. The CON is a pattern like this could itself potentially introduce issues, or be a little harder to maintain. Please let me know if I've gone too far and I can revert the changes back to an expanded form. Please also let me know what you think of this suite of test cases; whether they're all sensible, or somewhy superfluous.

Also, what do you think about playing things out this way:

• We hold off with this PR (again), and introduce a new check, specifically for any instance of diamond inheritance (regardless of what kind of field definitions exist). If identified, slap user on wrist and tell them Django doesn't support this. If we give them a firm NO now, then we'll have grounds to fix it in future without worrying about causing further regressions.
• We merge this PR. Failing test for diamond inheritance will be a sleeper.
• One day, ModelBase.__new__() will get rewritten, at which point we can remove the check and the @expectedfailure off the test.

I made a commit changing tests only, cause would like to know what you think; but happy to update docs too.

re the sanity you were referring to, I had a quick look with:

                # Add fields from abstract base class if it wasn't overridden.
                print(base._meta)
                print(parent_fields)
                for field in parent_fields:
                    print(new_class.__dict__)
                    if (field.name not in field_names and
                            field.name not in new_class.__dict__ and
                            field.name not in inherited_attributes):
                        new_field = copy.deepcopy(field)
                        print(f'BEFORE: {new_class._meta.local_fields}')
                        new_class.add_to_class(field.name, new_field)
                        print(f'AFTER: {new_class._meta.local_fields}')

new_class._meta.local_fields appears to have the new field cleanly added, once only, thanks to the field.name not in new_class.__dict__ check. Here's some sample output from running the above against one of my new tests:

--------------------------------------------------
DerivedFromBases12NO
model_inheritance.abstractbase1
[<django.db.models.fields.CharField: foo>]
{'__module__': 'model_inheritance.test_abstract_inheritance', '__doc__': None, '_meta': <Options for DerivedFromBases12NO>, 'DoesNotExist': <class 'model_inheritance.test_abstract_inheritance.DerivedFromBases12NO.DoesNotExist'>, 'MultipleObjectsReturned': <class 'model_inheritance.test_abstract_inheritance.DerivedFromBases12NO.MultipleObjectsReturned'>}
BEFORE: []
AFTER: [<django.db.models.fields.CharField: foo>]
model_inheritance.abstractbase2
[<django.db.models.fields.CharField: foo>]
{'__module__': 'model_inheritance.test_abstract_inheritance', '__doc__': None, '_meta': <Options for DerivedFromBases12NO>, 'DoesNotExist': <class 'model_inheritance.test_abstract_inheritance.DerivedFromBases12NO.DoesNotExist'>, 'MultipleObjectsReturned': <class 'model_inheritance.test_abstract_inheritance.DerivedFromBases12NO.MultipleObjectsReturned'>, 'foo': <django.db.models.query_utils.DeferredAttribute object at 0x7f85f00f8e50>}
--------------------------------------------------

Sorry for the slow follow-up: in the first-place, 2020, but it's taken me a while to have a window to sit down with this again, and to get to stare at ModelBase.__new__() for sufficiently long. 😳

Heh, no worries. Thanks for circling back round to this.

I'll need to refresh my thoughts a bit again, but if you're saying that diamond-inheritance-wrist-slapping already occurs currently (not sure how i failed to notice this..), then we're good to go with pushing out this change. I'll try to find some time to expand out the tests this weekend.

Cheers,

The key point is that such a model fails the check() call. (The old test didn't actually call check() so it was never caught...)

The key point is that such a model fails the check() call. (The old test didn't actually call check() so it was never caught...)

Ahh, ok that makes more sense. Thanks!

Great work on this @jarekwg ... @carltongibson how's it looking now?

@aidanlister it's just waiting for another chance to review again. It's on my long list for 3.2.

Closing in favour of #14508. Thanks @jarekwg!

No worries! I'm just happy to see it get addresed.