Abstract model field should not be equal across models

[Ryan Hiebert]

Consider the following models:

class A(models.Model):
    class Meta:
        abstract = True
    myfield = IntegerField()

class B(A):
    pass

class C(A):
    pass

If I pull the fields of B and C into a shared set, one will be de-duplicated away, because they compare as equal. I found this surprising, though in practice using a list was sufficient for my need. The root of the issue is that they compare equal, as fields only consider self.creation_counter when comparing for equality.

len({B._meta.get_field('myfield'), C._meta.get_field('myfield')}) == 1
B._meta.get_field('myfield') == C._meta.get_field('myfield')

We should adjust __eq__ so that if the field.model is different, they will compare unequal. Similarly, it is probably wise to adjust __hash__ and __lt__ to match.

When adjusting __lt__, it may be wise to order first by self.creation_counter so that cases not affected by this equality collision won't be re-ordered. In my experimental branch, there was one test that broke if I ordered them by model first.

I brought this up on IRC django-dev to check my intuitions, and those conversing with me there seemed to agree that the current behavior is not intuitive.

[Carlton Gibson]

OK, happy to look at a patch — assuming nothing breaks...

It'd be a change in behaviour, so would need calling out in the release notes.

[Ryan Hiebert]

​PR

Should this get backported? It feels like an (admittedly esoteric) bug fix to me. It affects ordering, though I've minimized that effect to only places where the ordering is currently undefined, as they are currently considered equal.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 502e75f9:

Fixed #31750 -- Made models.Field equality compare models for inherited fields.

[Nicolas Delaby]

Hi, I wonder if the submitted fix didn't introduce a subtle regression.
Given the example posted in the description, how would you evaluate the following statements ?

before ​https://github.com/django/django/commit/502e75f9ed5476ffe8229109acf0c23999d4b533

assert B._meta.get_field('myfield') == C._meta.get_field('myfield')
assert B._meta.get_field('myfield') == A._meta.get_field('myfield')
assert C._meta.get_field('myfield') == A._meta.get_field('myfield')

after ​https://github.com/django/django/commit/502e75f9ed5476ffe8229109acf0c23999d4b533

assert B._meta.get_field('myfield') != C._meta.get_field('myfield')
assert B._meta.get_field('myfield') != A._meta.get_field('myfield')
assert C._meta.get_field('myfield') != A._meta.get_field('myfield')

What if it shouldn't be this ? Since B (or C) doesn't override myfield, I would expect the subclass to evaluate equality as if the field was defined on itself (given the parent model is abstract)
maybe?

assert B._meta.get_field('myfield') != C._meta.get_field('myfield')
assert B._meta.get_field('myfield') == A._meta.get_field('myfield')
assert C._meta.get_field('myfield') == A._meta.get_field('myfield')

[Mariusz Felisiak]

Hi, I wonder if the submitted fix didn't introduce a subtle regression.

This is an expected change IMO. Model fields don't make much sense without models (even abstract) so we should distinguish between fields in abstract models and the same fields inherited from them.

[Nicolas Delaby]

Thanks for clarifying Mariusz.