Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#13902 closed (fixed)

When using a ManyToMany in list_filter, results may apper more than once

Reported by: rasca Owned by: rasca
Component: contrib.admin Version: dev
Severity: Keywords: admin change_list list_filter manytomany
Cc: rasca7@…, Carsten Fuchs Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Suppose you have a ManyToMany relationship between models A and B through a model C.
If you have more than one entry of C relating the same A and B instances, and in the admin in the model A change_list you are list_filtering by the relationship with B, and filter by the B instance that has more than 1 relation to the same A instance, the A instance appears as many times as there are relationships in the list, when it should appear only once.

I'm not sure where to add the .distinct() in admin.views.ChangeList.get_query_set(). We could add it when it returns the final queryset but I'm not sure if that's the most efficient way to do it. We could check to see if it's needed.
There's also another .distinct() before but that one only applies to the search field (in case it's needed). Maybe combine these two?

Attachments (6)

13902_distinct_in_changelist.diff (5.5 KB ) - added by rasca 14 years ago.
fixes #13902
13902_distinct_in_changelist.git.diff (5.5 KB ) - added by rasca 14 years ago.
fixes #13902, git diff
13902_distinct_in_changelist2.diff (6.3 KB ) - added by rasca 14 years ago.
new patch, checking fields.rel
13902_distinct_in_changelist3.patch (6.9 KB ) - added by rasca 14 years ago.
Updating patch for trunk after modifications due to security issues.
13902_distinct_in_changelist4.patch (7.2 KB ) - added by rasca 14 years ago.
new patch, this time checking for RelatedObject also
13902_distinct_in_changelist5.patch (7.4 KB ) - added by rasca 14 years ago.
new patch for trunk after modifications in r15286

Download all attachments as: .zip

Change History (16)

by rasca, 14 years ago

fixes #13902

by rasca, 14 years ago

fixes #13902, git diff

comment:1 by rasca, 14 years ago

Has patch: set
Owner: changed from nobody to rasca
Version: 1.2SVN

Patch 13902_distinct_in_changelist.git.diff fixes the problem (git format).

I don't know why the diff isn't properly showing in trac.
I compared it to http://code.djangoproject.com/attachment/ticket/10918/to_field-10918.git.diff and it has the same format.

This is my first code contribution to Django, so please tell me if there's anything I can do better. Thanks!

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

On the whole, this looks good. Don't worry about the diff display problem -- this is a known issue with Trac; it has something to do with patches that have "No newline at end of file" lines in them. This doesn't actually pose a problem applying the patch, but Trac won't display the patch at all. Don't get too hung up on this problem.

As for the patch: a couple of fairly minor fixes:

  • Constants are in ALLCAPS, variables shouldn't be. Use is_distinct, not DISTINCT.
  • Rather than using unittest.TestCase and manually tearing down the test, it will be faster to use Django's TestCase and use the rollback facilities that it provides.

... and one bigish issue:

I'm not convinced that finding more than one '__' in a list filter is enough of a qualifier. For example, __exact is an implied comparison so multiple_m2mid=1 will only have 1 __, but will be prone to duplicates.

That said, I'm wondering if there's an argument to be made that distinct() should always be applied. There will be a inefficiency in the query if we always apply distinct(), but the cases where it isn't required should be able to optimize out the distinct() at the SQL level.

comment:3 by Alex Gaynor, 14 years ago

I'd be -1 on using distinct() everywhere, there are DB engines that don't support distinct, and list filter should work on them. Also it seems it could use the same logic as here: http://code.djangoproject.com/browser/django/trunk/django/contrib/admin/views/main.py#L208 if there's a MultipleRel it can be duplicate, just call it then.

comment:4 by Carsten Fuchs, 14 years ago

Cc: Carsten Fuchs added

by rasca, 14 years ago

new patch, checking fields.rel

comment:5 by rasca, 14 years ago

milestone: 1.3

