Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#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)

16042.diff (2.5 KB ) - added by Thejaswi Puthraya 13 years ago.
git patch against the latest checkout
16042.2.diff (3.0 KB ) - added by Thejaswi Puthraya 13 years ago.
patch against the latest git checkout
16042.3.diff (3.0 KB ) - added by Thejaswi Puthraya 13 years ago.
changes as per hvdklauw's latest comments
16042.4.diff (5.8 KB ) - added by Julien Phalip 13 years ago.
Cleaner, more thorough tests
16042.5.diff (5.9 KB ) - added by Preston Holmes 13 years ago.
patch improved with missing import

Download all attachments as: .zip

Change History (17)

comment:1 by Jannis Leidel, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Thejaswi Puthraya, 13 years ago

Easy pickings: unset
Needs tests: unset
UI/UX: unset
Version: 1.3SVN

comment:3 by Thejaswi Puthraya, 13 years ago

Keywords: dceu2011 added

comment:4 by Thejaswi Puthraya, 13 years ago

Needs tests: set

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.

by Thejaswi Puthraya, 13 years ago

Attachment: 16042.diff added

git patch against the latest checkout

comment:5 by Thejaswi Puthraya, 13 years ago

Needs tests: unset

comment:6 by Julien Phalip, 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 Julien Phalip, 13 years ago

Also, the tests would be more robust if they asserted precise values instead of using the rather vague "assertLess".

by Thejaswi Puthraya, 13 years ago

Attachment: 16042.2.diff added

patch against the latest git checkout

comment:8 by Thejaswi Puthraya, 13 years ago

Patch needs improvement: unset

comment:9 by Harro, 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.

by Thejaswi Puthraya, 13 years ago

Attachment: 16042.3.diff added

changes as per hvdklauw's latest comments

by Julien Phalip, 13 years ago

Attachment: 16042.4.diff added

Cleaner, more thorough tests

by Preston Holmes, 13 years ago

Attachment: 16042.5.diff added

patch improved with missing import

comment:10 by Preston Holmes, 13 years ago

Triage Stage: AcceptedReady 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:11 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [16737]:

Fixed #16042 -- Use the content types caching in the comments contrib app. Thanks, ptone, Julien Phalip and Thejaswi Puthraya.

comment:12 by Simon Charette, 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.

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