Opened 5 years ago

Closed 4 years ago

Last modified 4 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: master
Severity: Keywords: admin change_list list_filter manytomany
Cc: rasca7@…, CarstenF Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by rasca

fixes #13902

Changed 5 years ago by rasca

fixes #13902, git diff

comment:1 Changed 5 years ago by rasca

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to rasca
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

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 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 5 years ago by Alex

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 Changed 5 years ago by CarstenF

  • Cc CarstenF added

Changed 5 years ago by rasca

new patch, checking fields.rel

comment:5 Changed 5 years ago by rasca

  • milestone set to 1.3

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

Changed 5 years ago by rasca

Updating patch for trunk after modifications due to security issues.

comment:6 Changed 5 years ago by ramiro

  • 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'

Changed 5 years ago by rasca

new patch, this time checking for RelatedObject also

comment:7 Changed 5 years ago by 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?

Changed 5 years ago by rasca

new patch for trunk after modifications in r15286

comment:8 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready 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 Changed 4 years ago by ramiro

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

In [15526]:

(The changeset message doesn't reference this ticket)

comment:10 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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