#25396 closed New feature (duplicate)
ModelAdmin.get_queryset doesn't accept ValueQuerySet
Reported by: | Piotr Roszatycki | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Django Admin crashes for following code:
models.py
from django.db import models from django.contrib.auth.models import User class Category(models.Model): name = models.CharField(max_length=100) class Meta: verbose_name_plural = 'categories' def __unicode__(self): return self.name class Note(models.Model): user = models.ForeignKey(User) category = models.ForeignKey(Category) keywords = models.CharField(max_length=400) title = models.CharField(max_length=200) date = models.DateTimeField() content = models.TextField(max_length=50000) def __unicode__(self): return self.title class UserNotesReport(models.Model): user = models.CharField(max_length=30, primary_key=True) category = models.CharField(max_length=100, primary_key=True) n = models.IntegerField()
admin.py
from django.contrib import admin import hello_blog.models as models from django.db.models import Count class CategoryAdmin(admin.ModelAdmin): pass admin.site.register(models.Category, CategoryAdmin) class NoteAdmin(admin.ModelAdmin): pass admin.site.register(models.Note, NoteAdmin) class UserNotesReportAdmin(admin.ModelAdmin): actions = None list_display = ['user__username', 'category__name', 'n'] list_display_links = None readonly_fields = ['user', 'category', 'n'] def user__username(self): pass def category__name(self): pass def get_queryset(self, request): qs = models.Note.objects.order_by('user', 'category').values('user__username', 'category__name').annotate(n=Count('*')) print(qs.query) return qs def has_add_permission(self, request): return False admin.site.register(models.UserNotesReport, UserNotesReportAdmin)
The fix is trivial:
diff --git a/django/contrib/admin/templatetags/admin_list.py b/django/contrib/admin/templatetags/admin_list.py index f4033e2..9e70f8c 100644 --- a/django/contrib/admin/templatetags/admin_list.py +++ b/django/contrib/admin/templatetags/admin_list.py @@ -199,7 +199,10 @@ def items_for_result(cl, result, form): empty_value_display = cl.model_admin.get_empty_value_display() row_classes = ['field-%s' % field_name] try: - f, attr, value = lookup_field(field_name, result, cl.model_admin) + if isinstance(result, dict): + f, attr, value = None, field_name, result.get(field_name) + else: + f, attr, value = lookup_field(field_name, result, cl.model_admin) except ObjectDoesNotExist: result_repr = empty_value_display else:
Change History (9)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Oh, it is probably the duplicate of this bug.
Yes, this is for GROUP BY statement and not only for related model (it might be a simple field, too).
Django Admin view might not be the best option for showing aggregated queryset, but as far as the fix is very simple, I think it might be supported. And other options are still worse: creating view in raw SQL, using dedicated view (with pagination, filters, actions, oh my... it would be a huge rewrite of admin stuff).
comment:3 by , 9 years ago
I tried to override Admin's internals so it is possible to use this feature with official Django 1.8.4 (with or without Django Suit template). The result is https://gist.github.com/dex4er/c658c38c3a12df7b92fc
As you can see there is a lot of duplicated code: 2 meaning lines of code and 153 lines of copy-and-paste :(
comment:4 by , 9 years ago
I understand your use case but I'm not sure simply allowing get_queryset()
to return a ValueQuerySet
will do here.
From what I understand this will break many things since your results omit the pk
from the results. Your provided examples are good example of customization required to make this work (e.g list_display
mapping to methods, has_add_permission
returning False
) and silence strange errors the admin code might be raising.
I'm still not convinced we should accept this new feature, even with a defined use case.
comment:5 by , 9 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Yes, I think when you find yourself overriding large portions of the admin, it's time to write your own views.
comment:6 by , 9 years ago
Writing own view which supports paging, filtering, actions, ordering and import/export (via additional packages) means that I need to copy-and-paste large portions of the admin view or need to reinvent the wheel.
I think adding a few methods and settings to ModelAdmin is still better option.
This patch doesn't break anything else in Django and just allows to make some new use cases for admin interface...
comment:7 by , 9 years ago
I believe a different API to construct the desired query might be the proper solution.
comment:8 by , 9 years ago
I'm not sure what do you mean "different API".
I've refactored this example use case and created "database view" with model.Manager
. The fields are aliased with annotate(aliased_field=F('original_field'))
.
This solution works also with django-import-export, so I can generate CSV or XLS file based on this aggregated QuerySet
!
models.py
from django.db import models from django.db.models import Count, F from django.contrib.auth.models import User class Category(models.Model): name = models.CharField(max_length=100) class Meta: verbose_name_plural = 'categories' class Note(models.Model): user = models.ForeignKey(User) category = models.ForeignKey(Category) keywords = models.CharField(max_length=400) title = models.CharField(max_length=200) date = models.DateTimeField() content = models.TextField(max_length=50000) class UserNotesReportManager(models.Manager): def get_queryset(self): return Note.objects \ .annotate(user_username=F('user__username'), category_name=F('category__name')) \ .values('user_username', 'category_name') \ .order_by('user_username', 'category_name') \ .annotate(n=Count('*')) class UserNotesReport(models.Model): user_username = models.CharField(max_length=30, primary_key=True) category_name = models.CharField(max_length=100, primary_key=True) n = models.IntegerField() objects = UserNotesReportManager()
admin.py
from django.contrib import admin import hello_blog.models as models class CategoryAdmin(admin.ModelAdmin): pass admin.site.register(models.Category, CategoryAdmin) class NoteAdmin(admin.ModelAdmin): pass admin.site.register(models.Note, NoteAdmin) class UserNotesReportAdmin(admin.ModelAdmin): actions = None list_display = ['user_username', 'category_name', 'n'] list_display_links = None readonly_fields = list_display def has_add_permission(self, request): return False admin.site.register(models.UserNotesReport, UserNotesReportAdmin)
This example is available at https://github.com/dex4er/django-hello-world/tree/blog_reports so you can check if the modification works correctly.
comment:9 by , 9 years ago
I mean a different way to construct the GROUP BY query without using values()
.
This looks like a duplicate of #24387 to me.
From what I understand you are adding this explicit
values()
call to GROUP BY related username and category?