Fixed #31262 -- Allowed dictionaries in Field.choices for named groups.

[orf]

Ticket: https://code.djangoproject.com/ticket/31262

[charettes]

I guess we could use something like the following to perform the instance checks

ordered_mapping_types = (OrderedDict,)
if sys.version_info >= (3, 7):
    ordered_mapping_types += (dict,)

[orf]

That's a really good point @charettes, I'll update the code to check for dict rather than an abstract Mapping.

Regarding <3.7 compatibility, 3.6 has ordered dictionaries. It's an implementation detail and not a language specification like it is in 3.7, but we assume that it's stable and not going to change. I believe that means we can just check for dict here and disregard OrderedDict entirely?

[charettes]

but we assume that it's stable and not going to change.

@orf thanks I didn't know about that assumption.

I believe that means we can just check for dict here and disregard OrderedDict entirely?

Yes based on the above assumption.

[orf]

One potentially sticky situation are callables being passed as choices. They should be able to yield dictionaries, but this makes the implementation for the CallableChoiceIterator a lot less nice :(

So far I've got

    def __iter__(self):
        value = self.choices_func()
        if isinstance(value, dict):
            yield from ChoiceField._flatten_dictionary_choices(value)
            return
        for item in value:
            if isinstance(item, dict):
                yield from ChoiceField._flatten_dictionary_choices(item)
            else:
                yield item

Which is not nice. It's tricky because you cannot just start iterating on the results of choices_func() because if it returns a plain dictionary it will begin iterating the keys.

The documentation about nested dictionaries also gets pretty complex as you're referring to fields and values in two contexts, i.e:

The key of the dictionary is the name to apply to the group, and the value is the choices inside that group. The value consists of another dictionary where the key is the field value, and the value is the human-readable name.

I think I've reworded it pretty well in the pull request, but I think it can be improved.

[smithdc1]

I'm wondering if we need three ways to do the same thing. If we have a preference should we be slightly more opinionated?

Could we even go as far as deprecating the 'old' way. Even if feature is deprecated in Django 4 the current approach would be supported in 3.2 LTS until 2024.

[orf]

Thank you so much for your review @pope1ni, it's looking a lot better now. Supporting the weird iterable-of-dicts format made things less clear for no good benefit.

I've merged your MR into this branch and fixed up a few issues that came up 👍

[carltongibson]

Following up on this, it's a problem with the ModelChoiceField.choices setter. With the change in play this branch is never hit:

    def _get_choices(self):
        # If self._choices is set, then somebody must have manually set
        # the property self.choices. In this case, just return self._choices.
        if hasattr(self, '_choices'):
            return self._choices

@orf @pope1 I might ask you to investigate further if have time. I'll try and fig more too...

[ngnpope]

@carltongibson Can you attach the sample project that you're using?

[carltongibson]

Hey @pope1ni — Yes, I'm trying to reduce it now.

[carltongibson]

Hi both, there's a reproduce with instructions here: https://github.com/carltongibson/issue31262 (Also a diff that fixes it but...)

[ngnpope]

Thanks @carltongibson - that really helped.

I've tracked this down. I was confused by the diff that you presented at first:

diff --git a/django/forms/fields.py b/django/forms/fields.py
index 0728db9922..32153819ff 100644
--- a/django/forms/fields.py
+++ b/django/forms/fields.py
@@ -791,7 +791,7 @@ class ChoiceField(Field):

         # Setting choices on the field also sets the choices on the widget.
         # Note that we bypass the property setter to avoid re-normalizing.
-        self._choices = self.widget._choices = value
+        self._choices = self.widget.choices = value

     def to_python(self, value):
         """Return a string."""

This is actually reverting to the original behaviour. I made this change as we've also added a property to handle normalization of choices if they are set directly on the widget to keep things consistent. Because of that I wanted to avoid re-nomalizing again when the field sets the choices on the widget.

The quick thing was to slap in a print statement here to see what self.widget referred to which resulted in django.contrib.admin.widgets.RelatedFieldWidgetWrapper, and it turns out that inherited from django.forms.Widget and not django.forms.widgets.ChoiceWidget as all the other widgets like SelectWidget do.

So the following alternate solution also fixes the issue:

diff --git a/django/contrib/admin/widgets.py b/django/contrib/admin/widgets.py
index 59d1004d2d..3d01abcfcd 100644
--- a/django/contrib/admin/widgets.py
+++ b/django/contrib/admin/widgets.py
@@ -221,7 +221,7 @@ class ManyToManyRawIdWidget(ForeignKeyRawIdWidget):
         return ','.join(str(v) for v in value) if value else ''
 
 
-class RelatedFieldWidgetWrapper(forms.Widget):
+class RelatedFieldWidgetWrapper(forms.widgets.ChoiceWidget):
     """
     This class is a wrapper to a given widget to add the add icon for the
     admin interface.
@@ -231,9 +231,8 @@ class RelatedFieldWidgetWrapper(forms.Widget):
     def __init__(self, widget, rel, admin_site, can_add_related=None,
                  can_change_related=False, can_delete_related=False,
                  can_view_related=False):
+        super().__init__(widget.attrs, widget.choices)
         self.needs_multipart_form = widget.needs_multipart_form
-        self.attrs = widget.attrs
-        self.choices = widget.choices
         self.widget = widget
         self.rel = rel
         # Backwards compatible check for whether a user can add related

So we could do that - which seems sensible anyway as this widget is essentially doing the same - but there is still the risk of breaking other custom widgets that do not have ChoiceWidget in their hierarchy. The other question is whether this should inherit from some other subclass of ChoiceWidget. That I didn't look into.

[carltongibson]

My suspicion this afternoon was that the only real safe thing is to keep the widget-level normalisation in __init__, where it is now, rather than moving it to the choices property, but I didn't look into that in depth. 🤔

[orf]

I just came back to this but unfortunately I've lost most of the context 🙈. I'd agree with Carlton that it seems safest to keep the widget normalization in __init__ rather than change the hierarchy.

I understand the desire to skip calling normalize_field_choices twice if we can help it, but is this really something that's a huge downside? Some simple benchmarking shows that it's fairly fast:

> from django.utils.crypto import get_random_string
> input_tuple = tuple((get_random_string(), get_random_string()) for i in range(250))
> input_dict = {get_random_string(): get_random_string() for i in range(250)}
> %timeit normalize_field_choices(input_tuple)
2.76 µs ± 197 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
> %timeit normalize_field_choices(input_dict)
109 µs ± 1.26 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

Or am I misunderstanding something?

[ngnpope]

My suspicion this afternoon was that the only real safe thing is to keep the widget-level normalisation in __init__, where it is now, rather than moving it to the choices property, but I didn't look into that in depth. :thinking:

I'd agree with Carlton that it seems safest to keep the widget normalization in __init__ rather than change the hierarchy.

I disagree because it is a common pattern to override Form.__init__() to pass some context, e.g. request.user, and dynamically build choices for certain fields. In this case we'd then not be able to use dictionaries as advertised without documenting and requiring the use of normalize_field_choices() explicitly. That feels like a loss for something that can quite happily work implicitly.

I honestly think what this boils down to is that RelatedFieldWidgetWrapper is a "fudge" to add add/edit/delete icons next to the widget for a related object:

https://github.com/django/django/blob/060e6bdd1f96f46d43fa34636284c20525a96f20/django/contrib/admin/options.py#L173-L175

But what it really does is to supplant an existing more-specific type of widget, i.e. SelectWidget which subclasses ChoicesWidget, with a generic Widget that copies various things from the original widget:

https://github.com/django/django/blob/060e6bdd1f96f46d43fa34636284c20525a96f20/django/contrib/admin/widgets.py#L235-L237

It should be noted that this wrapper explicitly sets self.choices in __init__(). This is because RelatedFieldWidgetWrapper is intended to be used for related fields, the available values of which are made available as a list of available choices, usually via a <select> element. Also of all widgets defined in django.forms.widgets the only widget that sets self.choices is ChoicesWidget. This is why I believe that we should fix the inheritance so that RelatedFieldWidgetWrapper subclasses ChoicesWidget.

Some simple benchmarking shows that it's fairly fast

I guess there is nothing stopping us from re-normalizing in the widget. It just seemed pointless to do it twice. If we take this approach we should add a comment to explain why we are normalizing a second time on the widget - which is to work around a quirk in the admin site. But it feels wrong not to fix what the admin site is doing rather than do the "wrong" thing in the forms framework to work around it.

[nessita]

Hello @orf! I'm doing some PR cleanup. Would you have time to keep working on this? Specifically, if you could rebase onto the latest main, and also resolve conflicts, that would be great. I can then try to push further the review process, since it seems quite advanced already.

Thank you! Natalia.

[nessita]

I think this is a useful feature, and I would not like to see all the effort from the author and the reviewers going to waste. So I took some time to locally branch the PR and rebase it onto main. It was not easy, but hopefully I have properly respected the proposed code and docs changes, though I moved the release notes from 3.1 to 5.0.

[nessita]

If no one beats me to it, next week I will check every review comment to see if they are already solved/applied/answered, and then I'll test it IRL.

[felixxm]

See Carlton's comment.

[nessita]

See Carlton's comment.

Thanks, great pointer! I'll re-read next week and see if I can pick this up, following the plan from Carlton.

[ngnpope]

See Carlton's comment.

Thanks, great pointer! I'll re-read next week and see if I can pick this up, following the plan from Carlton.

Yay - it'd be nice to see this land. See my two comments above for a proposed solution and reasoning. The issue stems from something hacky in the admin which ought to be fixed and not hold back this improvement.

[carltongibson]

Without re-reading all that -- which suggests the point about comments still stands 😅 I can't quite recall the details here but... -- my impression was that this should be resolvable. Thanks for looking at it @nessita!

[ngnpope]

Superseded by #16943.

[nessita]

Hello! I'm re-posting a comment I'm adding to the new PR in case people involved in this PR have mail filters that would flag this PR for further attention but not the new one:

Regarding changing the parent of RelatedFieldWidgetWrapper from Widget to ChoiceWidget:

I read the history around this change and I noticed the separated, clean, commit. Thanks for that. The first question that came to my mind is whether this change should be a separated PR, referencing this issue. My first instinct is that yes, it should be, but I may be missing context or details for which it may be a better idea to have it next to the other changes. Have you given that any consideration? Secondly, if we are pursuing this change (which seems sensible), I think we should audit RelatedFieldWidgetWrapper and remove those methods that are already provided by ChoiceWidget , like the __deepcopy__ implementation. What do you think?

Thanks!