#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)
Change History (16)
by , 14 years ago
Attachment: | 13902_distinct_in_changelist.diff added |
---|
comment:1 by , 14 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Version: | 1.2 → 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 by , 14 years ago
Triage Stage: | Unreviewed → 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 by , 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 , 14 years ago
Cc: | added |
---|
by , 14 years ago
Attachment: | 13902_distinct_in_changelist2.diff added |
---|
new patch, checking fields.rel
comment:5 by , 14 years ago
milestone: | → 1.3 |
---|
Uploaded new patch, taking into consideration Alex's note.
by , 14 years ago
Attachment: | 13902_distinct_in_changelist3.patch added |
---|
Updating patch for trunk after modifications due to security issues.
comment:6 by , 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 , 14 years ago
Attachment: | 13902_distinct_in_changelist4.patch added |
---|
new patch, this time checking for RelatedObject also
comment:7 by , 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 , 14 years ago
Attachment: | 13902_distinct_in_changelist5.patch added |
---|
new patch for trunk after modifications in r15286
comment:8 by , 14 years ago
Triage Stage: | Accepted → 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" ;-)
fixes #13902