Code

Opened 5 years ago

Last modified 3 weeks ago

#11707 new Bug

limit_choices_to on a ForeignKey can render duplicate options in formfield

Reported by: Chris.Wesseling@… Owned by: charstring
Component: Forms Version: master
Severity: Normal Keywords: ForeingKey limit_choices_to, dceu2011
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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)

limit_ForeignKey.patch (3.0 KB) - added by Chris.Wesseling@… 5 years ago.
limit_ForeignKey.2.patch (3.1 KB) - added by Chris.Wesseling@… 4 years ago.
limit_ForeignKey.3.patch (3.1 KB) - added by Chris.Wesseling@… 4 years ago.
update resolving conflict
limit_ForeignKey.4.patch (5.6 KB) - added by charstring 3 years ago.
just removed a the previous fix from the comments
limit_ForeignKey.1.2.X.patch (5.2 KB) - added by charstring 3 years ago.
backported to 1.2.X and refactored to reduce complexity
limit_ForeignKey.trunk.patch (5.2 KB) - added by charstring 3 years ago.
Refactored less complex against trunk
limit_ForeignKey.1.3.X.patch (5.2 KB) - added by charstring 3 years ago.
against 1.3.X branch

Download all attachments as: .zip

Change History (31)

Changed 5 years ago by Chris.Wesseling@…

comment:1 follow-up: Changed 5 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

Changed 4 years ago by Chris.Wesseling@…

comment:2 in reply to: ↑ 1 Changed 4 years ago by anonymous

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.

Changed 4 years ago by Chris.Wesseling@…

update resolving conflict

comment:3 Changed 4 years ago by Chris.Wesseling@…

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 4 years ago by SmileyChris

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:5 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

In [15607]:

Fixed #11707 - limit_choices_to on a ForeignKey can render duplicate options in formfield

Thanks to Chris Wesseling for the report and patch.

comment:6 Changed 3 years ago by lukeplant

In [15610]:

[1.2.X] Fixed #11707 - limit_choices_to on a ForeignKey can render duplicate options in formfield

Thanks to Chris Wesseling for the report and patch.

Backport of [15607] from trunk.

comment:7 follow-up: Changed 3 years ago by lukeplant

In [15791]:

Fixed #15559 - distinct queries introduced by [15607] cause errors with some custom model fields

This patch just reverts [15607] until a more satisfying solution can be
found.

Refs #11707

comment:8 Changed 3 years ago by lukeplant

In [15792]:

[1.2.X] Fixed #15559 - distinct queries introduced by [15607] cause errors with some custom model fields

This patch just reverts [15607] until a more satisfying solution can be
found.

Refs #11707

Backport of [15791] from trunk.

comment:9 Changed 3 years ago by lukeplant

  • Resolution fixed deleted
  • Status changed from closed to 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 3 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

comment:11 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by 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?

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 in reply to: ↑ 11 Changed 3 years ago by anonymous

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 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:14 Changed 3 years ago by DrMeers

  • 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 3 years ago by charstring

  • Owner changed from nobody to charstring
  • Status changed from reopened to new
  • UI/UX unset

comment:16 Changed 3 years ago by charstring

  • Keywords limit_choices_to, dceu2011 added; limit_choices_to removed
  • 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 3 years ago by charstring

just removed a the previous fix from the comments

comment:17 follow-up: Changed 3 years ago by simon29

This issue also breaks ModelChoiceField - MultipleObjectsReturned error

comment:18 in reply to: ↑ 17 Changed 3 years ago by charstring

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 3 years ago by charstring

backported to 1.2.X and refactored to reduce complexity

Changed 3 years ago by charstring

Refactored less complex against trunk

Changed 3 years ago by charstring

against 1.3.X branch

comment:19 follow-up: Changed 3 years ago 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.

comment:20 in reply to: ↑ 19 Changed 3 years ago by anonymous

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: Changed 3 years ago 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.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 5 months ago by charstring

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 3 weeks ago by timo

  • 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 in reply to: ↑ 22 Changed 3 weeks ago by charstring

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from charstring to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.