From 62497f9db0eb2b1118568d9d5b16c25e2ae31c0a Mon Sep 17 00:00:00 2001 From: Ahmad Nofal Date: Tue, 21 Jun 2022 11:30:18 +0400 Subject: [PATCH 1/4] Fix atomic transaction not routing to the the correct DB --- django_celery_results/managers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/django_celery_results/managers.py b/django_celery_results/managers.py index 3242e1d7..7df78e4e 100644 --- a/django_celery_results/managers.py +++ b/django_celery_results/managers.py @@ -87,7 +87,7 @@ def get_all_expired(self, expires): def delete_expired(self, expires): """Delete all expired results.""" - with transaction.atomic(): + with transaction.atomic(using=self.db): raw_delete(queryset=self.get_all_expired(expires)) From 2c971d7460780a041b0e6b351b1cd82051f4e221 Mon Sep 17 00:00:00 2001 From: Ahmad Nofal Date: Tue, 21 Jun 2022 13:55:36 +0400 Subject: [PATCH 2/4] Added test case for atomic transaction to ensure its using the correct db --- t/proj/db_routers.py | 26 ++++++++++++++++++++++++++ t/unit/test_models.py | 20 +++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 t/proj/db_routers.py diff --git a/t/proj/db_routers.py b/t/proj/db_routers.py new file mode 100644 index 00000000..17cf8a55 --- /dev/null +++ b/t/proj/db_routers.py @@ -0,0 +1,26 @@ +class AlwaysSecondaryDbRouter: + connection_name = "secondary" + + def db_for_read(self, model, **hints): + """ + Route read always for the specified connection + """ + return self.connection_name + + def db_for_write(self, model, **hints): + """ + Route write always for the specified connection + """ + return self.connection_name + + def allow_relation(self, obj1, obj2, **hints): + """ + Router have no opinion here + """ + return None + + def allow_migrate(self, db, app_label, model_name=None, **hints): + """ + Router have no opinion here + """ + return None diff --git a/t/unit/test_models.py b/t/unit/test_models.py index 4c2cff62..1aa69294 100644 --- a/t/unit/test_models.py +++ b/t/unit/test_models.py @@ -5,7 +5,7 @@ from celery import states, uuid from django.db import transaction from django.db.utils import InterfaceError -from django.test import TransactionTestCase +from django.test import TransactionTestCase, override_settings from django_celery_results.backends import DatabaseBackend from django_celery_results.models import GroupResult, TaskResult @@ -209,3 +209,21 @@ class TransactionError(Exception): raise TransactionError() except TransactionError: pass + + +@pytest.mark.usefixtures('depends_on_current_app') +class test_ModelsOnSecondaryDbOnly(TransactionTestCase): + """ + These tests will fail with the below error incase we + try to read/write from a db other than the secondary + + AssertionError: Database connections to 'default' are + not allowed in this test. + """ + databases = ['secondary'] + + @override_settings( + DATABASE_ROUTERS=['t.proj.db_routers.AlwaysSecondaryDbRouter'], + ) + def test_operations_with_atomic_transactions(self): + TaskResult.objects.delete_expired(expires=10) From 5f7a91a9c500805240f94fcb84ac979e9c491087 Mon Sep 17 00:00:00 2001 From: Ahmad Nofal Date: Wed, 22 Jun 2022 11:55:18 +0400 Subject: [PATCH 3/4] simplify the test --- t/proj/db_routers.py | 26 -------------------------- t/unit/test_models.py | 29 ++++++++++++++++++----------- 2 files changed, 18 insertions(+), 37 deletions(-) delete mode 100644 t/proj/db_routers.py diff --git a/t/proj/db_routers.py b/t/proj/db_routers.py deleted file mode 100644 index 17cf8a55..00000000 --- a/t/proj/db_routers.py +++ /dev/null @@ -1,26 +0,0 @@ -class AlwaysSecondaryDbRouter: - connection_name = "secondary" - - def db_for_read(self, model, **hints): - """ - Route read always for the specified connection - """ - return self.connection_name - - def db_for_write(self, model, **hints): - """ - Route write always for the specified connection - """ - return self.connection_name - - def allow_relation(self, obj1, obj2, **hints): - """ - Router have no opinion here - """ - return None - - def allow_migrate(self, db, app_label, model_name=None, **hints): - """ - Router have no opinion here - """ - return None diff --git a/t/unit/test_models.py b/t/unit/test_models.py index 1aa69294..49495330 100644 --- a/t/unit/test_models.py +++ b/t/unit/test_models.py @@ -212,18 +212,25 @@ class TransactionError(Exception): @pytest.mark.usefixtures('depends_on_current_app') -class test_ModelsOnSecondaryDbOnly(TransactionTestCase): +class test_ModelsWithoutDefaultDB(TransactionTestCase): """ - These tests will fail with the below error incase we - try to read/write from a db other than the secondary - - AssertionError: Database connections to 'default' are - not allowed in this test. + This class to ensure all operations are done on the + same db we use and dont leak accidentally into another + db. we dont include the default db in databases as by + default an incorrect behavior would route there and + would not be detectable. + + The tests will fail with the below error incase we + try to interact from a db other than the one we have + specified. + + `AssertionError: Database connections to 'default' are + not allowed in this test` """ - databases = ['secondary'] - @override_settings( - DATABASE_ROUTERS=['t.proj.db_routers.AlwaysSecondaryDbRouter'], - ) + non_default_test_db = 'secondary' + databases = [non_default_test_db] + def test_operations_with_atomic_transactions(self): - TaskResult.objects.delete_expired(expires=10) + TaskResult.objects.db_manager(self.non_default_test_db).delete_expired(expires=10) + GroupResult.objects.db_manager(self.non_default_test_db).delete_expired(expires=10) From 50df557ae2e99433b20ad22048e66abffde03c8e Mon Sep 17 00:00:00 2001 From: Ahmad Nofal Date: Wed, 22 Jun 2022 11:58:37 +0400 Subject: [PATCH 4/4] lint --- t/unit/test_models.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/t/unit/test_models.py b/t/unit/test_models.py index 49495330..e655f4ce 100644 --- a/t/unit/test_models.py +++ b/t/unit/test_models.py @@ -5,7 +5,7 @@ from celery import states, uuid from django.db import transaction from django.db.utils import InterfaceError -from django.test import TransactionTestCase, override_settings +from django.test import TransactionTestCase from django_celery_results.backends import DatabaseBackend from django_celery_results.models import GroupResult, TaskResult @@ -232,5 +232,9 @@ class test_ModelsWithoutDefaultDB(TransactionTestCase): databases = [non_default_test_db] def test_operations_with_atomic_transactions(self): - TaskResult.objects.db_manager(self.non_default_test_db).delete_expired(expires=10) - GroupResult.objects.db_manager(self.non_default_test_db).delete_expired(expires=10) + TaskResult.objects.db_manager( + self.non_default_test_db + ).delete_expired(expires=10) + GroupResult.objects.db_manager( + self.non_default_test_db + ).delete_expired(expires=10)