Fix atomic transaction not routing to the the correct DB#324
Conversation
auvipy
left a comment
There was a problem hiding this comment.
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 |
| @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.
can you explain this part a bit more please?
There was a problem hiding this comment.
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").
https://github.com/nofalx/django-celery-results/blob/fix-323/t/unit/test_models.py#L235:L240
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