Opened 17 years ago

Closed 3 years ago

#1891 closed Bug (fixed)

ForeignKey with m2m filter can duplicate foreign model entries in ModelForm/ModelChoiceField

Reported by: mattimustang@… Owned by: Crowley Shaita
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords:
Cc: mattimustang@…, gary.wilson@…, Aurelio Tinio, charette.s@…, someuniquename@… 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

My app has the following models similar to:

PROXY_GROUPS = ('Terminal Server', 'Socks Proxy', 'Web Proxy')

class Group(models.Model):
    name = models.CharField(maxlength=32)

class Asset(models.Model):
    name = models.CharField(maxlength=32)
    groups = models.ManyToManyField(Group)

class Proxy(models.Model):
    asset = models.ForeignKey(Asset, limit_choices_to={'groups__name__in': PROXY_GROUPS, 'distinct':True})
    port = models.PositiveIntegerField()

Which spews an error when I try to add a Proxy object in the admin. The error is something to do with 'distinct' not being a valid field (Sorry i can post the exact error later).

How can I achieve the equivalent to this in the current post-mr trunk?

From #django earlier today:

malcolmt	mattimustang_: I think you've found a bug. You're right, distinct has become a method on QuerySets, not a filter parameter now. So you may be temporarily stuck. Can you file a ticket so we don't forget to fix this somehow, please?

Attachments (1)

limit_choice_to.diff (5.8 KB) - added by Alex Gaynor 16 years ago.
Patch against qs-rf

Download all attachments as: .zip

Change History (30)

comment:1 Changed 17 years ago by anonymous

Sorry, but sqlite does not enforce foreign keys at all, see http://www.sqlite.org/omitted.html

quoting:

SQL Features That SQLite Does Not Implement

Rather than try to list all the features of SQL92 that SQLite does support, it is much easier to list those that it does not. Unsupported features of SQL92 are shown below.
The order of this list gives some hint as to when a feature might be added to SQLite. Those features near the top of the list are likely to be added in the near future. There are no immediate plans to add features near the bottom of the list.

FOREIGN KEY constraints FOREIGN KEY constraints are parsed but are not enforced.

comment:2 Changed 17 years ago by anonymous

Sorry, the previous text was meant for a different bug. Please ignore it ...

comment:3 Changed 17 years ago by jkocherhans

Resolution: invalid
Status: newclosed

I don't see how distinct=True makes any sense in the context of limit_choices_to. The result of limit_choices_to will have to include the primary key (that's the whole point of ForeignKey) so a SELECT DISTINCT will never return fewer rows than a plain old SELECT. I'm closing this for now. Please re-open it if you think I'm missing something.

comment:4 Changed 17 years ago by mattimustang@…

Resolution: invalid
Status: closedreopened

Try this:

Create an Asset in admin (in the example models above) called proxy1.

Add both Socks Proxy and Web Proxy groups to it.

Then without distinct=True, when you go to create a Proxy object you see the proxy1 Asset twice in the dropdown in the admin. Using distinct=True in Pre-MR django reduced this to 1 proxy1 item in the dropdown.

comment:5 Changed 17 years ago by jkocherhans

Ahhh I see now. groups__name__in is causing a join between the Proxy table and the Asset <-> Group many to many table. The extra Group objects are making extra distinct rows.

Trying to shoehorn all the new manager methods into kwargs is not going to be pretty. What if limit_choices_to was deprecated in favor of passing a QuerySet as the choices argument?

For example:

PROXY_GROUPS = ('Terminal Server', 'Socks Proxy', 'Web Proxy')

class Group(models.Model):
    name = models.CharField(maxlength=32)

class Asset(models.Model):
    name = models.CharField(maxlength=32)
    groups = models.ManyToManyField(Group)

class Proxy(models.Model):
    asset = models.ForeignKey(Asset, choices=Asset.objects.filter('groups__name__in': PROXY_GROUPS).distinct())
    port = models.PositiveIntegerField()

comment:6 Changed 17 years ago by mattimustang@…

Using a a QuerySet is what I discussed with malcomt on #django but we didn't get too far with it. As long as it could handle the choices changing that would be great, ie. adding/removing assets from the PROXY_GROUPS. Would it only take a QuerySet or any callable?

comment:7 Changed 17 years ago by Malcolm Tredinnick <malcolm@…>

I had a "doh!" moment about this bug in the shower this morning: we don't need to make "distinct" something that is specified by the user. We can work out precisely when it is required and just Do The Right Thing(tm).

