[Django] #32013: TypeError: Field 'id' expected a number but got <django.forms.models.ModelChoiceIteratorValue object at 0xdeadbeef>
Django
<django.forms.models.ModelChoiceIteratorValue object at 0xdeadbeef>
--------------------------------------+------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
<django.forms.models.ModelChoiceIteratorValue object at 0xdeadbeef>
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:1>
Django
<django.forms.models.ModelChoiceIteratorValue object at 0xdeadbeef>
Reporter: Jaap Roes | Owner: nobody
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => invalid
Comment:
Yes is a documented (see
[https://docs.djangoproject.com/en/3.1/ref/forms/fields/#iterating-
relationship-choices "Iterating relationship choices"] and
[https://docs.djangoproject.com/en/3.1/ref/forms/fields/#django.forms.ModelChoiceIterator.__iter__
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:2>
Django
---------------------------+--------------------------------------
Reporter: Jaap Roes | Owner: nobody
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:3>
Django
---------------------------+--------------------------------------
Type: Bug | Status: new
Component: Forms | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jaap Roes):
* status: closed => new
* resolution: invalid =>
Comment:
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)),)]
}}}
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:4>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => invalid
Comment:
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
[https://docs.djangoproject.com/en/3.1/ref/forms/fields/#fields-which-
handle-relationships 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:5>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
[https://github.com/django/django/pull/13430 PR with backward
incompatibility note].
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:6>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jaap Roes):
Replying to [comment:5 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
[https://docs.djangoproject.com/en/3.1/ref/forms/fields/#fields-which-
handle-relationships 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:7>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:8>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jaap Roes):
Replying to [comment:8 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:9>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:10>
Django
---------------------------+--------------------------------------
Component: Forms | Version: 3.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"ba6b32e5efc4c813ba4432777b3b1743d4205d14" ba6b32e]:
{{{
#!CommitTicketReference repository=""
revision="ba6b32e5efc4c813ba4432777b3b1743d4205d14"
Refs #32013 -- Added backward incompatibility note about
ModelChoiceIterator changes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32013#comment:11>