Opened 12 years ago

Closed 12 years ago

Last modified 8 years ago

#4189 closed (fixed)

Fix for COMMENTS_BANNED_USERS_GROUP exploding the ORM when using the templatetag.

Reported by: Brett Hoerner <bretthoerner@…> Owned by: Adrian Holovaty
Component: contrib.comments Version: 0.96
Severity: Keywords: comments, COMMENTS_BANNED_USERS_GROUP
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


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

if not 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 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(

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):
        return self.get_content_object().get_absolute_url() + "#c" + str(
    except AttributeError:
        return ""

Attachments (3) (1.1 KB) - added by Brett Hoerner <bretthoerner@…> 12 years ago.
get_absolute_url() patch (1.1 KB) - added by Brett Hoerner <bretthoerner@…> 12 years ago.
djt-4189-combined.diff (3.2 KB) - added by Jeremy Dunck <jdunck@…> 12 years ago.
Supercedes previous 2 patches.

Download all attachments as: .zip

Change History (8)

Changed 12 years ago by Brett Hoerner <bretthoerner@…>

Attachment: added

get_absolute_url() patch

Changed 12 years ago by Brett Hoerner <bretthoerner@…>

Attachment: added


comment:1 Changed 12 years ago by Jeremy Dunck <jdunck@…>

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.

Changed 12 years ago by Jeremy Dunck <jdunck@…>

Attachment: djt-4189-combined.diff added

Supercedes previous 2 patches.

comment:2 Changed 12 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedReady for checkin

comment:3 Changed 12 years ago by Malcolm Tredinnick

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.

comment:4 Changed 12 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(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.

comment:5 Changed 8 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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