Django

Code

Ticket #4189 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

Fix for COMMENTS_BANNED_USERS_GROUP exploding the ORM when using the templatetag.

Reported by: Brett Hoerner <bretthoerner@bretthoerner.com> Assigned to: adrian
Milestone: 1.0 beta Component: django.contrib.comments
Version: 0.96 Keywords: comments, COMMENTS_BANNED_USERS_GROUP
Cc: Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Using Django comments with COMMENTS_BANNED_USERS_GROUP drops you into the following branch:

if not self.free and settings.COMMENTS_BANNED_USERS_GROUP:
    kwargs['select'] = {'is_hidden': 'user_id IN (SELECT user_id FROM auth_user_groups WHERE group_id = %s)' % settings.COMMENTS_BANNED_USERS_GROUP}
comment_list = get_list_function(**kwargs).order_by(self.ordering + 'submit_date').select_related()

Which in turn shoves the kwarg 'select' into a filter function, thus making the ORM puke violently. A patch is attached:

comment_list = get_list_function(**kwargs).order_by(self.ordering + 'submit_date').select_related()
if not self.free and settings.COMMENTS_BANNED_USERS_GROUP:
    comment_list = comment_list.extra(select={'is_hidden': 'user_id IN (SELECT user_id FROM auth_user_groups WHERE group_id = %s)' % settings.COMMENTS_BANNED_USERS_GROUP})

Also, the Comment/FreeComment models' get_absolute_url() methods can cause a site to fail loudly if an object is erased and latest comments are still referred to. An example is a "latest comments" rail showing comments on different objects. If one of the objects is removed the following code will cause a rendering error, instead of failing silently. (get_content_object returns None and then get_absolute_url explodes)

def get_absolute_url(self):
    return self.get_content_object().get_absolute_url() + "#c" + str(self.id)

This is a problem because non-developers (template creators and/or admin users) can bring a site to a halt using comments (I believe Django prefers templates to fail silently instead of loudly when they can), I recommend a safe/harmless change such as this:

def get_absolute_url(self):
    try:
        return self.get_content_object().get_absolute_url() + "#c" + str(self.id)
    except AttributeError:
        return ""

Attachments

models.py.diff (1.1 kB) - added by Brett Hoerner <bretthoerner@bretthoerner.com> on 04/30/07 14:03:02.
get_absolute_url() patch
comments.py.diff (1.1 kB) - added by Brett Hoerner <bretthoerner@bretthoerner.com> on 04/30/07 14:03:27.
COMMENTS_BANNED_USERS_GROUP / ORM patch
djt-4189-combined.diff (3.2 kB) - added by Jeremy Dunck <jdunck@gmail.com> on 05/09/07 12:39:44.
Supercedes previous 2 patches.

Change History

04/30/07 14:03:02 changed by Brett Hoerner <bretthoerner@bretthoerner.com>

  • attachment models.py.diff added.

get_absolute_url() patch

04/30/07 14:03:27 changed by Brett Hoerner <bretthoerner@bretthoerner.com>

  • attachment comments.py.diff added.

COMMENTS_BANNED_USERS_GROUP / ORM patch

05/09/07 12:39:14 changed by Jeremy Dunck <jdunck@gmail.com>

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

We found one more issue while using comments with 0.96 and settings.COMMENTS_BANNED_USERS_GROUP / settings.COMMENTS_SKETCHY_USERS_GROUP.

I'm attaching a combined patch.

To be clear, the bugs reported here aren't obvious to other people using the comments framework because the framework short-circuits around the problem areas if settings does not include a truth-y related *_USERS_GROUP setting.

There don't seem to be any tests on the comments app, or I'd add tests to cover this particular edge case.

05/09/07 12:39:44 changed by Jeremy Dunck <jdunck@gmail.com>

  • attachment djt-4189-combined.diff added.

Supercedes previous 2 patches.

07/22/07 01:11:18 changed by Simon G. <dev@simon.net.nz>

  • stage changed from Unreviewed to Ready for checkin.

08/11/07 05:29:11 changed by mtredinnick

On balance, I think the handling for get_absolute_url() is incorrect: the function is now (after patch applcation) returning the wrong answer in the 'except' case, because no URL is valid. A more correct solution is to not call get_absolute_url on non-existent objects and so the callers need to be examined. However, given that all of this code is slated for replacement prior to 1.0, it's not worth worrying about this at the moment. I'll apply the patch as a stop-gap until the comments rewrite.

08/11/07 05:50:39 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

(In [5848]) Fixed #4189 -- Fixed crashes in a couple of corner cases in the comments app. Not a perfect fix (see ticket), but it will do as a holdover until the new comments framework is in place.


Add/Change #4189 (Fix for COMMENTS_BANNED_USERS_GROUP exploding the ORM when using the templatetag.)




Change Properties
Action