Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite recursive loop with SafeDeleteQueryset #233

Open
kursat-ozer opened this issue Jun 7, 2023 · 2 comments
Open

Infinite recursive loop with SafeDeleteQueryset #233

kursat-ozer opened this issue Jun 7, 2023 · 2 comments

Comments

@kursat-ozer
Copy link

Hi,

I am having an issue similar to 117

I am using django-treebeard for hierarchical data in DB and deleting an item with descendants should also delete all related descendants. I also want these items to be safely deleted so policy is SOFT_DELETE_CASCADE.

I think I am using the treebeard and safedelete packages in the right way but if you see anything wrong with it please do tell:
Note: The models are slightly different but this is how it is basically.

class FolderQuerySet(MP_NodeQuerySet, SafeDeleteQueryset):
    pass

class Folder(MP_Node, SafeDeleteModel):
    ...
    objects = SafeDeleteManager.from_queryset(FolderQuerySet)()
    all_objects = SafeDeleteAllManager.from_queryset(FolderQuerySet)()
    deleted_objects = SafeDeleteDeletedManager.from_queryset(FolderQuerySet)()

When I try to delete a folder,

  • it first finds all descendants
  • then should safe delete all items and cascade.
    But in the SafeDeleteQueryset on line 66 it calls _, delete_response = obj.delete(force_policy=force_policy) and this causes the mp_node delete call and starts infinite recursion.
    Updating the line with _, delete_response = obj._delete(force_policy=force_policy) should fix this, right?
class SafeDeleteQueryset(query.QuerySet):
    ...

    def delete(self, force_policy: Optional[int] = None) -> Tuple[int, Dict[str, int]]:
        """Overrides bulk delete behaviour.

        .. note::
            The current implementation loses performance on bulk deletes in order
            to safely delete objects according to the deletion policies set.

        .. seealso::
            :py:func:`safedelete.models.SafeDeleteModel.delete`
        """
        assert self.query.can_filter(), "Cannot use 'limit' or 'offset' with delete."

        if force_policy == NO_DELETE:
            return (0, {})
        elif force_policy == HARD_DELETE:
            return self.hard_delete_policy_action()
        else:
            deleted_counter: Counter = Counter()
            # TODO: Replace this by bulk update if we can
            for obj in self.all():
                _, delete_response = obj.delete(force_policy=force_policy)
                deleted_counter.update(delete_response)
            self._result_cache = None
            return sum(deleted_counter.values()), dict(deleted_counter)
@Gagaro
Copy link
Member

Gagaro commented Jun 19, 2023

I'm not sure we should overpass the delete method.
What's causing the recursion in the mp_node delete method?
You should probably have your own mixin inheriting from both MP_Node and SafeDeleteModel and fix the conflict in its own delete method (or your model directly if you only use it there).

@kursat-ozer
Copy link
Author

kursat-ozer commented Jul 12, 2023

Sorry for the late response,

What's causing the recursion in the mp_node delete method?

MP_Node just propagates the delete to the next in inheritance which is normal I think.
SafeDelete behaves normal in single object operations. But when a bulk operation is needed, it causes recursive calls. It is not related to MP_Node, it will be the same with any other library that requires modified delete.
As I said in my post it is the same issue with #117
And this is the comment explains the logic:#117 (comment)
And this is the line why an another method created to do the same task:

return self._delete(force_policy, **kwargs)

You should probably have your own mixin inheriting from both MP_Node and SafeDeleteModel and fix the conflict in its own delete method (or your model directly if you only use it there).

What I did is inheriting only SafeDelete and fixing the line I said above and using this model as base.

But this needs a fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants