Opened 14 years ago
Closed 3 years ago
#11707 closed Bug (fixed)
limit_choices_to on a ForeignKey can render duplicate options in formfield
Reported by: | Owned by: | Crowley Shaita | |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | ForeingKey limit_choices_to, dceu2011 |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If you pass a Q object as limit_choices_to on a ForeignKey field involving a join, you may end up with duplicate options in your form.
See regressiontest in patch for a clear view on the problem.
Attachments (7)
Change History (44)
Changed 14 years ago by
Attachment: | limit_ForeignKey.patch added |
---|
comment:1 follow-up: 2 Changed 14 years ago by
Triage Stage: | Unreviewed → Ready for checkin |
---|
Changed 14 years ago by
Attachment: | limit_ForeignKey.2.patch added |
---|
comment:2 Changed 14 years ago by
comment:3 Changed 14 years ago by
Is there something I can do to get this checked in? I re-read the Triage docs. As far as I can see "A developer checks in the fix" is the only step left.
comment:4 Changed 14 years ago by
The 1.2 roadmap shows that we're in a feature freeze. I'd suggest bringing this up on the django-dev google group a week or so after 1.2 final is released.
comment:9 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Re-opened due to the fix being reverted, as above. For future reference, a possible alternative solution might be to do filtering of duplicates in Python, at the point of rendering the form field, rather than in the database.
comment:10 Changed 13 years ago by
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
comment:11 follow-up: 12 Changed 13 years ago by
Replying to lukeplant:
(The changeset message doesn't reference this ticket)
Can someone point me to an example of such a custom model field or, even better, a test showing the breakage?
Replying to lukeplant:
For future reference, a possible alternative solution might be to do filtering of duplicates in Python, at the point of rendering the form field, rather than in the database.
Assuming 'limit_choices_to' is only used by Forms...
comment:12 Changed 13 years ago by
Replying to charstring:
Replying to lukeplant:
(The changeset message doesn't reference this ticket)
Can someone point me to an example of such a custom model field or, even better, a test showing the breakage?
The discussion linked from the description of the other ticket has an example. It's in dpaste so may not be long-lived. Copying here for reference:
class PointField(models.Field): description = _("A geometric point") __metaclass__ = models.SubfieldBase pattern = re.compile('^\(([\d\.]+),([\d\.]+)\)$') def db_type(self, connection): if connection.settings_dict['ENGINE'] is not 'django.db.backends.postgresql_psycopg2': return None return 'point' def to_python(self, value): if isinstance(value, tuple): return (float(value[0]), float(value[1])) if not value: return (0, 0) match = self.pattern.findall(value)[0] return (float(match[0]), float(match[1])) def get_prep_value(self, value): return self.to_python(value) def get_db_prep_value(self, value, connection, prepared=False): # Casts dates into the format expected by the backend if not prepared: value = self.get_prep_value(value) return '({0}, {1})'.format(value[0], value[1]) def get_prep_lookup(self, lookup_type, value): raise TypeError('Lookup type %r not supported.' % lookup_type) def value_to_string(self, obj): value = self._get_val_from_obj(obj) return self.get_db_prep_value(value)
comment:13 Changed 13 years ago by
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:14 Changed 13 years ago by
Easy pickings: | unset |
---|
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
).
comment:15 Changed 13 years ago by
Owner: | changed from nobody to Chris Wesseling |
---|---|
Status: | reopened → new |
UI/UX: | unset |
comment:16 Changed 13 years ago by
Keywords: | dceu2011 added |
---|---|
Patch needs improvement: | unset |
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!
Changed 13 years ago by
Attachment: | limit_ForeignKey.4.patch added |
---|
just removed a the previous fix from the comments
comment:17 follow-up: 18 Changed 12 years ago by
This issue also breaks ModelChoiceField - MultipleObjectsReturned error
comment:18 Changed 12 years ago by
Replying to 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".
Changed 12 years ago by
Attachment: | limit_ForeignKey.1.2.X.patch added |
---|
backported to 1.2.X and refactored to reduce complexity
Changed 12 years ago by
Attachment: | limit_ForeignKey.trunk.patch added |
---|
Refactored less complex against trunk
comment:19 follow-up: 20 Changed 12 years ago by
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.
comment:20 Changed 12 years ago by
Replying to jacob:
Discussion from IRC:
[09:26am] jacobkm: It also needs additions to documentation to mention that Q() objects are acceptable in 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.
comment:21 follow-up: 22 Changed 12 years ago by
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.
comment:22 follow-up: 24 Changed 10 years ago by
Replying to 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 12 the same as the one that now lives in django.contrib.gis?
comment:23 Changed 9 years ago by
Patch needs improvement: | set |
---|
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.
comment:24 Changed 9 years ago by
Replying to charstring:
Is the
PointField
mentioned in 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 12 uses the builtin postgres point type, *not* the postgis point type that django.crontib.gis does.
comment:25 Changed 3 years ago by
Owner: | changed from Chris Wesseling to Crowley Shaita |
---|---|
Status: | new → assigned |
comment:28 Changed 3 years ago by
Patch needs improvement: | unset |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Unchecking Patch needs improvement so that this goes back into the review queue.
comment:29 Changed 3 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:31 Changed 3 years ago by
Triage Stage: | Accepted → Unreviewed |
---|
comment:32 follow-up: 33 Changed 3 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
Crowley, please stop changing a triage stage without any reason.
comment:33 Changed 3 years ago by
Replying to felixxm:
Crowley, please stop changing a triage stage without any reason.
Sorry, I thought that's how I would get it to be reviewed.
comment:34 Changed 3 years ago by
Patch needs improvement: | set |
---|
comment:35 Changed 3 years ago by
Patch needs improvement: | unset |
---|
comment:36 Changed 3 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
Replying to SmileyChris:
I've updated the patch to resolve the conflicts I've had since you flagged this one as "Ready for checkin".
No real change.