Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#31124 closed Bug (fixed)

Model.get_FOO_display() does not work correctly with inherited choices.

Reported by: Yash Jhunjhunwala Owned by: Carlton Gibson
Component: Database layer (models, ORM) Version: 3.0
Severity: Release blocker Keywords:
Cc: Carlton Gibson, Sergey Fedoseev Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mariusz Felisiak)

Given a base model with choices A containing 3 tuples
Child Model inherits the base model overrides the choices A and adds 2 more tuples
get_foo_display does not work correctly for the new tuples added

Example:

class A(models.Model):
   foo_choice = [("A","output1"),("B","output2")]
   field_foo = models.CharField(max_length=254,choices=foo_choice)
   class Meta:
       abstract = True

class B(A):
   foo_choice = [("A","output1"),("B","output2"),("C","output3")]
   field_foo = models.CharField(max_length=254,choices=foo_choice)

Upon invoking get_field_foo_display() on instance of B ,
For value "A" and "B" the output works correctly i.e. returns "output1" / "output2"
but for value "C" the method returns "C" and not "output3" which is the expected behaviour

Change History (17)

comment:1 Changed 10 months ago by Mariusz Felisiak

Component: UncategorizedDatabase layer (models, ORM)
Easy pickings: unset
Resolution: needsinfo
Status: newclosed
Summary: get foo display - model inheritance does not work correctlyModel.get_FOO_display() does not work correctly with inherited choices.

Thanks for this report. Can you provide models and describe expected behavior? Can you also check if it's not a duplicate of #30931?, that was fixed in Django 2.2.7.

comment:2 in reply to:  1 ; Changed 10 months ago by Yash Jhunjhunwala

Description: modified (diff)

Replying to felixxm:

Thanks for this report. Can you provide models and describe expected behavior? Can you also check if it's not a duplicate of #30931?, that was fixed in Django 2.2.7.

comment:3 in reply to:  2 Changed 10 months ago by Yash Jhunjhunwala

Added the models and expected behaviour. It is not a duplicate of #30931. Using Django 2.2.9

Replying to felixxm:

Thanks for this report. Can you provide models and describe expected behavior? Can you also check if it's not a duplicate of #30931?, that was fixed in Django 2.2.7.

comment:4 Changed 10 months ago by Mariusz Felisiak

Cc: Carlton Gibson Sergey Fedoseev added
Description: modified (diff)
Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted
Version: 2.23.0

Thanks for an extra info. I was able to reproduce this issue, e.g.

>>> B.objects.create(field_foo='A').get_field_foo_display()
output1
>>> B.objects.create(field_foo='B').get_field_foo_display()
output2
>>> B.objects.create(field_foo='C').get_field_foo_display()
C

Regression in 2d38eb0ab9f78d68c083a5b78b1eca39027b279a (Django 2.2.7).

Last edited 10 months ago by Mariusz Felisiak (previous) (diff)

comment:5 Changed 10 months ago by George Popides

Owner: changed from nobody to George Popides
Status: newassigned

may i work on this?

Last edited 10 months ago by George Popides (previous) (diff)

comment:6 Changed 10 months ago by George Popides

After digging in, i have found that the choices of B model are the same with the A model, despiite them being the proper ones in init.
Migration is correct, so now i must find why the choices of model B are ignored.
Being my first issue, some hints would be appreciated.
Thanks

comment:8 Changed 10 months ago by Mariusz Felisiak

Has patch: set

comment:9 Changed 10 months ago by zeynel

Patch needs improvement: set

I think this ticket is very much related to the discussions on #30931.

The line if not hasattr(cls, 'get_%s_display' % self.name) breaks the expected behaviour on model inheritance, which causing this bug. (see https://github.com/django/django/commit/2d38eb0ab9f78d68c083a5b78b1eca39027b279a#diff-bf776a3b8e5dbfac2432015825ef8afeR766)

IMO there are three important points to discuss:

1- Obviously get_<field>_display() should work as expected with inheritance, so this line should be reverted/fixed: if not hasattr(cls, 'get_%s_display' % self.name)

2- I think developers should be able to override get_<field>_display() method on the model class:

class Bar(models.Model):
   foo = models.CharField('foo', choices=[(0, 'foo')])
   
   def get_foo_display(self):
      return 'something'

b = Bar(foo=0)
assert b.get_foo_display() == 'something'

3- I think Field should not check an attribute of model class and make a decision based on it. This check and set logic should be delegated to BaseModel with an abstraction to make it less magical and more clean. Maybe something like this:

class ModelBase(type):
  ....

  def add_overridable_to_class(cls, name, value):
    // Set value only if the name is not already defined in the class itself (no `hasattr`)
    if name not in cls.__dict__:
      setattr(cls, name, value)

class Field(RegisterLookupMixin):
  ...

  def contribute_to_class(self, cls, name, private_only=False):
    ...
    if self.choices is not None:
      cls.add_overridable_to_class('get_%s_display' % self.name, partialmethod(cls._get_FIELD_display, field=self))
Last edited 10 months ago by zeynel (previous) (diff)

comment:10 Changed 10 months ago by George Popides

Why would checking on fields class be a bad idea?
If you are a field of a model that is not abstract but your parent is an abstract method, wouldn't you want to override your parent's method if you both have the same method?

comment:11 in reply to:  10 Changed 10 months ago by zeynel

Replying to George Popides:

Why would checking on fields class be a bad idea?
If you are a field of a model that is not abstract but your parent is an abstract method, wouldn't you want to override your parent's method if you both have the same method?

Well it is not something I would prefer because it makes two classes tightly coupled to each other, which means it is hard to change one without touching to the other one and you always need to think about side effects of your change. Which eventually makes this two classes hard to test and makes the codebase hard to maintain.

Your logic about overriding might/or might not be true. I would just execute this logic on ModelBase rather than Field.

comment:12 Changed 10 months ago by Carlton Gibson

I've opened https://github.com/django/django/pull/12284 which provides a fix for this, but also demonstrates how this issue will keep coming up...

My inclination here is to back-out the fix for #30931 and document overriding _get_FIELD_display() for this kind of use-case.

My worry is that we get ever more baroque in handling the edge-cases... — but we could take the fix here and draw a line at that point maybe?

comment:13 Changed 9 months ago by Carlton Gibson

Patch needs improvement: unset
Severity: NormalRelease blocker

comment:14 Changed 9 months ago by Mariusz Felisiak

Owner: changed from George Popides to Carlton Gibson

comment:15 Changed 9 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:16 Changed 9 months ago by Carlton Gibson <carlton@…>

Resolution: fixed
Status: assignedclosed

In 29c126bb:

Fixed #31124 -- Fixed setting of get_FOO_display() when overriding inherited choices.

Regression in 2d38eb0ab9f78d68c083a5b78b1eca39027b279a

comment:17 Changed 9 months ago by Carlton Gibson <carlton.gibson@…>

In 57468ea:

[3.0.x] Fixed #31124 -- Fixed setting of get_FOO_display() when overriding inherited choices.

Regression in 2d38eb0ab9f78d68c083a5b78b1eca39027b279a

Backport of 29c126bb349526b5f1cd78facbe9f25906f18563 from master

Note: See TracTickets for help on using tickets.
Back to Top