-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fix atomic transaction not routing to the the correct DB #324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add some tests for the change?
I have added a new test class and a db router as these are required to test this specific scenario using The test passes after the edits to the before the changes I added to
|
t/unit/test_models.py
Outdated
@pytest.mark.usefixtures('depends_on_current_app') | ||
class test_ModelsOnSecondaryDbOnly(TransactionTestCase): | ||
""" | ||
These tests will fail with the below error incase we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain this part a bit more please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will try to run the method delete_expired
from the Custom Manager ResultManager
where the bug occurs.
The issue is that if the manager was directed to use a db other than the "default" db, the method delete_expired
would run the operation on the "default" anyways. Since the function does not return anything and is a delete operation I can not use assert
My method of testing is the below: (let me know if there is a better way)
I specify the allowed db to test on as "secondary" only. If the test tries to use the "default" db it will throw error:
https://github.com/nofalx/django-celery-results/blob/fix-323/t/unit/test_models.py#L231:L232
I specify the TaskResult
and the GroupResult
manager which extend ResultManager
to use "secondary" db using .db_manager("secondary")
.
Before the fixes this test would fail as transaction.atomic()
would try to use the "default" db. After the fix the method doesnt use "default" db anymore and the test pass.
This to fix the issue explained at: #323