Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#16042 closed Bug (fixed)

comments framework uses uncached content types

Reported by: Craig de Stigter Owned by: nobody
Component: contrib.comments Version: master
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 5 years ago.
git patch against the latest checkout
16042.2.diff (3.0 KB) - added by Thejaswi Puthraya 5 years ago.
patch against the latest git checkout
16042.3.diff (3.0 KB) - added by Thejaswi Puthraya 5 years ago.
changes as per hvdklauw's latest comments
16042.4.diff (5.8 KB) - added by Julien Phalip 5 years ago.
Cleaner, more thorough tests
16042.5.diff (5.9 KB) - added by Preston Holmes 5 years ago.
patch improved with missing import

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by Jannis Leidel

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 Changed 5 years ago by Thejaswi Puthraya

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

comment:3 Changed 5 years ago by Thejaswi Puthraya

Keywords: dceu2011 added

comment:4 Changed 5 years ago by Thejaswi Puthraya

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.

Changed 5 years ago by Thejaswi Puthraya

Attachment: 16042.diff added

git patch against the latest checkout

comment:5 Changed 5 years ago by Thejaswi Puthraya

Needs tests: unset

comment:6 Changed 5 years ago by Julien Phalip

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

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

Changed 5 years ago by Thejaswi Puthraya

Attachment: 16042.2.diff added

patch against the latest git checkout

comment:8 Changed 5 years ago by Thejaswi Puthraya

Patch needs improvement: unset

comment:9 Changed 5 years ago by Harro

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.

Changed 5 years ago by Thejaswi Puthraya

Attachment: 16042.3.diff added

changes as per hvdklauw's latest comments

Changed 5 years ago by Julien Phalip

Attachment: 16042.4.diff added

Cleaner, more thorough tests

Changed 5 years ago by Preston Holmes

Attachment: 16042.5.diff added

patch improved with missing import

comment:10 Changed 5 years ago by Preston Holmes

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

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

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