Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#28748 closed Bug (fixed)

Named groups in choices are not properly validated

Reported by: Scott Stevens Owned by: François Freitag
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: choices
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using named groups for the choices attribute, I accidentally created them incorrectly, like so:

tournament = models.PositiveSmallIntegerField(db_index=True, choices=(
    (4, 'g'),
    ('NamedTuple', (5, 'h'), ),
))

Given that specifying choices as (4, 'g'), ('NamedTuple', (5, 'h'), (6, 'i'), ), raises E.005 (as does 1, 2, 3,), I would expect the same result for these malformed named groups.

Instead, it allows ./manage.py makemigrations to run successfully (the check is not performed when launching ./manage.py shell, which might be its own issue). However, it raises this exception when performing a full_clean() on a model and thus validating the field in question (assuming the field has been set):

  File "...\django\db\models\base.py", line 1144, in full_clean
    self.clean_fields(exclude=exclude)
  File ...\django\db\models\base.py", line 1186, in clean_fields
    setattr(self, f.attname, f.clean(raw_value, self))
  File "...\django\db\models\fields\__init__.py", line 607, in clean
    self.validate(value, model_instance)
  File "...\django\db\models\fields\__init__.py", line 583, in validate
    for optgroup_key, optgroup_value in option_value:
TypeError: 'int' object is not iterable

The flatchoices attribute ends up as [5, 'h', (4, 'g')].

It is my understanding that this behaviour is unintended, as the _check_choices() method performs validation on the choices attribute.

Specifically, this clause

elif any(isinstance(choice, str) or
    not is_iterable(choice) or len(choice) != 2
    for choice in self.choices):
  return [
    checks.Error(
      "'choices' must be an iterable containing "
      "(actual value, human readable name) tuples.",
      obj=self,
      id='fields.E005',
    )
  ]

Does not check beyond the length of each choice, even if that "choice" is a named group.

This can be further seen with this set of choices:

tournament = models.PositiveSmallIntegerField(db_index=True, choices=(
  (4, 'g'),
  ('NamedTuple', (
    (5, 'h'),
    (2, 3, 4, ),
  )),
))

Migrations are again created successfully, but when validating the field, it raises
ValueError: too many values to unpack (expected 2)
on the same line as before.

Change History (16)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Aaron Kirkbride, 7 years ago

Owner: changed from nobody to Aaron Kirkbride
Status: newassigned

comment:3 by Aaron Kirkbride, 7 years ago

Owner: Aaron Kirkbride removed
Status: assignednew

comment:4 by François Freitag, 7 years ago

Has patch: set
Owner: set to François Freitag
Status: newassigned

comment:5 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:6 by François Freitag, 7 years ago

Patch needs improvement: unset

comment:7 by Carlton Gibson, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In f9844f4:

Fixed #28748 -- Made model field choices check more strict for named groups.

comment:9 by Matthias Kestenholz, 7 years ago

This patch regresses a common use case of adding lazily translated choices. Just apply the following patch:

(.env) ~/Projects/django/tests$ git diff                                                                                                                                                           
diff --git a/tests/i18n/models.py b/tests/i18n/models.py                                                                                                                                           
index 15b4de57b6..fc801c71cf 100644
--- a/tests/i18n/models.py
+++ b/tests/i18n/models.py
@@ -16,3 +16,10 @@ class Company(models.Model):
 
     class Meta:
         verbose_name = _('Company')
+
+
+class TranslatedChoicesModel(models.Model):
+    choice = models.CharField(max_length=10, choices=[
+        ('a', _('a')),
+        ('b', _('b')),
+    ])

Then, run the testsuite:

(.env) ~/Projects/django/tests$ ./runtests.py i18n                                                                                                                                                 
Testing against Django installed in '/home/matthias/Projects/django/django' with up to 4 processes                                                                                                 
Creating test database for alias 'default'...                                                                                                                                                      
Cloning test database for alias 'default'...                                                                                                                                                       
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Creating test database for alias 'other'...
Cloning test database for alias 'other'...
Cloning test database for alias 'other'...
Cloning test database for alias 'other'...
Cloning test database for alias 'other'...
Traceback (most recent call last):
  File "./runtests.py", line 473, in <module>
    options.exclude_tags,
  File "./runtests.py", line 278, in django_tests
    extra_tests=extra_tests,                                                                                                                                                                       
  File "/home/matthias/Projects/django/django/test/runner.py", line 599, in run_tests                                                                                                              
    self.run_checks()                                                                                                                                                                              
  File "/home/matthias/Projects/django/django/test/runner.py", line 561, in run_checks
    call_command('check', verbosity=self.verbosity)
  File "/home/matthias/Projects/django/django/core/management/__init__.py", line 141, in call_command
    return command.execute(*args, **defaults)
  File "/home/matthias/Projects/django/django/core/management/base.py", line 335, in execute
    output = self.handle(*args, **options)
  File "/home/matthias/Projects/django/django/core/management/commands/check.py", line 65, in handle
    fail_level=getattr(checks, options['fail_level']),
  File "/home/matthias/Projects/django/django/core/management/base.py", line 410, in check
    raise SystemCheckError(msg)
django.core.management.base.SystemCheckError: SystemCheckError: System check identified some issues:

ERRORS:
i18n.TranslatedChoicesModel.choice: (fields.E005) 'choices' must be an iterable containing (actual value, human readable name) tuples.

System check identified 1 issue (0 silenced).

I noticed this because I'm running the feincms3 testsuite with Django@master as well (https://travis-ci.org/matthiask/feincms3/jobs/326453581)

A possible fix might include checking for Promise instances as well in is_value:

    def _check_choices(self):
        from django.utils.functional import Promise
        if not self.choices:
            return []

        def is_value(value):
            # Run this:
            return isinstance(value, str) or isinstance(value, Promise) or not is_iterable(value)
            # instead of this:
            # return isinstance(value, str) or not is_iterable(value)

comment:10 by Tim Graham, 7 years ago

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

comment:11 by François Freitag, 7 years ago

Has patch: set
Status: newassigned
Version: 2.0master

Thank you for the detailed bug report! I added the check for the Promise type as you suggested.

It made me realize that bytes should probably be allowed in the choices as well.

PR

Edit: choices should not allow bytes. CharField.to_python() calls str() on the value. Providing b'foo' would result in "b'foo'" stored in the database. Thanks to Simon Charette for the correction.

Last edited 7 years ago by François Freitag (previous) (diff)

comment:12 by Tim Graham <timograham@…>, 7 years ago

In 3aa9ab3:

Refs #28748 -- Reallowed lazy values in model field choices.

Regression in f9844f484186fa13399bf8b96f58616687fe158a.

Thanks Matthias Kestenholz for the report and suggestions.

comment:13 by Tim Graham, 7 years ago

Resolution: fixed
Status: assignedclosed

comment:14 by Tim Graham, 6 years ago

The most recent commit caused a regression where lazy() can no longer be used for choices (found in django-localflavor).

PR

comment:15 by GitHub <noreply@…>, 6 years ago

In c03e4171:

Refs #28748 -- Reallowed lazy model field choices.

Regression in 3aa9ab39cce6b2a27d6334ad0148c8f37b6f5986.

comment:16 by Tim Graham <timograham@…>, 6 years ago

In 4ca64f2b:

[2.1.x] Refs #28748 -- Reallowed lazy model field choices.

Regression in 3aa9ab39cce6b2a27d6334ad0148c8f37b6f5986.

Backport of c03e41712b2274f524d32bc2aef455ed82c9e3b4 from master

Note: See TracTickets for help on using tickets.
Back to Top