Fixed #31262 -- Added support for mappings on model fields and ChoiceField's choices.

[ngnpope]

ticket-31262

Supersedes PR #12449.

I've updated this with a commit to fix the performance issue raised by @carltongibson based on my comments in the previous PR. I've tested it with the reproducer that he provided and it reduces the queries from 41 to 11 as expected:

Before	After

I've also added a couple of !fixup commits to roll into the main commit, one has a few further example tweaks in the documentation and the other handles pushing down the normalization of ChoicesMeta subclasses into the new normalize_field_choices() helper since the support for passing these direct as choices was added recently in a2eaea8.

[nessita]

Hi @ngnpope, thanks for this work! I'm a little busy this week with some GSoC duties but I'll get to this review either this or next week.

[nessita]

Thanks for this work!

I'm adding an initial set of comments, mostly focused on docs.

[nessita]

This looks great! I really like the direction of the proposed changes. I added what I would consider a final set of tweaks, which include minimal doc changes and some extra, dedicated tests for the new normalize_field_choices method.

[nessita]

Hi @ngnpope! I've been on vacation but I'll re-review next week. Thanks!

[nessita]

Almost there! I think we are just missing tests for the new utils file (see comment) and a few minor doc extensions.

Let me know if you have time to work on this, otherwise I can push this to the final line. Thank you!

[ngnpope]

Hey @nessita. Am just looking into a few more tweaks, so will get back to you on this soon.

[ngnpope]

Hi @nessita 👋🏻

So I've updated this to add a bunch of explicit tests for normalize_field_choices(). In the process I fixed a few other things which I've detailed below. The whole world of choices stuff has grown additional use cases rather organically over the years and hopefully this will ensure that things are more robust and maintainable for the long term.

[nessita]

Hi @nessita 👋🏻

So I've updated this to add a bunch of explicit tests for normalize_field_choices(). In the process I fixed a few other things which I've detailed below. The whole world of choices stuff has grown additional use cases rather organically over the years and hopefully this will ensure that things are more robust and maintainable for the long term.

Great news! So just to clarify, is this ready for another review @ngnpope?

[ngnpope]

...is this ready for another review...

Yup! Thanks.

[nessita]

...is this ready for another review...

Yup! Thanks.

I'm working on this review now. Do you think that, given the advanced stage of the review, we could squash the many commits into one at this point?

EDIT: I just saw your inline comment about this, so ignore this comment please :-)

[nessita]

We are so so close! I added some comments about a few missing tests, not out of pedantry, but because I think those would help me/readers fully grasp the goal of some of the guards in the match-case block.

I have also re-checked the performance thing and we still are at 11 queries 💃

Then, once the above is completed, I could help in rebasing onto main since I'm about to land a few improvements to the radioselect and select tests (see PR #16977), and that will cause some conflicts.

Thank you!!!

[ngnpope]

Thanks for the latest round of reviews @nessita. I've added a few fixups (not squashed in yet so you can see) and responses to your comments above.

[nessita]

Thanks for the latest round of reviews @nessita. I've added a few fixups (not squashed in yet so you can see) and responses to your comments above.

Excellent, I will re-review today. I've just landed the PR I mentioned earlier, I can rebase and fix conflicts if you are busy. Let me know!

[nessita]

I replied to the comments, I'll work on rebasing onto main and resolving conflicts.

[nessita]

Hey @ngnpope, I rebased onto main, fixed conflicts, and made a new commit mimicking the new/extended tests for nested widgets in the test_radioselect file. Please pull before making your changes and a new push! Thanks :-)

[ngnpope]

Oh no. I rebased too 😂 I'll have a look and attempt to resolve the differences. (Good thing I'm a rebasing guru.)

[nessita]

Oh no. I rebased too joy I'll have a look and attempt to resolve the differences. (Good thing I'm a rebasing guru.)

Sorry, I was trying to help!

[nessita]

