Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30931 closed Cleanup/optimization (fixed)

Cannot override get_FOO_display() in Django 2.2+.

Reported by: Jim Ouwerkerk Owned by: Carlton Gibson
Component: Documentation Version: 2.2
Severity: Release blocker Keywords:
Cc: Sergey Fedoseev Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (27)

comment:1 by Jim Ouwerkerk, 5 years ago

Type: UncategorizedBug

comment:2 by Mariusz Felisiak, 5 years ago

Cc: Sergey Fedoseev Carlton Gibson added
Severity: NormalRelease blocker
Summary: Cannot override `get_FIELD_display` anymoreCannot override get_FOO_display() in Django 2.2+.
Triage Stage: UnreviewedAccepted

Thanks for this report.

Regression in a68ea231012434b522ce45c513d84add516afa60.
Reproduced at 54a7b021125d23a248e70ba17bf8b10bc8619234.

comment:3 by Carlton Gibson, 5 years ago

Owner: changed from nobody to Carlton Gibson
Status: newassigned

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.

comment:4 by Sergey Fedoseev, 5 years ago

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.

comment:5 by Mariusz Felisiak, 5 years ago

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

comment:6 by Sergey Fedoseev, 5 years ago

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>

comment:7 by Carlton Gibson, 5 years ago

Component: Database layer (models, ORM)Documentation
Severity: Release blockerNormal
Type: BugCleanup/optimization

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 is 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.

Version 0, edited 5 years ago by Carlton Gibson (next)

in reply to:  7 comment:8 by Sergey Fedoseev, 5 years ago

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))

comment:9 by Carlton Gibson, 5 years ago

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

comment:10 by Carlton Gibson, 5 years ago

Has patch: set

in reply to:  9 comment:11 by Sergey Fedoseev, 5 years ago

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 ¯\_(ツ)_/¯.

comment:12 by Carlton Gibson, 5 years ago

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.

in reply to:  12 comment:13 by Sergey Fedoseev, 5 years ago

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.

comment:14 by Jim Ouwerkerk, 5 years ago

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.

comment:15 by Carlton Gibson, 5 years ago

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.

comment:16 by Carlton Gibson, 5 years ago

Owner: Carlton Gibson removed
Status: assignednew

in reply to:  15 comment:17 by Sergey Fedoseev, 5 years ago

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.

comment:18 by Carlton Gibson, 5 years ago

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 Mariusz now.

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:19 by Carlton Gibson, 5 years ago

Cc: Carlton Gibson removed

comment:20 by Claude Paroz, 5 years ago

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.

comment:21 by Carlton Gibson, 5 years ago

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.)

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:22 by Claude Paroz, 5 years ago

I think that's the right thing to do.

comment:23 by Carlton Gibson, 5 years ago

Severity: NormalRelease blocker

comment:24 by Carlton Gibson, 5 years ago

Owner: set to Carlton Gibson
Status: newassigned

PR updated ready for Monday.

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 2d38eb0a:

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

Thanks Sergey Fedoseev for the implementation idea.

Regression in a68ea231012434b522ce45c513d84add516afa60.

comment:26 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

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

comment:27 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

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

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