Add system check for field choices using unorderable iterables

[Harry Percival]

Not sure if this is *quite* a bug, but thought I'd mention it since it was a puzzler.

I had an app that seemed to be generatic infinite migrations, ie "makemigrations" would always create new migrations even if I didn't do anything about it.

Eventually I figured out it was (I think) due to Python's non-deterministic dict ordering behaviour (this may be Python 3 only, IIRC said nondetermininstic ordering is to avoid some sort of security thing).

code, eg:

THINGS = {
    'key1': 'val1',
    'key2': 'val2',
    #etc
}

# ...

    myfield = models.CharField(choices=[(k,v) for k,v in THINGS], max_length=255)

I fixed it by using "sorted" on my list comprehension, but I imagine other people might be v confused, and it feels like something ppl are quite likely to do (use a dict for the basis of choices in a field?)

[Harry Percival]

ah, i think it's not just python 3: ​https://mail.python.org/pipermail/python-announce-list/2012-March/009394.html

[Simon Charette]

I'm afraid there's not much we can do about it.

The choices option is meant to be ordered and the migration framework correctly detects changes in ordering.

[Natalia Bidart]

#35854 was a duplicate. ​The recent support added for callables in choices is a nice workaround (#24561).

[Natalia Bidart]

Based on conversations in #35854, we are repurposing and reopening this ticket to grow the choices existing system check to advice against unorderable iterables.

[Natalia Bidart]

Karl, would you like to prepare a patch?

[Karl]

Replying to Natalia Bidart:

Karl, would you like to prepare a patch?

I'll get on it!

[Karl]

I have not been entirely clear on how check error numbers are created. I can see where they're documented but it is not self-evident to me whether there is a pattern to them. Should this be "E013"? Should it just be added into the existing "E004"?

Also, barring testing against known unstable types, does anyone have any clever ways to do this test? I'm going to just check specifically against sets (dicts are something we want to continue to allow, right?). I'll use a list so it can be modified in the future if needed, but that's the only one I'm currently aware of this problem for.

[Natalia Bidart]

Karl: as for the error code, I would use the same code E004 by strengthening the guard in the if condition, and I guess checking against set is reasonable (system checks are best effort):

django/db/models/fields/__init__.py

@@ -318 +318 @@
         if not self.choices:
             return []
 
-        if not isinstance(self.choices, Iterable) or isinstance(self.choices, str):
+        if not isinstance(self.choices, Iterable) or isinstance(self.choices, (str, set)):
             return [
                 checks.Error(
-                    "'choices' must be a mapping (e.g. a dictionary) or an iterable "
-                    "(e.g. a list or tuple).",
+                    "'choices' must be a mapping (e.g. a dictionary) or an stable iterable "
+                    "(e.g. a list or tuple, but not set).",
                     obj=self,
                     id="fields.E004",
                 )

Test are needed though! See tests/invalid_models_tests/test_ordinary_fields.py.
Also please note that docs/ref/checks.txt will need updating.

[Claude Paroz]

Or replace not isinstance(self.choices, Iterable) by not isinstance(self.choices, (Sequence, Mapping))?

[Karl]

I'm cool with just adding to E004.

However, I've run into a new issue that I haven't been able to identify.

My test works if the field uses a set like {1,2}, but not if it is a set of tuples like {("a","A"), ("b","B")}. If I type check right before actually doing the test it is somehow identified as a list. It seems that Django is doing some work on the value of choices before it does the check? Anyone know what's going on there?

[Karl]

For example, this test passes:

def test_unordered_choices(self):
        class Model(models.Model):
            CHOICES_INT = {1,2}
            field = models.IntegerField(choices=CHOICES_INT)

        field = Model._meta.get_field("field")
        self.assertEqual(
            field.check(),
            [
                Error(
                    "'choices' must be a mapping (e.g. a dictionary) or an order-stable "
                    " iterable (e.g. a list or tuple).",
                    obj=field,
                    id="fields.E004",
                ),
            ],
        )

But this one does not (i.e. the check does not get flagged):

def test_unordered_choices(self):
        class Model(models.Model):
            CHOICES_INT = {(1, "2"), (2, "2")}
            field = models.IntegerField(choices=CHOICES_INT)

        field = Model._meta.get_field("field")
        self.assertEqual(
            field.check(),
            [
                Error(
                    "'choices' must be a mapping (e.g. a dictionary) or an order-stable "
                    " iterable (e.g. a list or tuple).",
                    obj=field,
                    id="fields.E004",
                ),
            ],
        )

AssertionError: Lists differ: [] != [<Error: level=40, msg="'choices' must be [135 chars]13'>]

Second list contains 1 additional elements.
First extra element 0:
<Error: level=40, msg="'choices' must be a 'orderable' iterable (e.g. a list or tuple), not 'set'.", hint=None, obj=<django.db.models.fields.IntegerField: field>, id='fields.E013'>

- []
+ [<Error: level=40, msg="'choices' must be a 'orderable' iterable (e.g. a list or tuple), not 'set'.", hint=None, obj=<django.db.models.fields.IntegerField: field>, id='fields.E013'>]

[Karl]

Here's my code so far, open to any ideas: ​https://github.com/WoosterTech/django/tree/ticket_26609