Opened 11 years ago
Last modified 21 months ago
#20749 new New feature
Add validation for type of Field.choices arguments
Reported by: | Owned by: | ||
---|---|---|---|
Component: | Core (System checks) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | timograham@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (16)
comment:1 by , 11 years ago
Type: | Uncategorized → Bug |
---|
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 11 years ago
comment:5 by , 11 years ago
Has patch: | set |
---|
comment:6 by , 11 years ago
Patch needs improvement: | set |
---|---|
Version: | 1.5 → master |
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.
comment:7 by , 11 years ago
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.
comment:8 by , 11 years ago
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
comment:9 by , 11 years ago
Patch needs improvement: | unset |
---|
comment:10 by , 11 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
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.
comment:11 by , 11 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
I've added a test. The specific commit is here: https://github.com/dellis23/django/commit/c3098afb0ba240e1195d4dce03146089717fe74e
comment:12 by , 11 years ago
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.
comment:13 by , 11 years ago
Cc: | added |
---|---|
Has patch: | unset |
Summary: | get_FOO_display does not return proper value or warn for inconsistent type → Add validation for type of Field.choices arguments |
Type: | Bug → New feature |
comment:14 by , 9 years ago
Component: | Database layer (models, ORM) → Core (System checks) |
---|
comment:15 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:16 by , 21 months ago
We should also take in account #27704. The validation should also work for ArrayField.base_field.choices
imo.
I've added a pull request for this issue: https://github.com/django/django/pull/1393