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

feat: raw delete expired instead of Queryset.delete #235

Merged
merged 12 commits into from Oct 11, 2021
4 changes: 2 additions & 2 deletions django_celery_results/managers.py
Expand Up @@ -8,7 +8,7 @@
from django.conf import settings
from django.db import connections, models, router, transaction

from .utils import now
from .utils import now, raw_delete

W_ISOLATION_REP = """
Polling results with transaction isolation level 'repeatable-read'
Expand Down Expand Up @@ -88,7 +88,7 @@ def get_all_expired(self, expires):
def delete_expired(self, expires):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for this method needs to be updated as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of update? the current test looks sufficient to me.

"""Delete all expired results."""
with transaction.atomic():
self.get_all_expired(expires).delete()
raw_delete(queryset=self.get_all_expired(expires))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another thing i want to be assured that running makemigrations detect any change in migrations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added similar test for django-silk two days back, where i simply called management command makemigrations silk --check --dry-run - if there are pending migrations then it returns with exit code 1.

Should we implement the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already this test which does that.

def test_models_match_migrations(self):
"""Make sure that no model changes exist.
This logic is taken from django's makemigrations.py file.
Here just detect if model changes exist that require
a migration, and if so we fail.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we replace this with call_command thing i mentioned above? One advantage is that it is a higher level Django API - changes in Django internal will not affect the test - until only the command behaviour is changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which part do you want to replace with what? can you elaborate 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.

Running makemigrations django_celery_results --check --dry-run command inside the test as mentioned above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made those changes. Please review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit - 113ac2e



class TaskResultManager(ResultManager):
Expand Down
5 changes: 5 additions & 0 deletions django_celery_results/utils.py
Expand Up @@ -15,3 +15,8 @@ def now():
return now_localtime(timezone.now())
else:
return timezone.now()


def raw_delete(queryset):
"""Raw delete given queryset."""
return queryset._raw_delete(queryset.db)
41 changes: 41 additions & 0 deletions t/unit/test_utils.py
@@ -0,0 +1,41 @@
from celery import uuid
from django.test import TransactionTestCase

from django_celery_results.models import TaskResult
from django_celery_results.utils import raw_delete

_TEST_RECORDS_COUNT = 100


class RawDeleteTest(TransactionTestCase):
"""Unit test for `utils.raw_delete` func."""

def setUp(self):
# setup test results to be used against raw_delete
TaskResult.objects.bulk_create(
[TaskResult(task_id=uuid()) for _ in range(_TEST_RECORDS_COUNT)]
)
assert TaskResult.objects.count() == _TEST_RECORDS_COUNT

def _get_sample_results(self, count):
return TaskResult.objects.values_list("pk").order_by("?")[:count]

def test_all_rows_delete(self):
raw_delete(
queryset=TaskResult.objects.all()
)
assert TaskResult.objects.count() == 0

def test_correct_rows_delete(self):
# sample random 10 rows
sample_size = 10
sample_ids = self._get_sample_results(count=sample_size)
# update task_name to "test"
TaskResult.objects.filter(pk__in=sample_ids).update(task_name="test")
# "raw delete" results with name = "test"
raw_delete(
queryset=TaskResult.objects.filter(task_name="test")
)
assert TaskResult.objects.filter(task_name="test").count() == 0
results_remaining = _TEST_RECORDS_COUNT - sample_size
assert TaskResult.objects.count() == results_remaining