Code

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#16042 closed Bug (fixed)

comments framework uses uncached content types

Reported by: cdestigter 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 3 years ago.
git patch against the latest checkout
16042.2.diff (3.0 KB) - added by thejaswi_puthraya 3 years ago.
patch against the latest git checkout
16042.3.diff (3.0 KB) - added by thejaswi_puthraya 3 years ago.
changes as per hvdklauw's latest comments
16042.4.diff (5.8 KB) - added by julien 3 years ago.
Cleaner, more thorough tests
16042.5.diff (5.9 KB) - added by ptone 3 years ago.
patch improved with missing import

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by jezdez

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by thejaswi_puthraya

  • Easy pickings unset
  • Needs tests unset
  • UI/UX unset
  • Version changed from 1.3 to SVN

comment:3 Changed 3 years ago by thejaswi_puthraya

  • Keywords dceu2011 added

comment:4 Changed 3 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 3 years ago by thejaswi_puthraya

git patch against the latest checkout

comment:5 Changed 3 years ago by thejaswi_puthraya

  • Needs tests unset

comment:6 Changed 3 years ago by julien

  • 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 3 years ago by julien

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

Changed 3 years ago by thejaswi_puthraya

patch against the latest git checkout

comment:8 Changed 3 years ago by thejaswi_puthraya

  • Patch needs improvement unset

comment:9 Changed 3 years ago by hvdklauw

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 3 years ago by thejaswi_puthraya

changes as per hvdklauw's latest comments

Changed 3 years ago by julien

Cleaner, more thorough tests

Changed 3 years ago by ptone

patch improved with missing import

comment:10 Changed 3 years ago by ptone

  • Triage Stage changed from Accepted to 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:11 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16737]:

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

comment:12 Changed 2 years ago by charettes

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.