Code

Opened 12 months ago

Last modified 11 months ago

#20749 assigned New feature

Add validation for type of Field.choices arguments

Reported by: ellisd23@… Owned by: ellisd23
Component: Database layer (models, ORM) 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

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.

Attachments (0)

Change History (13)

comment:1 Changed 12 months ago by ellisd23@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 12 months ago by anonymous

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 12 months ago by ellisd23

  • Owner changed from nobody to ellisd23
  • Status changed from new to assigned

comment:4 Changed 12 months ago by ellisd23

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

comment:5 Changed 12 months ago by ellisd23

  • Has patch set

comment:6 Changed 12 months ago by ptone

  • Patch needs improvement set
  • Version changed from 1.5 to 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 Changed 12 months 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 12 months 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: https://github.com/dellis23/django/commit/308d9d10967727395db22757661170467412d6d6

comment:9 Changed 12 months ago by ellisd23

  • Patch needs improvement unset

comment:10 Changed 12 months ago by mjtamlyn

  • 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 Changed 12 months ago by ellisd23

  • Needs tests unset
  • Patch needs improvement unset

comment:12 Changed 12 months ago 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.

comment:13 Changed 11 months ago by timo

  • Cc timograham@… added
  • Has patch unset
  • Summary changed from get_FOO_display does not return proper value or warn for inconsistent type to Add validation for type of Field.choices arguments
  • Type changed from Bug to New feature

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from ellisd23 to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.