I have rebased fixing conflicts and suggested yet a minor rewording of the release notes. @smithdc1 would you fancy taking a look at that (the promotion to the What's New section)?

On the other hand, I want to look at this branch with a different mindset. I have just noticed that this work allows for passing a callable to model field's choices, which would solve ticket #24561 (but a duplicate of that ticket has some notes that I want to re-read and analyze before merging this branch).

And, if we indeed fixed that ticket, we should mention that in the added release notes and link to it and etc.

[ngnpope]

"... an instance of ..." isn't quite correct as an enum is not instantiated. You pass the enum class itself - an instance is an enum member.

[nessita]

Thanks @ngnpope, will tweak that.

On (un)related news, I found out this branch now allows for field choices to be callables (yey) but if they are, every time makemigrations is run, a new migration is created, even without making any chance at all. I'll investigate more tomorrow.

[nessita]

I've been doing extensive testing. Everything looks good except that:

1. There is a behavior change such that: in main passing a callable for a model field choices triggers a check error:

ERRORS:
ticket_31262.TestModel.field1: (fields.E004) 'choices' must be an iterable (e.g., a list or tuple).

but with this branch there is no error. One would think this may be a good news and that we are solving ticket #24561 along the way, but see below.

2. When a given model field choices is a callable, running makemigrations on the unmodified model generates a new migration every time. This is because, as far as I have debugged, the migration autodetector code (in generate_altered_fields -> deep_deconstruct) would generate different representations of the field's choices for the old_field and new_field.

Specifically, suppose a model like this:

from django.db import models

def get_choices():
    return ((i, i) for i in range(3))


class TestModel(models.Model):
    field1 = models.PositiveSmallIntegerField(choices=get_choices, default=1)

The migrations autodetector builds these representations:

old_field_dec = ('django.db.models.PositiveSmallIntegerField', [], {'default': 1, 'choices': [(0, 0), (1, 1), (2, 2)]})
new_field_dec = ('django.db.models.PositiveSmallIntegerField', [], {'default': 1, 'choices': <django.utils.choices.CallableChoiceIterator object at 0x7fbe18f53bd0>})

Given the above, and considering that I don't think it's wise to try to solve ticket #24561 in this same PR, I think we should change normalize_choices so we keep consuming the callables as the code in main was doing (despite this being a sort-of step backwards).

A naïve change is to remove the case Callable() if depth == 0 that returns a CallableChoiceIterator but that has other implications. We need to think how to (at least for now) handle differently choices for model fields vs choices for forms ChoiceField (that does allow callables to be used).

@ngnpope Do you have opinions? Ideas? (I will have to change my vote until we sort this out, just to match the current state of this PR landeability).

I will also (try to) craft a test for the migration autodetector to catch this issue. @felixxm any ideas or suggestions are welcome!

[ngnpope]

I think it's ok to consolidate this behaviour for normalizing choices as we have done to ensure consistency, but not do anything more to address full support for callables in model fields as per ticket-24561. Callable support (as well as incomplete) is undocumented so I don't think we need to worry too much. (Sorry, but I can't dig more into this for a while.)

[nessita]

I agree with @ngnpope and @carltongibson that we shouldn't do anything else in this branch to address full support for callables in model fields.

But, I don't think that we can land this as is because the following items are, IMHO, regressions (despite callable support being undocumented for model field choices):

1. In main, defining a callable for a choices param would result in a check error, and in this branch all checks pass.
2. If someone defines choices in a field/model to be a callable (possible because of item 1), makemigrations generates a new migration for that field/model every single time is run (sort of like endless migrations for the field/model).

I agree with Carlton in that we should go for the minimum viable to maintain the behavior on main while landing this work. In order to do that, and considering that I've narrowed the issue to be caused by the removals of:

        # in Field.__init__
        if isinstance(choices, collections.abc.Iterator):
            choices = list(choices)

        # in Field.deconstruct
            if name == "choices" and isinstance(value, collections.abc.Iterable):
                value = list(value)

EDIT: I have a newer (and better?) proposal in the comment next to this one ⏬

I'm proposing two things: PR #17167 to add a few more related tests in main, and a temporary expansion to normalize_choices so we can optionally unroll iterables for choices in model fields:

diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index fbbb6e59c7..9cc4d2fe2c 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -545,7 +545,7 @@ class Field(RegisterLookupMixin):

     @choices.setter
     def choices(self, value):
-        self._choices = normalize_choices(value)
+        self._choices = normalize_choices(value, unroll=True)

     @cached_property
     def cached_col(self):
diff --git a/django/utils/choices.py b/django/utils/choices.py
index 7732bc3697..498c506be5 100644
--- a/django/utils/choices.py
+++ b/django/utils/choices.py
@@ -18,7 +18,7 @@ class CallableChoiceIterator(ChoiceIterator):
         yield from normalize_choices(self.choices_func())


-def normalize_choices(value, *, depth=0):
+def normalize_choices(value, *, depth=0, unroll=False):
     """Normalize choices values consistently for fields and widgets."""

     match value:
@@ -44,11 +44,11 @@ def normalize_choices(value, *, depth=0):
             # String-like types are iterable, so the guard above ensures that
             # they're handled by the default case below.
             pass
+        case Callable() if unroll or depth == 1:
+            value = value()
         case Callable() if depth == 0:
             # If at the top level, wrap callables to be evaluated lazily.
             return CallableChoiceIterator(value)
-        case Callable() if depth < 2:
-            value = value()
         case _:
             return value

I believe that the ultimate fix to avoid the unrolling of callable choices (and thus also fixing ticket-24561) is to have the migrations' autodetector and serializer to treat callable choices just like how callables for default are handled. Right now the main roadblock I found is that the Serializer._registry would match instances of CallableChoiceIterator against Iterable instead of treat them as functions/methods so they are serialized as FunctionTypeSerializer. I think we all agree this should be a separated PR.

[nessita]

@ngnpope @carltongibson I've been thinking and trying different approaches to solve my latest concern (callable choices for model fields).

Today I put together a solution (that I like better) which does not involve the unrolling of iterable/callable choices for model fields. Instead, I'm proposing changes to a field's deconstruct so we return the wrapped func from inside CallableChoiceIterator (when we have one). This way we avoid generating infinite migrations for callable choices (which brings us a step closer to solving the related ticket) with a solution that I humbly consider more "correct".

I'm also ensuring that the model field's checks for callable choices are still being raised to be consistent with main, though these are easily removable if needed (for example, when someone works on ticket-24561).

My plan would be to, ideally, land this by the end of next week (feature freeze is approaching fast!).
Let me know your thoughts!

[nessita]

I'm landing this now, if any of you have further concerns, please reach out and we'll create follow up PRs. Thanks!

[ngnpope]

Thanks @nessita for getting this across the line and all of the reviews! Sorry, I meant to reply at some point this week, but life…

Small heads-up for next time - it looks like this was squashed down and lost all of the Co-authored-by tags. Also looks as though the Refs #24561 -- … commit was intended to be separate?

[nessita]

Thanks @nessita for getting this across the line and all of the reviews! Sorry, I meant to reply at some point this week, but life…

Small heads-up for next time - it looks like this was squashed down and lost all of the Co-authored-by tags. Also looks as though the Refs #24561 -- … commit was intended to be separate?

Yeah I realized this last night after inspecting the git log. Sorry about that!