Opened 17 years ago

Closed 17 years ago

Last modified 12 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

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 (3)

models.py.diff (1.1 KB ) - added by Brett Hoerner <bretthoerner@…> 17 years ago.
get_absolute_url() patch
comments.py.diff (1.1 KB ) - added by Brett Hoerner <bretthoerner@…> 17 years ago.
COMMENTS_BANNED_USERS_GROUP / ORM patch
djt-4189-combined.diff (3.2 KB ) - added by Jeremy Dunck <jdunck@…> 17 years ago.
Supercedes previous 2 patches.

Download all attachments as: .zip

Change History (8)

by Brett Hoerner <bretthoerner@…>, 17 years ago

Attachment: models.py.diff added

get_absolute_url() patch

by Brett Hoerner <bretthoerner@…>, 17 years ago

Attachment: comments.py.diff added

COMMENTS_BANNED_USERS_GROUP / ORM patch

comment:1 by Jeremy Dunck <jdunck@…>, 17 years ago

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.

by Jeremy Dunck <jdunck@…>, 17 years ago

Attachment: djt-4189-combined.diff added

Supercedes previous 2 patches.

comment:2 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedReady for checkin

comment:3 by Malcolm Tredinnick, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Jacob, 12 years ago

milestone: 1.0 beta

Milestone 1.0 beta deleted

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