#16042 closed Bug (fixed)
comments framework uses uncached content types
Reported by: | Craig de Stigter | Owned by: | nobody |
---|---|---|---|
Component: | contrib.comments | Version: | dev |
Severity: | Normal | Keywords: | dceu2011 |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
We found our list views were doing too many database queries for content types, even with caching turned on.
Turns out the comments framework is to blame, for using the uncached method ContentType.objects.get(...)
This one-liner fixes it:
--- a/django/contrib/comments/templatetags/comments.py +++ b/django/contrib/comments/templatetags/comments.py @@ -49,7 +49,7 @@ class BaseCommentNode(template.Node): def lookup_content_type(token, tagname): try: app, model = token.split('.') - return ContentType.objects.get(app_label=app, model=model) + return ContentType.objects.get_by_natural_key(app, model) except ValueError: raise template.TemplateSyntaxError("Third argument in %r must be in the format 'app.model'" % tagname) except ContentType.DoesNotExist:
Attachments (5)
Change History (17)
comment:1 by , 13 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 13 years ago
Easy pickings: | unset |
---|---|
Needs tests: | unset |
UI/UX: | unset |
Version: | 1.3 → SVN |
comment:3 by , 13 years ago
Keywords: | dceu2011 added |
---|
comment:4 by , 13 years ago
Needs tests: | set |
---|
comment:5 by , 13 years ago
Needs tests: | unset |
---|
comment:6 by , 13 years ago
Patch needs improvement: | set |
---|
The patch is looking good but the tests are raising a warning:
UserWarning: A {% csrf_token %} was used in a template, but the context did not provide the value. This is usually caused by not using RequestContext.
comment:7 by , 13 years ago
Also, the tests would be more robust if they asserted precise values instead of using the rather vague "assertLess
".
comment:8 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 13 years ago
small remark on the tests. Store the current DEBUG value in the setUp on self and restore it in tearDown, now you assume it's False when you start the tests.
comment:10 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Other than a missing model import in the tests (which I fixed in 16042.5), the submitted patch seems to solve the original problem - and demonstrates via tests as requested in previous comments.
comment:12 by , 13 years ago
As #17256 and calls such as
# Force the CT to be cached ct = ContentType.objects.get_for_model(Article)
in r16737 tests highlight that get_by_natural_key
doesn't actually cache anything.
#17256's currently attached patch make sure get_by_natural_key
actually cache cts and remove the needs for get_for_model
calls in the tests.
As per a discussion with freakyboy3742 on IRC, this requires a test that makes use of assertNumQueries to show the number of queries did in fact reduce.