Cannot override get_FOO_display() in Django 2.2+.

[Jim Ouwerkerk]

I cannot override the get_FIELD_display function on models since version 2.2. It works in version 2.1.

Example:

class FooBar(models.Model):
    foo_bar = models.CharField(_("foo"), choices=[(1, 'foo'), (2, 'bar')])

    def __str__(self):
        return self.get_foo_bar_display()  # This returns 'foo' or 'bar' in 2.2, but 'something' in 2.1

    def get_foo_bar_display(self):
        return "something"

What I expect is that I should be able to override this function.

[Mariusz Felisiak]

Thanks for this report.

Regression in a68ea231012434b522ce45c513d84add516afa60.
Reproduced at 54a7b021125d23a248e70ba17bf8b10bc8619234.

[Carlton Gibson]

OK, I have a lead on this. Not at all happy about how it looks at first pass, but I'll a proof of concept PR together for it tomorrow AM.

[Sergey Fedoseev]

I don't think it should be marked as blocker since it looks like it was never supported, because it depends on the order of attrs passed in ModelBase.__new__(). So on Django 2.1 and Python 3.7:

In [1]: import django
   ...: django.VERSION
In [2]: from django.db import models 
   ...:  
   ...: class FooBar(models.Model): 
   ...:     def get_foo_bar_display(self): 
   ...:         return "something" 
   ...:  
   ...:     foo_bar = models.CharField("foo", choices=[(1, 'foo'), (2, 'bar')]) 
   ...:  
   ...:     def __str__(self): 
   ...:         return self.get_foo_bar_display()  # This returns 'foo' or 'bar' in 2.2, but 'something' in 2.1 
   ...:  
   ...:     class Meta: 
   ...:         app_label = 'test' 
   ...:  
   ...: FooBar(foo_bar=1)
Out[2]: <FooBar: foo>

Before ​Python 3.6 the order of attrs wasn't defined at all.

[Mariusz Felisiak]

Sergey, an example from the ticket description works for me with Django 2.1 and Python 3.6, 3.7 and 3.8.

[Sergey Fedoseev]

In [2]: import django 
   ...: django.VERSION                               
Out[2]: (2, 1, 13, 'final', 0)

In [3]: import sys 
   ...: sys.version                                  
Out[3]: '3.5.7 (default, Oct 17 2019, 07:04:41) \n[GCC 8.3.0]'

In [4]: from django.db import models 
   ...:  
   ...: class FooBar(models.Model): 
   ...:     foo_bar = models.CharField("foo", choices=[(1, 'foo'), (2, 'bar')]) 
   ...:  
   ...:     def __str__(self): 
   ...:         return self.get_foo_bar_display()  # This returns 'foo' or 'bar' in 2.2, but 'something' in 2.1 
   ...:  
   ...:     def get_foo_bar_display(self): 
   ...:         return "something" 
   ...:  
   ...:     class Meta: 
   ...:         app_label = 'test' 
   ...:  
   ...: FooBar(foo_bar=1)                            
Out[4]: <FooBar: foo>

[Carlton Gibson]

OK, so there is a behaviour change here, but Sergey is correct that it does depend on attr order, so it's hard to say that this can be said to ever have been thought of as supported, with the exact example provided.

This example produces the opposite result on 2.1 (even on >=PY36):

    def test_overriding_display_backwards(self):

        class FooBar2(models.Model):

            def get_foo_bar_display(self):
                return "something"

            foo_bar = models.CharField("foo", choices=[(1, 'foo'), (2, 'bar')])

        f = FooBar2(foo_bar=1)

        # This returns 'foo' or 'bar' in both 2.2 and 2.1
        self.assertEqual(f.get_foo_bar_display(), "foo")

Because get_foo_bar_display() is defined before foo_bar is gets replaced in the the add_to_class() step.

