rsinger86 / django-lifecycle

Declarative model lifecycle hooks, an alternative to Signals.
https://rsinger86.github.io/django-lifecycle/
MIT License
1.34k stars 89 forks source link

on_commit may fail when not using default db #123

Open lingfromSh opened 1 year ago

lingfromSh commented 1 year ago

Problems

In my project, i have some serveral databases. so, my settings.py looks like below.

# settings.py
DATABASES = {
  "default": {
      ...
  },
  "sales": {
      ...
   },
   ...
}

When i set on_commit=True and operated with some objects of sales, it failed.

And i find we call transaction.on_commit

# mixins.py
class OnCommitHookedMethod(AbstractHookedMethod):
    """ Hooked method that should run on_commit """

    @property
    def name(self) -> str:
        # Append `_on_commit` to the existing method name to allow for firing
        # the same hook within the atomic transaction and on_commit
        return f"{self.method.__name__}_on_commit"

    def run(self, instance: Any) -> None:
        # Use partial to create a function closure that binds `self`
        # to ensure it's available to execute later.
        _on_commit_func = partial(self.method, instance)
        _on_commit_func.__name__ = self.name

        # Can we allow passing using param here? Or, get using from instance directly
        transaction.on_commit(_on_commit_func) 

# django/db/transaction.py

def get_connection(using=None):
    """
    Get a database connection by name, or the default database connection
    if no name is provided. This is a private API.
    """
    if using is None:   # here!
        using = DEFAULT_DB_ALIAS
    return connections[using]
lingfromSh commented 1 year ago

Now i just add using in MyOnCommitHookedMethod to replace the origin.

from django.db import router

## HookMethod
class MyOnCommitHookedMethod(OnCommitHookedMethod):
    """Hooked method that should run on_commit"""

    def run(self, instance: Any, using=None) -> None:
        # Use partial to create a function closure that binds `self`
        # to ensure it's available to execute later.
        _on_commit_func = partial(self.method, instance)
        _on_commit_func.__name__ = self.name
        using = using or router.db_for_write(instance.__class__, instance)
        transaction.on_commit(_on_commit_func, using=using)
EnriqueSoria commented 1 year ago

Your approach makes sense to me, except for having using=None as a parameter of run method. Where would you specify this parameter?

If we remove it I think it could work for everyone

class OnCommitHookedMethod(AbstractHookedMethod):
    """ Hooked method that should run on_commit """

    @property
    def name(self) -> str:
        # Append `_on_commit` to the existing method name to allow for firing
        # the same hook within the atomic transaction and on_commit
        return f"{self.method.__name__}_on_commit"

    def run(self, instance: Any) -> None:
        # Use partial to create a function closure that binds `self`
        # to ensure it's available to execute later.
        _on_commit_func = partial(self.method, instance)
        _on_commit_func.__name__ = self.name

        transaction.on_commit(_on_commit_func, using=router.db_for_write(instance.__class__, instance)) 
lingfromSh commented 1 year ago

Thanks for your comment.

For some historical reasons, I have overwritten some methods inherited from LifecycleModel in my base Model class. Thus, i can pass using to Hookmethod.run.

In normal case, adding using=router.db_for_write(instance.__class__, instance) is okay.