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

skip_hooks context manager #135

Open
TonisPiip opened this issue Oct 19, 2023 · 1 comment
Open

skip_hooks context manager #135

TonisPiip opened this issue Oct 19, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@TonisPiip
Copy link

TonisPiip commented Oct 19, 2023

Due to the current way skip_hooks works, you need to pass it into save(), you can not suppress hooks in the case of using model.objects.get_or_create. You can't just pass in skip_hooks to that method as it's used when initing the new model, and in inital get request.

For create() it would be possible to eaisly fix by popping out skip_hooks from init's kwags

    def create(self, **kwargs):
        """
        Create a new object with the given kwargs, saving it to the database
        and returning the created object.
        """
        obj = self.model(**kwargs)
        self._for_write = True
        obj.save(force_insert=True, using=self.db)
        return obj
class LifecycleModelMixin(object):
    def __init__(self, *args, **kwargs):
        self._skip_hooks = kwargs.pop("skip_hooks", None) #newline
        super().__init__(*args, **kwargs)
        self._initial_state = self._snapshot_state()

and then in save also check the non_null sate of self._get_hooks

    @transaction.atomic
    def save(self, *args, **kwargs):
        skip_hooks = kwargs.pop("skip_hooks", False) or self._skip_hooks

But that doesn't fix that skip_hooks=True will be passed to the queryset filter.
So in that case, the only solution would be some global var/instance which is set using enter and exit

class SkipHooks() :
    skip_hooks = False
    def __enter__(self):
         self.skip_hooks = True
    def __exit__(self):
         self.skip_hooks = False

skip_hooks = SkipHooks()

with skip_hooks:
    foo.save()
    @transaction.atomic
    def save(self, *args, **kwargs):
        skip_hooks = kwargs.pop("skip_hooks", False) or self._skip_hooks or skip_hooks.skip_hooks

Does anyone have any comments on this?

And if I were to make the proposed changes, would they PR be accepted?

@EnriqueSoria EnriqueSoria added the enhancement New feature or request label Nov 1, 2023
@EnriqueSoria
Copy link
Collaborator

EnriqueSoria commented Nov 1, 2023

Thanks for this suggestion!

I like the context manager approach, I've seen similar patterns in other projects like django-modeltranslation:

https://github.com/deschler/django-modeltranslation/blob/2061f6c264d7cb889ae14d2d52a7547df6d58663/modeltranslation/utils.py#L133C13-L155

https://github.com/deschler/django-modeltranslation/blob/master/modeltranslation/thread_context.py#L9-L21

If you want to provide an implementation for this let me know, if not when I have some spare time I could try to do it.

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

No branches or pull requests

2 participants