[Django] #20749: get_FOO_display does not return proper value or warn for inconsistent type
Django
----------------------------------------------+--------------------
Reporter: ellisd23@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
If I have a model like so:
{{{
class Program(models.Model):
STATUS_CHOICES = (
(1, 'Inactive'),
(2, 'Active'),
)
status = models.CharField(choices=STATUS_CHOICES)
}}}
And I try get_status_display(), I will get '1' or '2' as the output rather
than 'Inactive' or 'Active'. Django silently coerces the type of the
choices upon saving, but does not give an indicator upon retrieval that
the choice was not found. _get_FIELD_display in db/models/base.py
attempts to pull the value from the choices, but since python does not see
int and str keys as equivalent, it does not find the value; instead, it
defaults to the value in the database.
The end-user solution is to make sure all types are correct, but this
could cause confusion for a newbie since this is all done without warning.
I see a few possible solutions:
1) warn the user when creating a choice field that is not of the same
type as the model.
2) warn the user when get_FOO_display is called but no value is found.
3) coerce the keys to strings before attempting to retrieve in
get_FOO_display. I'm not sure if this would have other implications.
--
Ticket URL: <https://code.djangoproject.com/ticket/20749>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: ellisd23@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ellisd23@…):
* needs_better_patch: => 0
* type: Uncategorized => Bug
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:1>
Django
Reporter: ellisd23@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by anonymous):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:2>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => ellisd23
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:3>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by ellisd23):
I've added a pull request for this issue:
https://github.com/django/django/pull/1393
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:4>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:5>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* version: 1.5 => master
Comment:
The current patch will not suffice, as it only addresses Char and Integer
fields, but choices can be applied to more Field types than that.
between options 1 or 2, I'd lean slightly to 2, as it is a more
straightforward implementation.
The question is whether there are enough valid cases where you want to
call get_FIELD_display on a model where you know the value is not a key in
your choices, and getting a bunch of warning noise would be annoying.
I think that is a pretty small set of cases, and has to be weighed against
keeping the status quo which can result in a pretty confusing problem as
it doesn't include any immediate hint as to why you are getting the label
back.
If the implementation proves to not be practical, a warning in the docs
should be clarify what happens when you ask for a label for a mismatched
type between choices-key/field.
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:6>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by ellisd23):
Yeah, I figured this was at least a step in the right direction. Is there
a way we could generically test that the type of the choice aligns with
the chosen field? I'll dig around in the code for the fields.
I'm worried that with option 2, since these methods would likely be used
frequently in templates, it would lead to a large number of warnings. It
seems like option 1 would be more "pure" as well, since your types really
should match the field as it will be stored and returned.
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:7>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by ellisd23):
Okay, I believe I've a solution that is 1) lean, only modifying one
function in the codebase; 2) better for the user, adding the functionality
as intended rather than bothering the user with warnings; and 3) flexible,
using a field's to_python function to look up the value rather than
attempting to discern the type.
I've added the commit to the pull request. The specific commit is here:
https://github.com/dellis23/django/commit/308d9d10967727395db22757661170467412d6d6
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:8>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:9>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by mjtamlyn):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
The patch at https://github.com/django/django/pull/1393 looks good, but it
needs a test so I can really understand exactly what you're trying to do.
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:10>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by ellisd23):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
I've added a test. The specific commit is here:
https://github.com/dellis23/django/commit/c3098afb0ba240e1195d4dce03146089717fe74e
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:11>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
It seems to me this introduces a performance penalty (iterating through
choices to calling `to_python`) which in most cases shouldn't be necessary
as long as users declare their models correctly. I would tend to favor the
model validation solution. I wonder if the idea of using `to_python` might
work there.
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: ellisd23@… | Owner: ellisd23
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: timograham@… (added)
* has_patch: 1 => 0
* type: Bug => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:13>
Django
Reporter: ellisd23@… | Owner: ellisd23
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Database layer (models, ORM) => Core (System checks)
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:14>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: ellisd23 => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:15>
Django
--------------------------------------+------------------------------------
Reporter: ellisd23@… | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anvesh Mishra):
We should also take in account
[https://code.djangoproject.com/ticket/27704 #27704].
--
Ticket URL: <https://code.djangoproject.com/ticket/20749#comment:16>