Opened 11 years ago

Last modified 13 months ago

#20749 new New feature

Add validation for type of Field.choices arguments

Reported by: ellisd23@… 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 ellisd23@…, 11 years ago

Type: UncategorizedBug

comment:2 by anonymous, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by ellisd23, 11 years ago

Owner: changed from nobody to ellisd23
Status: newassigned

comment:4 by ellisd23, 11 years ago

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

comment:5 by ellisd23, 11 years ago

Has patch: set

comment:6 by Preston Holmes, 11 years ago

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 by ellisd23, 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 ellisd23, 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 ellisd23, 11 years ago

Patch needs improvement: unset

comment:10 by Marc Tamlyn, 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 ellisd23, 11 years ago

Needs tests: unset
Patch needs improvement: unset

comment:12 by Tim Graham, 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 Tim Graham, 11 years ago

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 by Tim Graham, 9 years ago

Component: Database layer (models, ORM)Core (System checks)

comment:15 by Mariusz Felisiak, 3 years ago

Owner: ellisd23 removed
Status: assignednew

comment:16 by Anvesh Mishra, 13 months ago

We should also take in account #27704. The validation should also work for ArrayField.base_field.choices imo.

Last edited 13 months ago by Anvesh Mishra (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top