Re: [Django] #11707: limit_choices_to on a ForeignKey can render duplicate options in formfield
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
Chris.Wesseling@… | Status: reopened
Type: Bug | Component: Forms
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Resolution: | limit_choices_to
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by DrMeers):
* easy: => 0
Comment:
This is nasty because it not only renders duplicates but also blows up
when `.get()` is called on the queryset if you select one of the
duplicates (`MultipleObjectsReturned`).
--
Ticket URL: <http://code.djangoproject.com/ticket/11707#comment:14>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Chris.Wesseling@… | Status: new
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Resolution: | limit_choices_to
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by charstring):
* status: reopened => new
* owner: nobody => charstring
* ui_ux: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: charstring
Chris.Wesseling@… | Status: new
Type: Bug | Component: Forms
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Needs documentation: 0 | Needs tests: 0
-------------------------------------+-------------------------------------
Changes (by charstring):
* keywords: ForeingKey limit_choices_to => ForeingKey limit_choices_to,
dceu2011
* needs_better_patch: 1 => 0
Comment:
Talked to Russ. Picked one of the unclean solutions:
filter in python before displaying and checking again before getting the
choice.
Thanks to Jonas and Roald!
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: charstring
Chris.Wesseling@… | Status: new
Type: Bug | Component: Forms
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Resolution: | limit_choices_to, dceu2011
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by simon29):
This issue also breaks ModelChoiceField - MultipleObjectsReturned error
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: charstring
Chris.Wesseling@… | Status: new
Type: Bug | Component: Forms
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Resolution: | limit_choices_to, dceu2011
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by charstring):
Replying to [comment:17 simon29]:
> This issue also breaks ModelChoiceField - MultipleObjectsReturned error
By "this issue also breaks", do you mean, you've tried the patch and it
needs improvement?
If it does work, please set it to "ready for checkin".
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: charstring
Chris.Wesseling@… | Status: new
Type: Bug | Component: Forms
Milestone: | Severity: Normal
Version: SVN | Keywords: ForeingKey
Resolution: | limit_choices_to, dceu2011
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by jacob):
Discussion from IRC:
{{{
[02:24am] I don't see a test case here that emulates the failures seen
when the previous (committed then reverted) approach. Am I just missing
it?
[09:26am] jacobkm: I also can't say I'm particularly happy with the patch,
particularly iterating over the qs in distinct_choices().
[09:26am] chars:It's pretty hard to test for me. It's a case where
Postgres can't compare the values.
[09:26am] chars: So it can't test for uniqueness
[09:26am] jacobkm: It also needs additions to documentation to mention
that Q() objects are acceptable in limit_choices_to.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:19>
Django
-------------------------------------+-------------------------------------
Type: Bug | charstring
Component: Forms | Status: new
Keywords: ForeingKey | Resolution:
limit_choices_to, dceu2011 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anonymous):
Replying to [comment:19 jacob]:
> Discussion from IRC:
> {{{
> [09:26am] jacobkm: It also needs additions to documentation to mention
that Q() objects are acceptable in limit_choices_to.
> }}}
[https://docs.djangoproject.com/en/1.3/ref/models/fields/#django.db.models.ForeignKey.limit_choices_to
Documentation] on ForeignKey.limit_choices_to already mentions: "Instead
of a dictionary this can also be a Q object for more complex queries."
Further discussion:
{{{
17:00 < chars> jacobkm: The only known case that broke the original
.distinct()
solution was in Postgres. So maybe if #6422 gets accepted,
we
could test for the distinct_on_fields feature and then
distinct
on the pk, which is unique by definition.
17:00 < jacobkm> chars: see now *that* makes me a lot happier.
17:00 < chars> And fallback to the vanilla .distinct() if the backend
doesn't
support it.
}}}
That's #6422.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner:
Type: Bug | charstring
Component: Forms | Status: new
Severity: Normal | Version: SVN
Keywords: ForeingKey | Resolution:
limit_choices_to, dceu2011 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charstring):
DISTINCT is just a special GROUP BY...
So an empty `.annotate()` does the trick too, since it groups by the pk.
And the DBMS should by definition be able to compare pk's.
I'll try to put up a patch tonight.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner:
Type: Bug | charstring
Component: Forms | Status: new
Keywords: ForeingKey | Resolution:
limit_choices_to, dceu2011 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charstring):
Replying to [comment:21 charstring]:
> DISTINCT is just a special GROUP BY...
>
> So an empty `.annotate()` does the trick too, since it groups by the pk.
And the DBMS should by definition be able to compare pk's.
> I'll try to put up a patch tonight.
Well, that was a long night. ;)
I got implemented the `.annotate()` solution in here
https://github.com/CharString/django/tree/ticket-11707
Is the `PointField` mentioned in [comment:12] the same as the one that now
lives in django.contrib.gis?
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner:
Type: Bug | charstring
Component: Forms | Status: new
Severity: Normal | Version: master
Keywords: ForeingKey | Resolution:
limit_choices_to, dceu2011 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
I think the `PointField` in comment 12 is a custom field that's different
from the one in contrib.gis. It's difficult for me to tell from the
comments what the issue was. In any case, I'm going to mark this as "Patch
needs improvement" since it appears it needs additional tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner:
Type: Bug | charstring
Component: Forms | Status: new
Severity: Normal | Version: master
Keywords: ForeingKey | Resolution:
limit_choices_to, dceu2011 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charstring):
Replying to [comment:22 charstring]:
> Is the `PointField` mentioned in [comment:12] the same as the one that
now lives in django.contrib.gis?
No, it isn't. I've installed postgis for this bug. postgis points *can* be
tested on equality.. the `PointField` in [comment:12] uses the builtin
postgres point type, *not* the postgis point type that django.crontib.gis
does.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:24>
Django
-------------------------------------+-------------------------------------
| Shaita
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage: Accepted
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Chris Wesseling => Alvin Shaita
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:25>
Django
-------------------------------------+-------------------------------------
| Shaita
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
limit_choices_to, dceu2011 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage: Ready for
limit_choices_to, dceu2011 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Crowley Shaita):
The solution is here [https://github.com/django/django/pull/13315]
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Ready for checkin => Accepted
Comment:
Unchecking ''Patch needs improvement'' so that this goes back into the
review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: ForeingKey | Triage Stage: Accepted
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Component: Forms | Version: master
Severity: Normal | Resolution:
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
Patch is not merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Crowley, please stop changing a triage stage without any reason.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage: Accepted
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Crowley Shaita):
Replying to [comment:32 felixxm]:
> Crowley, please stop changing a triage stage without any reason.
Sorry, I thought that's how I would get it to be reviewed.
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage: Accepted
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ForeingKey | Triage Stage: Accepted
limit_choices_to, dceu2011 |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
limit_choices_to, dceu2011 | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: Chris.Wesseling@… | Owner: Crowley
| Shaita
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: ForeingKey | Triage Stage: Ready for
limit_choices_to, dceu2011 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"556fa4bbba5ba86bc1646a86fb11ab55405d4aa4" 556fa4bb]:
{{{
#!CommitTicketReference repository=""
revision="556fa4bbba5ba86bc1646a86fb11ab55405d4aa4"
Fixed #1891, Fixed #11707 -- Prevented duplicates with limit_choices_to on
multi-value relations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11707#comment:37>