Semantically order shouldn't make a difference. Given that it does, I can't see that we're bound to maintain that behaviour.
(There's a possible fix in Field.contribute_to_class() but implementing that just reverses the pass/fail behaviour depending on order...)

Rather, the correct way to implement this on 2.2+ is:

    def test_overriding_display(self):
        class FooBar(models.Model):
            foo_bar = models.CharField("foo", choices=[(1, 'foo'), (2, 'bar')])

            def _get_FIELD_display(self, field):
                if field.attname == 'foo_bar':
                    return "something"
                return super()._get_FIELD_display(field)

        f = FooBar(foo_bar=1)
        self.assertEqual(f.get_foo_bar_display(), "something")

This is stable for declaration order on version 2.2+.

This approach requires overriding _get_FIELD_display() before declaring fields on 2.1, because otherwise Model._get_FIELD_display() is picked up during Field.contribute_to_class(). This ordering dependency is, ultimately, the same issue that was addressed in a68ea231012434b522ce45c513d84add516afa60, and the follow-up in #30254.

The behaviour in 2.1 (and before) was incorrect. Yes, there's a behaviour change here but it's a bugfix, and all bugfixes are breaking changes if you're depending on the broken behaviour.

I'm going to downgrade this from Release Blocker accordingly. I'll reclassify this as a Documentation issue and provide the working example, as overriding _get_FIELD_display() is a legitimate use-case I'd guess.

[Sergey Fedoseev]

Replying to Carlton Gibson:

(There's a possible fix in Field.contribute_to_class() but implementing that just reverses the pass/fail behaviour depending on order...)

Doesn't this fix it?

if not hasattr(cls, 'get_%s_display' % self.name):
    setattr(cls, 'get_%s_display' % self.name,
                  partialmethod(cls._get_FIELD_display, field=self))

[Carlton Gibson]

Hi Sergey,

No. That's exactly the "implementing that just reverses the pass/fail behaviour depending on order..." — two test cases, one with the override declared before the field, one after, the one fails as it is, the other fails if you put that guard in... — That's on 2.1, but it's the underlying issue being exposed: the right approach is overriding _get_FIELD_display(). (Rather than us trying to patch something into Field....)

​PR

[Sergey Fedoseev]

Replying to Carlton Gibson:

Hi Sergey,

No. That's exactly the "implementing that just reverses the pass/fail behaviour depending on order..." — two test cases, one with the override declared before the field, one after, the one fails as it is, the other fails if you put that guard in... — That's on 2.1, but it's the underlying issue being exposed: the right approach is overriding _get_FIELD_display(). (Rather than us trying to patch something into Field....)

​PR

​Both tests pass for me ¯\_(ツ)_/¯.

[Carlton Gibson]

Yes, on 2.2 they would. But it's a change of behaviour from 2.1 (which is the point here I take it).

The new __new__() behaviour is correct™.

So we have a choice:

• Add code to Field, which IMO is very much the wrong place. (Field shouldn't need to examine the __dict__ of the class it's being applied to — that way lies madness, or
• Document the correct way to customize this, requiring 0 code changes.

I've opted for the latter.

[Sergey Fedoseev]

Replying to Carlton Gibson:

Yes, on 2.2 they would.

They pass on 2.1 too, since the mentioned commit is based on stable/2.1.x branch, but I'm not sure why this is relevant since we agreed that this isn't something that should be backported.

Field shouldn't need to examine the __dict__ of the class it's being applied to

It already ​does it.

Document the correct way to customize this, requiring 0 code changes.

I don't think that overriding non-public methods could be considered as the correct way of customization.

[Jim Ouwerkerk]

I would expect that overriding the function should work. It's pretty common to override functions in Python and dev's expect that they can override other methods. I don't understand why it should not be supported for the get_FOO_display method. I think it's a basic expectation that it would work.

[Carlton Gibson]

They pass on 2.1 too...

Err. Can I ask you to check again please. We're obviously seeing something different.

$ git show --name-only
commit 522af9d6737550ef035a173c08a8276028b68917 (HEAD -> stable/2.1.x, origin/stable/2.1.x)
...
(django) ~/Documents/Django-Stack/django/tests (stable/2.1.x)$ git diff
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index 50d22bef0c..38aed6c818 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -744,8 +744,9 @@ class Field(RegisterLookupMixin):
             if not getattr(cls, self.attname, None):
                 setattr(cls, self.attname, DeferredAttribute(self.attname))
         if self.choices:
-            setattr(cls, 'get_%s_display' % self.name,
-                    partialmethod(cls._get_FIELD_display, field=self))
+            if not hasattr(cls, 'get_%s_display' % self.name):
+                setattr(cls, 'get_%s_display' % self.name,
+                        partialmethod(cls._get_FIELD_display, field=self))
 
     def get_filter_kwargs_for_object(self, obj):
         """
diff --git a/tests/model_fields/tests.py b/tests/model_fields/tests.py
index 45f61a0034..31766a2d5c 100644
--- a/tests/model_fields/tests.py
+++ b/tests/model_fields/tests.py
@@ -157,3 +157,25 @@ class GetChoicesTests(SimpleTestCase):
         lazy_func = lazy(lambda x: 0 / 0, int)  # raises ZeroDivisionError if evaluated.
         f = models.CharField(choices=[(lazy_func('group'), (('a', 'A'), ('b', 'B')))])
         self.assertEqual(f.get_choices(include_blank=True)[0], ('', '---------'))
+
+    def test_overriding_FIELD_display(self):
+        class FooBar(models.Model):
+            foo_bar = models.CharField(choices=[(1, 'foo'), (2, 'bar')])
+
+            def _get_FIELD_display(self, field):
+                if field.attname == 'foo_bar':
+                    return "something"
+                return super()._get_FIELD_display(field)
+
+        f = FooBar(foo_bar=1)
+        self.assertEqual(f.get_foo_bar_display(), "something")
+
+    def test_overriding_FIELD_display_2(self):
+        class FooBar2(models.Model):
+            def get_foo_bar_display(self):
+                return 'something'
+
+            foo_bar = models.CharField(choices=[(1, 'foo'), (2, 'bar')])
+
+        f = FooBar2(foo_bar=1)
+        self.assertEqual(f.get_foo_bar_display(), 'something')
(django) ~/Documents/Django-Stack/django/tests (stable/2.1.x)$ ./runtests.py model_fields.tests.GetChoicesTests
Testing against Django installed in '/Users/carlton/Documents/Django-Stack/django/django' with up to 4 processes
Creating test database for alias 'default'...
Creating test database for alias 'other'...
System check identified no issues (0 silenced).
...F.
======================================================================
FAIL: test_overriding_FIELD_display (model_fields.tests.GetChoicesTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/carlton/Documents/Django-Stack/django/tests/model_fields/tests.py", line 171, in test_overriding_FIELD_display
    self.assertEqual(f.get_foo_bar_display(), "something")
AssertionError: 'foo' != 'something'
- foo
+ something


----------------------------------------------------------------------
Ran 5 tests in 0.004s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

It's relevant precisely because it's a change in behaviour. ("Regression from previous version...")

I don't think that overriding non-public methods could be considered as the correct way of customization.

OK. In this case I disagree. (That's allowed.) I think it's a rare use-case, worth documenting, but not worth adding a code path for.

It already does...

Yes, but more isn't better here.

(You're welcome to think different.)

@Jim: Yes, I hear your sentiment, but, for me, here you're dealing with special methods added by the metaclass. It's not a normal case of subclassing.

I would much rather state, plainly, the vanilla way of achieving the desired result, even if pointing to an otherwise private method, than add code to magic the same thing. Every hook we add back from Field makes the code here marginally more difficult to maintain and adjust. Again for me, this is niche use-case that doesn't merit that addition.

I'd hope that at least make sense.

[Sergey Fedoseev]

Replying to Carlton Gibson:

We're obviously seeing something different.

I guess that's because you override _get_FIELD_display() but not get_foo_bar_display() in test_overriding_FIELD_display.

[Carlton Gibson]

Ah, yes, you're right. I've copied the second test in wrong.

But the point is (still) the change in behaviour from 2.1.

That's still a bug fix, so OK it's changed, that being the nature of bug fixes.

I'm happy to document this, as suggested. I'd be happy to instead just add a warning saying "Meh, don't override methods added by the metaclass....".
Between those, I quite like the option of documenting the solution, since it opens the box for people a little. The warning just leads to a "Why...?", and given that it's come up, it's presumably a valid use-case.

Anyhow, I'm leaving to to Mariusz now.

[Claude Paroz]

I also think that if we find a way to make the get_<field>_display() properly override the metaclass-added method, I think we should do it, possibly with a backwards compatibility note if it breaks some old behavior.

[Carlton Gibson]

Then the fix is easy enough. It’s just the addition to Field.contribute_to_class().

But if we’re going to fix it, then it should be as a Release Blocker to be backported to 2.2. (Otherwise it ≈works in 2.1, doesn’t work in 2.2 and works again in 3.0.)

[Claude Paroz]

I think that's the right thing to do.

[Carlton Gibson]

PR updated ready for Monday.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2d38eb0a:

Fixed #30931 -- Restored ability to override Model.get_FIELD_display().

Thanks Sergey Fedoseev for the implementation idea.

Regression in a68ea231012434b522ce45c513d84add516afa60.

[Mariusz Felisiak <felisiak.mariusz@…>]

In dd2ca8b0:

[3.0.x] Fixed #30931 -- Restored ability to override Model.get_FIELD_display().

Thanks Sergey Fedoseev for the implementation idea.

Regression in a68ea231012434b522ce45c513d84add516afa60.

Backport of 2d38eb0ab9f78d68c083a5b78b1eca39027b279a from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In 785d170:

[2.2.x] Fixed #30931 -- Restored ability to override Model.get_FIELD_display().

Thanks Sergey Fedoseev for the implementation idea.

Regression in a68ea231012434b522ce45c513d84add516afa60.

Backport of 2d38eb0ab9f78d68c083a5b78b1eca39027b279a from master