Add validation for type of Field.choices arguments

[ellisd23@…]

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.

[ellisd23]

I've added a pull request for this issue: ​https://github.com/django/django/pull/1393

[Preston Holmes]

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.

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

[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

[Marc Tamlyn]

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.

[ellisd23]

I've added a test. The specific commit is here: ​https://github.com/dellis23/django/commit/c3098afb0ba240e1195d4dce03146089717fe74e

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

[Anvesh Mishra]

We should also take in account #27704.