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: | 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)
Change History (30)
comment:1 Changed 17 years ago by
comment:2 Changed 17 years ago by
Sorry, the previous text was meant for a different bug. Please ignore it ...
comment:3 Changed 17 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
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
Resolution: | invalid |
---|---|
Status: | closed → reopened |
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 follow-up: 9 Changed 17 years ago by
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
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
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
Cc: | mattimustang@… added |
---|
Would it make sense to combine 'choices' and 'limit_choices_to' into one field option?
comment:9 Changed 17 years ago by
Cc: | gary.wilson@… added |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
Replying to jkocherhans:
What if
limit_choices_to
was deprecated in favor of passing aQuerySet
as thechoices
argument?
This would be cool.
comment:10 Changed 16 years ago by
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
comment:11 Changed 16 years ago by
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
Has patch: | set |
---|---|
Needs documentation: | set |
Patch needs improvement: | set |
comment:13 Changed 16 years ago by
Keywords: | nfa-fixed added; qs-rf removed |
---|---|
Triage Stage: | Design decision needed → Fixed 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
Triage Stage: | Fixed on a branch → Accepted |
---|
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
Keywords: | nfa-fixed removed |
---|
comment:16 Changed 14 years ago by
Cc: | Aurelio Tinio added |
---|
comment:17 Changed 13 years ago by
Component: | Core framework → Database layer (models, ORM) |
---|
comment:18 Changed 13 years ago by
Severity: | normal → Normal |
---|---|
Type: | defect → New feature |
#11707 is a bit related.
comment:21 Changed 11 years ago by
Cc: | charette.s@… added |
---|
comment:22 Changed 11 years ago by
Status: | reopened → new |
---|
comment:23 Changed 10 years ago by
Owner: | changed from nobody to Chris Wilson |
---|---|
Status: | new → assigned |
comment:24 Changed 10 years ago by
Patch needs improvement: | unset |
---|---|
Summary: | 'distinct':True no longer usable in limit_choices_to → ForeignKey with m2m filter can duplicate foreign model entries in ModelForm/ModelChoiceField |
Type: | New feature → Bug |
Created a patch and test in a pull request.
comment:25 Changed 10 years ago by
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
Cc: | someuniquename@… added |
---|
comment:27 Changed 4 years ago by
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
Owner: | changed from Chris Wilson to Crowley Shaita |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
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
FOREIGN KEY constraints FOREIGN KEY constraints are parsed but are not enforced.