#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 , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:4 by , 7 years ago
Has patch: | set |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:5 by , 7 years ago
Patch needs improvement: | set |
---|
comment:6 by , 7 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 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 , 7 years ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
comment:11 by , 7 years ago
Has patch: | set |
---|---|
Status: | new → assigned |
Version: | 2.0 → master |
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.
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.
comment:13 by , 7 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:14 by , 6 years ago
The most recent commit caused a regression where lazy()
can no longer be used for choices (found in django-localflavor).
PR