Opened 11 months ago

Closed 8 months ago

Last modified 4 months 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: master
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 Changed 11 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:2 Changed 11 months ago by Aaron Kirkbride

Owner: changed from nobody to Aaron Kirkbride
Status: newassigned

comment:3 Changed 11 months ago by Aaron Kirkbride

Owner: Aaron Kirkbride deleted
Status: assignednew

comment:4 Changed 10 months ago by François Freitag

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

comment:5 Changed 9 months ago by Tim Graham

Patch needs improvement: set

comment:6 Changed 8 months ago by François Freitag

Patch needs improvement: unset

comment:7 Changed 8 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:8 Changed 8 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In f9844f4:

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

comment:9 Changed 8 months ago by 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)

comment:10 Changed 8 months ago by Tim Graham

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

comment:11 Changed 8 months ago by François Freitag

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 8 months ago by François Freitag (previous) (diff)

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

comment:13 Changed 8 months ago by Tim Graham

Resolution: fixed
Status: assignedclosed

comment:14 Changed 4 months ago by Tim Graham

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

PR

comment:15 Changed 4 months ago by GitHub <noreply@…>

In c03e4171:

Refs #28748 -- Reallowed lazy model field choices.

Regression in 3aa9ab39cce6b2a27d6334ad0148c8f37b6f5986.

comment:16 Changed 4 months ago by Tim Graham <timograham@…>

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