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

[rasca]

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?

[rasca]

fixes #13902

[rasca]

fixes #13902, git diff

[rasca]

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!

[Russell Keith-Magee]

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.

[Alex Gaynor]

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.

[rasca]

new patch, checking fields.rel

[rasca]

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

[rasca]

Updating patch for trunk after modifications due to security issues.

[Ramiro Morales]

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'

[rasca]

new patch, this time checking for RelatedObject also

[rasca]

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?

[rasca]

new patch for trunk after modifications in r15286

[Julien Phalip]

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" ;-)

[Ramiro Morales]

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.

[Jacob]

Milestone 1.3 deleted