If the limit_choices_to attribute adds a ForeignKey field to the query that does not have unique=True, then we should be using "select distinct". Otherwise, a simple "select" does the trick. There are some subtleties such as if we include a combination of fields that together comprise a "unique_together" block, then we don't need "distinct" any longer (because it is back to being unique by default).

Although the explanation looks complex, the code is probably not going to be too bad (although adding anything to django/db/models/query.py makes my head hurt). I'll look at writing a patch for this when I get some time (probably on the weekend, since it's queued up behind some other Django work).

comment:8 Changed 17 years ago by mattimustang@…

Cc: mattimustang@… added

Would it make sense to combine 'choices' and 'limit_choices_to' into one field option?

comment:9 in reply to:  5 Changed 17 years ago by Gary Wilson <gary.wilson@…>

Cc: gary.wilson@… added
Triage Stage: UnreviewedDesign decision needed

Replying to jkocherhans:

What if limit_choices_to was deprecated in favor of passing a QuerySet as the choices argument?

This would be cool.

comment:10 Changed 16 years ago by Chris Beaven

Keywords: qs-rf added

Tagging for queryset refactor - maybe something that Malcolm wants to look at. I also like the idea of deprecating limit_choices_to and adding the option of passing a QuerySet to choices

Changed 16 years ago by Alex Gaynor

Attachment: limit_choice_to.diff added

Patch against qs-rf

comment:11 Changed 16 years ago by Alex Gaynor

I have attached a patch which should allow you to pass either a callable or a QuerySet to the choices parameter of a relational field, my one concern however is that it is not possible to use the limit/offset syntax with this because the QuerySet is evaluated and that would cause the queryset to be evauated at load time, rather than each time the choices need to be generated.

comment:12 Changed 16 years ago by Alex Gaynor

Has patch: set
Needs documentation: set
Patch needs improvement: set

comment:13 Changed 16 years ago by Gary Wilson

Keywords: nfa-fixed added; qs-rf removed
Triage Stage: Design decision neededFixed on a branch

On newforms-admin, you can use ModelAdmin.get_form to return a custom from with a custom model choice field that makes use of the queryset argument.

comment:14 Changed 16 years ago by Malcolm Tredinnick

Triage Stage: Fixed on a branchAccepted

That solution only applied to admin. limit-choices-to is used for more than just the admin interface... it's useful wherever you extract the list of options for a ForeignKey.

comment:15 Changed 15 years ago by Brian Rosner

Keywords: nfa-fixed removed

comment:16 Changed 14 years ago by Aurelio Tinio

Cc: Aurelio Tinio added

comment:17 Changed 13 years ago by Julien Phalip

Component: Core frameworkDatabase layer (models, ORM)

comment:18 Changed 13 years ago by Łukasz Rekucki

Severity: normalNormal
Type: defectNew feature

#11707 is a bit related.

comment:19 Changed 12 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:20 Changed 12 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:21 Changed 11 years ago by Simon Charette

Cc: charette.s@… added

comment:22 Changed 11 years ago by Aymeric Augustin

Status: reopenednew

comment:23 Changed 10 years ago by Chris Wilson

Owner: changed from nobody to Chris Wilson
Status: newassigned

comment:24 Changed 10 years ago by Chris Wilson

Patch needs improvement: unset
Summary: 'distinct':True no longer usable in limit_choices_toForeignKey with m2m filter can duplicate foreign model entries in ModelForm/ModelChoiceField
Type: New featureBug

Created a patch and test in a pull request.

comment:25 Changed 10 years ago by Tim Graham

Needs documentation: unset
Patch needs improvement: set

I don't think the overhead of applying distinct in cases where it's not needed is ideal. We should either add logic so that it's not called when it's not needed (as suggested by Malcolm above), or implement a solution that will solve this by allowing a custom queryset to be used for choices (e.g. #13181 or #2445).

comment:26 Changed 10 years ago by Evstifeev Roman

Cc: someuniquename@… added

comment:27 Changed 4 years ago by Josh

Can we add an optional keyword argument to conditionally apply the DISTINCT select? This just bit me on a production app.

Maybe something like:

account_manager = models.ForeignKey(
    User,
    limit_choices_to=Q(groups__name='Sales') | Q(groups__name='Admin'),
    limit_choices_distinct=True,  # default False
)

comment:28 Changed 3 years ago by Mariusz Felisiak

Owner: changed from Chris Wilson to Crowley Shaita
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:29 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 556fa4bb:

Fixed #1891, Fixed #11707 -- Prevented duplicates with limit_choices_to on multi-value relations.

Note: See TracTickets for help on using tickets.
Back to Top