danirus / django-comments-xtd

A pluggable Django comments application with thread support, follow-up notifications, mail confirmation, like/dislike flags, moderation, a ReactJS plugin and Bootstrap 5.3.
https://django-comments-xtd.readthedocs.io
BSD 2-Clause "Simplified" License
595 stars 158 forks source link

Found another issue with #318 #333

Closed Khoding closed 2 years ago

Khoding commented 2 years ago

318 #332 Found another issue while trying the update

It seems that modifying from the admin site still breaks, just adding if qs: before the second qs.update works for comments with a nested count of 1, but will break for anything above that. (error is still the same)

this is what works for nested count of 0 or 1, will break if it's higher:

if qs:
        qs.update(is_public=are_public)
    while len(nested):
        cm_id = nested.pop()
        qs = XtdComment.objects.filter(~Q(pk=cm_id), parent_id=cm_id)
        nested.extend([cm.id for cm in qs])
        if qs:
            qs.update(is_public=are_public)

Sadly this goes beyond my knowledge, so I'll have to let someone who has time and knows Python more than me to make it work for any case.

If I had to think about a solution I'd say we'd have to iterate the whole thing as long as it's below the maximum set thread_level or if it reaches it's nested count, but again I'll let someone who knows stuff do it properly, wouldn't want to make the whole package extremely slow by creating a mess.

Sorry for not trying harder before proposing a fix though, that's on me.

danirus commented 2 years ago

Hi @Khoding, thanks for bringing it up.

At the moment of the failure the MySQLdb connections.py module is trying to run this SQL query:

b' ORDER BY `django_comments_xtd_xtdcomment`.`thread_id` ASC, `django_comments_xtd_xtdcomment`.`order` ASC'

Which obviously is wrong (it starts with ORDER). I will try to get some time to investigate where is the rest of the SQL query. For the time being I would recommend to avoid Django 3.2.

danirus commented 2 years ago

I believe I have found a little more about this issue in the Django Bug Tracker, here.

Khoding commented 2 years ago

Interesting, but I'm not sure that's the reason as I'm using Django 3.2.7 and still having the problem. According to the release notes, it was fixed in version 3.2.1. But it seems to be a similar problem, I'll retry with some versions between 3.2.1 and 3.2.7 tomorrow, maybe it was reintroduced at some point.

danirus commented 2 years ago

The problems happens using the order_by in the method get_queryset of the XtdCommentManager. That code in the get_queryset was meant to accelerate reading the comments, but it doesn't do any favor when running updates on fields not related with the User or the ContentType models.

This issue does not happen with PostgreSQL. It happens with MySQL/MariaDB in Django 3.2 (I only tested in 3.2.7), but not in Django 3.1 or previous versions. So I believed that the changes introduced in Django 3.2, maybe those related with ticket 32654, or maybe from 3.1.x to 3.2.0, are the source of the problem.

If you are using a clone of django-comments-xtd, I would suggest to create a new manager for XtdComment, and use that manager in the publish_or_withhold_nested_comments:

class XtdComment(Comment):
    thread_id = models.IntegerField(default=0, db_index=True)
    parent_id = models.IntegerField(default=0)
    level = models.SmallIntegerField(default=0)
    order = models.IntegerField(default=1, db_index=True)
    followup = models.BooleanField(blank=True, default=False,
                                   help_text=_("Notify follow-up comments"))
    nested_count = models.IntegerField(default=0, db_index=True)
    objects = XtdCommentManager()
    norel_objects = CommentManager()              # <-- this is new.

# Then, replace objects with norel_objects in publish_or_withhold_nested_comments:

def publish_or_withhold_nested_comments(comment, shall_be_public=False):
    qs = get_model().norel_objects.filter(~Q(pk=comment.id),
                                          parent_id=comment.id)
    nested = [cm.id for cm in qs]
    qs.update(is_public=shall_be_public)
    while len(nested):
        cm_id = nested.pop()
        qs = get_model().norel_objects.filter(~Q(pk=cm_id), parent_id=cm_id)
        nested.extend([cm.id for cm in qs])
        qs.update(is_public=shall_be_public)
    # Update nested_count in parents comments in the same thread.
    # The comment.nested_count doesn't change because the comment's is_public
    # attribute is not changing, only its nested comments change, and it will
    # help to re-populate nested_count should it be published again.
    if shall_be_public:
        op = F('nested_count') + comment.nested_count
    else:
        op = F('nested_count') - comment.nested_count
    get_model().norel_objects.filter(thread_id=comment.thread_id,
                                     level__lt=comment.level,
                                     order__lt=comment.order)\
                             .update(nested_count=op)

I will fix it in master and roll a new release soon.

Khoding commented 2 years ago

Nice, good job finding the issue, and thanks for the help.