Opened 3 years ago

Last modified 14 months ago

#20749 assigned New feature

Add validation for type of Field.choices arguments

Reported by: ellisd23@… Owned by: ellisd23
Component: Core (System checks) Version: master
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


If I have a model like so:

class Program(models.Model):
        (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/ 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 (14)

comment:1 Changed 3 years ago by ellisd23@…

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Type: UncategorizedBug

comment:2 Changed 3 years ago by anonymous

Triage Stage: UnreviewedAccepted

comment:3 Changed 3 years ago by ellisd23

Owner: changed from nobody to ellisd23
Status: newassigned

comment:4 Changed 3 years ago by ellisd23

I've added a pull request for this issue:

comment:5 Changed 3 years ago by ellisd23

Has patch: set

comment:6 Changed 3 years ago by Preston Holmes

Patch needs improvement: set
Version: 1.5master

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 Changed 3 years ago 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.

comment:8 Changed 3 years ago 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:

comment:9 Changed 3 years ago by ellisd23

Patch needs improvement: unset

comment:10 Changed 3 years ago by Marc Tamlyn

Needs tests: set
Patch needs improvement: set

The patch at looks good, but it needs a test so I can really understand exactly what you're trying to do.

comment:11 Changed 3 years ago by ellisd23

Needs tests: unset
Patch needs improvement: unset

comment:12 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham

Cc: timograham@… added
Has patch: unset
Summary: get_FOO_display does not return proper value or warn for inconsistent typeAdd validation for type of Field.choices arguments
Type: BugNew feature

comment:14 Changed 14 months ago by Tim Graham

Component: Database layer (models, ORM)Core (System checks)
Note: See TracTickets for help on using tickets.
Back to Top