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

Fix atomic transaction not routing to the the correct DB #324

Merged
merged 4 commits into from Jun 22, 2022

Conversation

nofalx
Copy link
Contributor

@nofalx nofalx commented Jun 21, 2022

This to fix the issue explained at: #323

Copy link
Member

@auvipy auvipy left a 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?

@nofalx
Copy link
Contributor Author

nofalx commented Jun 21, 2022

I have added a new test class and a db router as these are required to test this specific scenario

using make test

The test passes after the edits to the manager.py file

before the changes I added to manager.py the test fails with the below error:

FAILED t/unit/test_models.py::test_ModelsOnSecondaryDbOnly::test_operations_with_atomic_transactions - AssertionError: Database connections to 'default' are not allowed in this test. Add 'default' to t.unit.test_models.test_ModelsOnSecondaryDbOnly.databases to ensure proper test isolation and silence this failure.

Screenshot from 2022-06-21 13-50-10

@nofalx nofalx requested a review from auvipy June 21, 2022 10:04
t/proj/db_routers.py Outdated Show resolved Hide resolved
@pytest.mark.usefixtures('depends_on_current_app')
class test_ModelsOnSecondaryDbOnly(TransactionTestCase):
"""
These tests will fail with the below error incase we
Copy link
Member

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?

Copy link
Contributor Author

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").

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.

@nofalx nofalx requested a review from auvipy June 22, 2022 08:16
@auvipy auvipy merged commit e927770 into celery:master Jun 22, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants