Field choice attribute returns different objects in forms.

[Jaap Roes]

I'm upgrading a project from Django 3.0 to Django 3.1. Running the tests shows 3 unexpected errors. All are the same TypeError: Field 'id' expected a number but got <django.forms.models.ModelChoiceIteratorValue object at 0xdeadbeef>

I can trace this back to this code in the __init__ of a ModelForm.

def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)
    field = self.fields["some_fk_field"]

    new_choices = []
    for k, v in field.choices:
        do_add = True

        if not k:
            new_choices.append((k, v))
            continue

        obj = SomeModel.objects.get(pk=k)  # *CRASH*

        [...snip a bunch logic...]

        if do_add:
            new_choices.append((k, v))

    field.choices = new_choices

We've recently inherited this code, added tests and upgraded all the way from 1.4 to 3.0. This bit of code worked in all previous versions but has started raising errors in 3.1.

[Jaap Roes]

I guess this is technically documented here ​https://docs.djangoproject.com/en/3.1/ref/forms/fields/#django.forms.ModelChoiceIteratorValue and in the release notes (under the Minor features heading).

Replacing the obj = SomeModel.objects.get(pk=k) line with obj = k.instance fixes the issue (and avoids a lookup).

However, it's not a very graceful way of discovering this new feature (luckily we have pretty good coverage in our test suite).

I'm not sure if this is fixable. Maybe it's possible to somehow proxy to the value attribute when evaluating a ModelChoiceIteratorValue in the the context of a database lookup?

At the very least this should be listed under the Backwards incompatible heading in the Django 3.1 release notes.

[Mariusz Felisiak]

Yes is a documented (see ​Iterating relationship choices and ​ModelChoiceIterator.__iter__()) and intended change (see #30998), it also simplifies your code. I guess we could add a short note in the "Backwards incompatible" section. Would you like to prepare a clarification? If not I can add sth.

I'm not sure if this is fixable. Maybe it's possible to somehow proxy to the value attribute when evaluating a ModelChoiceIteratorValue in the the context of a database lookup?

Yes, you can use k.value.

[Jaap Roes]

I'm left wondering if other places that manipulate field.choices will start breaking in unexpected ways.

There's several instances of things like:

form.fields["category"].choices = [(pk, label) for pk, label in form.fields["category"].choices if pk == line.get_category().pk]

and

self.fields["realm"].choices = [(k, v) for (k, v) in self.fields["realm"].choices if str(k) != str(self.exclude.pk)]

or

self.fields["matter"].choices += [(self.data["matter"], str(Matter.objects.get(pk=self.data["matter"], realm=self.realm)),)]

or even

self.fields["clients"].choices = [(c.pk, c.get_name()) for c in Client.objects.filter(realm=self.realm, active=True)]

Our tests don't complain, but that might just be because we're not covering/asserting deep enough. Can you confirm this code (and other code that manipulates / works with choices) will remain functional? Or should we audit the entire codebase to see this new ModelChoiceIteratorValue will break stuff?

P.S. I'm reopening this issue so a PR that adds some clarification to the docs/release notes can target this issue.

[Mariusz Felisiak]

I closed it because we will not revert 67ea35df52f2e29bafca8881e4f356934061644e and we can add small backwards incompatible note that Refs to this ticket.

Our tests don't complain, but that might just be because we're not covering/asserting deep enough.

It shouldn't be bad because ModelChoiceIteratorValue defines __eq__() which compares value. Also, as far as I'm aware overriding choices for forms.ModelMultipleChoiceField and forms.ModelChoiceField is not a documented pattern I would rather override .queryset, see ​Fields which handle relationship.

Or should we audit the entire codebase to see this new ModelChoiceIteratorValue will break stuff?

Yes, you should always audit your code when you bump multiple releases.

[Mariusz Felisiak]

​PR with backward incompatibility note.

[Jaap Roes]

Replying to felixxm:

[...] Also, as far as I'm aware overriding choices for forms.ModelMultipleChoiceField and forms.ModelChoiceField is not a documented pattern I would rather override .queryset, see ​Fields which handle relationship.

Sure, but this pattern is commonly used and I've seen it been recommended on StackOverflow on several occasions. I'll investigate further and check if nothing breaks here. This codebase is over 10 years old and not always written by expert Django developers, so it's not unexpected for anti-patterns to show up.

Or should we audit the entire codebase to see this new ModelChoiceIteratorValue will break stuff?

Yes, you should always audit your code when you bump multiple releases.

I'm only bumping a single release here. The migration from 1.4 to 3.0 has been completed months ago and came with it's own trials and tribulations. I wasn't expecting such breakage from bumping to 3.1.

[Carlton Gibson]

Hi Jaap. In normal cases I would expect manually setting choices to work just fine, since we proxy __str__ &co to the underlying object. The filtering cases are interesting but a .value would resolve the more esoteric ones.

[Jaap Roes]

Replying to Carlton Gibson:

Hi Jaap. In normal cases I would expect manually setting choices to work just fine, since we proxy __str__ &co to the underlying object. The filtering cases are interesting but a .value would resolve the more esoteric ones.

I did some further manual testing and checking and can confirm everything else seems to work fine. Is Django itself not depending on the first item of the choices tuple being a ModelChoiceIteratorValue instance?

Since in some cases the entire choices attribute is set to a list of [(pk, str), ...] I can imagine at some point code internal to Django breaking as the first item doesn't have a .value or .instance attribute. It doesn't seem to break *yet*, but it might? Should I go and wrap things in a ModelChoiceIteratorValue just to be sure?

[Carlton Gibson]

Should I go and wrap things in a ModelChoiceIteratorValue just to be sure?

I wouldn't :)

Choices are pretty much only cast to strings (for rendering)… like maybe there's some other case but it's not coming to mind easily… (and there are a mountain of tests around the form layer…)

At this point I'd rather look at concrete issues that come up that worry about things that might break.

[GitHub <noreply@…>]

In ba6b32e:

Refs #32013 -- Added backward incompatibility note about ModelChoiceIterator changes.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 73d084bf:

[3.1.x] Refs #32013 -- Added backward incompatibility note about ModelChoiceIterator changes.

Backport of ba6b32e5efc4c813ba4432777b3b1743d4205d14 from master

[Jaap Roes]

Would it be possible to for the ModelChoiceIteratorValue to inherit from/behave more like a django.db.models.Value? That way it would behave correctly when used for database lookups (etc.) right?

I could work on a patch if that would be an acceptable addition to Django.

[Carlton Gibson]

Hey Japp. I’m missing the use-case here: ModelChoiceField’s choices are not normally used in DB lookups. Rather you get the value from the field via your form’s cleaned_data. (For this choices aren’t used at all, but the queryset, that is used to check the submitted value is valid.)
AFACS, choices are only used for rendering the widget. Short of a concrete case (which we’re very happy to look at) I don’t think there’s anything to address here.

[Jaap Roes]

Replying to Carlton Gibson:

Hey Japp. I’m missing the use-case here: ModelChoiceField’s choices are not normally used in DB lookups. Rather you get the value from the field via your form’s cleaned_data. (For this choices aren’t used at all, but the queryset, that is used to check the submitted value is valid.)
AFACS, choices are only used for rendering the widget. Short of a concrete case (which we’re very happy to look at) I don’t think there’s anything to address here.

The use case is (partially) in the ticket description. It's code I encountered "in the wild" that caused me to open this ticket in the first place.

Basically it's fetching the object from the choices by pk and then does "a bunch logic" (by calling methods on the object) to figure out if the choice should be in the choices list.

I was mostly wondering if it would be possible to make the ModelChoiceIteratorValue behave like a Value so that code would remain working. It would then also be a great way to issue a deprecation warning and guide users to use ModelChoiceIteratorValue.instance instead. Just as a way to make upgrading a bit more safer/convenient for others.

I'm unsure how common this pattern is, so it might be overkill.

[Carlton Gibson]

Yes, OK, thanks. In that case, yes, I think using .value is the way to go here. For me, extra code wouldn't be worth the complexity. I hope that makes sense.