Named groups in choices are not properly validated

[Scott Stevens]

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.

[François Freitag]

​PR

[Tim Graham <timograham@…>]

In f9844f4:

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

[Matthias Kestenholz]

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)

[François Freitag]

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

[Tim Graham <timograham@…>]

In 3aa9ab3:

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

Regression in f9844f484186fa13399bf8b96f58616687fe158a.

Thanks Matthias Kestenholz for the report and suggestions.

[Tim Graham]

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

​PR

[GitHub <noreply@…>]

In c03e4171:

Refs #28748 -- Reallowed lazy model field choices.

Regression in 3aa9ab39cce6b2a27d6334ad0148c8f37b6f5986.

[Tim Graham <timograham@…>]

In 4ca64f2b:

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

Regression in 3aa9ab39cce6b2a27d6334ad0148c8f37b6f5986.

Backport of c03e41712b2274f524d32bc2aef455ed82c9e3b4 from master