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

on_commit may fail when not using default db #123

Open
lingfromSh opened this issue Jan 30, 2023 · 3 comments
Open

on_commit may fail when not using default db #123

lingfromSh opened this issue Jan 30, 2023 · 3 comments

Comments

@lingfromSh
Copy link

lingfromSh commented Jan 30, 2023

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 lingfromSh closed this as not planned Won't fix, can't repro, duplicate, stale Jan 30, 2023
@lingfromSh lingfromSh reopened this Jan 30, 2023
@lingfromSh
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

lingfromSh commented Jan 30, 2023

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.

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