Uploaded new patch, taking into consideration Alex's note.

by rasca, 14 years ago

Updating patch for trunk after modifications due to security issues.

comment:6 by Ramiro Morales, 14 years ago

Patch needs improvement: set

The modification to the search checks in the patch is making admin_views tests to fail, these tests are related to search_fields option values that specify a reverse FK accesor:

======================================================================
ERROR: test_search_on_sibling_models (regressiontests.admin_views.tests.AdminSearchTest)
Check that a search that mentions sibling models
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/upstream/tests/regressiontests/admin_views/tests.py", line 1491, in test_search_on_sibling_models
    response = self.client.get('/test_admin/admin/admin_views/recommendation/?q=bar')
  File "django/upstream/django/test/client.py", line 441, in get
    response = super(Client, self).get(path, data=data, **extra)
  File "django/upstream/django/test/client.py", line 225, in get
    return self.request(**r)
  File "django/upstream/django/core/handlers/base.py", line 111, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "django/upstream/django/contrib/admin/options.py", line 298, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "django/upstream/django/utils/decorators.py", line 93, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "django/upstream/django/views/decorators/cache.py", line 79, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "django/upstream/django/contrib/admin/sites.py", line 190, in inner
    return view(request, *args, **kwargs)
  File "django/upstream/django/utils/decorators.py", line 28, in _wrapper
    return bound_func(*args, **kwargs)
  File "django/upstream/django/utils/decorators.py", line 93, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "django/upstream/django/utils/decorators.py", line 24, in bound_func
    return func(self, *args2, **kwargs2)
  File "django/upstream/django/contrib/admin/options.py", line 1034, in changelist_view
    self.list_select_related, self.list_per_page, self.list_editable, self)
  File "django/upstream/django/contrib/admin/views/main.py", line 66, in __init__
    self.query_set = self.get_query_set()
  File "django/upstream/django/contrib/admin/views/main.py", line 261, in get_query_set
    f = self.lookup_opts.get_field(field_name.split('__',1)[0])
  File "django/upstream/django/db/models/options.py", line 279, in get_field
    raise FieldDoesNotExist('%s has no field named %r' % (self.object_name, name))
FieldDoesNotExist: Recommendation has no field named 'titletranslation'

by rasca, 14 years ago

new patch, this time checking for RelatedObject also

comment:7 by rasca, 14 years ago

I've just uploaded a new patch, not sure if it's correct though.

The problem with the test fail was that the model inherited from another concrete model and the get_field() returned FieldDoesNotExist. This are the models from the test:

class Title(models.Model):
    pass

class TitleTranslation(models.Model):
    title = models.ForeignKey(Title)
    text = models.CharField(max_length=100)

class Recommender(Title):
    pass

class Recommendation(Title):
    recommender = models.ForeignKey(Recommender)

class RecommendationAdmin(admin.ModelAdmin):
    search_fields = ('titletranslation__text', 'recommender__titletranslation__text',)

What I now did is to use get_field_by_name() and perform and extra check to see if it's a RelatedObject, in this case isinstance(get_field_by_name('titletranslation')[0], models.related.RelatedObject)

I'm not familiarized with this part of Django, so I have no idea if what I did is correct. If it's not, can you give me some pointers please?

by rasca, 14 years ago

new patch for trunk after modifications in r15286

comment:8 by Julien Phalip, 14 years ago

Triage Stage: AcceptedReady for checkin

The patch looks good and tests are now passing. Just one small suggestion: I think that "use_distinct" (or "do_distinct") would be a better variable name than "is_distinct" ;-)

comment:9 by Ramiro Morales, 14 years ago

Resolution: fixed
Status: newclosed

In [15526]:

Fixed #13902 -- Fixed admin list_filter so it doesn't show duplicate results when it includes a field spec that involves m2m relationships with an intermediate model. Thanks Iván Raskovsky for the report and patch.

comment:10 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

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