[Django] #26609: Spurious migrations cause by non-deteministic field choices order

[Django]

#26609: Spurious migrations cause by non-deteministic field choices order
----------------------------------------------+------------------------
Reporter: hjwp | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.9
Severity: Normal | Keywords: migrations
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+------------------------
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?)

--
Ticket URL: <https://code.djangoproject.com/ticket/26609>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------

Reporter: hjwp | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:

Keywords: migrations | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by hjwp):

* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:1>

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------

Reporter: hjwp | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:

Keywords: migrations | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

Comment (by hjwp):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:2>

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------
Reporter: hjwp | Owner: nobody
Type: Bug | Status: closed

Component: Database layer | Version: 1.9
(models, ORM) |

Severity: Normal | Resolution: wontfix

Keywords: migrations | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by charettes):

* status: new => closed
* resolution: => wontfix

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:3>

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: nobody

Type: Bug | Status: closed
Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution: wontfix

Keywords: migrations | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):

#35854 was a duplicate.
[https://docs.djangoproject.com/en/5.1/releases/5.0/#more-options-for-
declaring-field-choices The recent support added for callables in choices]
is a nice workaround (#24561).
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:4>

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: nobody

Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev

(models, ORM) |
Severity: Normal | Resolution:

Keywords: migrations | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* resolution: wontfix =>
* stage: Unreviewed => Accepted
* status: closed => new
* type: Bug => Cleanup/optimization
* version: 1.9 => dev

Comment:

Based on conversations in #35854, we are repurposing and reopening this
ticket to grow the choices existing system check to advice against
unorderable iterables.
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:5>

[Django]

#26609: Spurious migrations caused by non-deteministic field choices order
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* cc: Karl, Claude Paroz (added)

Comment:

Karl, would you like to prepare a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:6>

[Django]

#26609: Add system check for field choices using unorderable iterables

-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* summary: Spurious migrations caused by non-deteministic field choices
order => Add system check for field choices using unorderable
iterables

--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:7>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Karl):

Replying to [comment:6 Natalia Bidart]:

> Karl, would you like to prepare a patch?

I'll get on it!
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:8>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------

Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned

Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Karl):

* owner: nobody => Karl
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:9>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:10>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by 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):

{{{#!diff
diff --git a/django/db/models/fields/__init__.py
b/django/db/models/fields/__init__.py
index d1f31f0211..add7236a3a 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -318,11 +318,11 @@ class Field(RegisterLookupMixin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:11>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Claude Paroz):

Or replace `not isinstance(self.choices, Iterable)` by `not
isinstance(self.choices, (Sequence, Mapping))`?
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:12>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:13>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by 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'>]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:14>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Karl
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Karl):

Here's my code so far, open to any ideas:
https://github.com/WoosterTech/django/tree/ticket_26609
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:15>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------

Reporter: Harry Percival | Owner: Mariusz
Type: | Felisiak
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* has_patch: 0 => 1
* owner: Karl => Mariusz Felisiak

Comment:

[https://github.com/django/django/pull/20267 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:16>

[Django]

#26609: Add system check for field choices using unorderable iterables
-------------------------------------+-------------------------------------
Reporter: Harry Percival | Owner: Mariusz
Type: | Felisiak

Cleanup/optimization | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by GitHub <noreply@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"97acd4d2f92eef8c285bac070d437bf0fd52e071" 97acd4d2]:
{{{#!CommitTicketReference repository=""
revision="97acd4d2f92eef8c285bac070d437bf0fd52e071"
Fixed #26609 -- Extended fields.E004 system check for unordered iterables.

Co-authored-by: Karl Wooster <karl.w...@alleima.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26609#comment:17>