From 90b9b381dc200364d64d56992a90128bcb73dea4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Mar 2023 11:28:58 +0100 Subject: [PATCH 1/2] Celery: cheat `job_status` view to return `finished` after 5 polls The frontend hits this endpoint every 1s. As we don't know when the task has finished now since we disabled the Celery result backend, we are just cheating it and return finished after 5 times the view is called with the same `task_id`. This fixes the issue we have in the UI when clicking the circled arrows to re-sync the remote repositories. --- readthedocs/api/v2/views/task_views.py | 53 +++++++------------ readthedocs/core/utils/tasks/__init__.py | 2 - readthedocs/core/utils/tasks/public.py | 41 ++------------ readthedocs/core/utils/tasks/retrieve.py | 27 +--------- readthedocs/rtd_tests/tests/test_api.py | 22 -------- .../rtd_tests/tests/test_privacy_urls.py | 16 ------ 6 files changed, 22 insertions(+), 139 deletions(-) diff --git a/readthedocs/api/v2/views/task_views.py b/readthedocs/api/v2/views/task_views.py index b5271271641..13c9de5dc2c 100644 --- a/readthedocs/api/v2/views/task_views.py +++ b/readthedocs/api/v2/views/task_views.py @@ -1,61 +1,44 @@ """Endpoints relating to task/job status, etc.""" import structlog - +from django.core.cache import cache from django.urls import reverse -from redis import ConnectionError from rest_framework import decorators, permissions from rest_framework.renderers import JSONRenderer from rest_framework.response import Response -from readthedocs.core.utils.tasks import TaskNoPermission, get_public_task_data from readthedocs.oauth import tasks - log = structlog.get_logger(__name__) -SUCCESS_STATES = ('SUCCESS',) -FAILURE_STATES = ( - 'FAILURE', - 'REVOKED', -) -FINISHED_STATES = SUCCESS_STATES + FAILURE_STATES -STARTED_STATES = ('RECEIVED', 'STARTED', 'RETRY') + FINISHED_STATES - - -def get_status_data(task_name, state, data, error=None): - data = { - 'name': task_name, - 'data': data, - 'started': state in STARTED_STATES, - 'finished': state in FINISHED_STATES, - # When an exception is raised inside the task, we keep this as SUCCESS - # and add the exception message into the 'error' key - 'success': state in SUCCESS_STATES and error is None, - } - if error is not None: - data['error'] = error - return data - @decorators.api_view(['GET']) @decorators.permission_classes((permissions.AllowAny,)) @decorators.renderer_classes((JSONRenderer,)) def job_status(request, task_id): - try: - task_name, state, public_data, error = get_public_task_data( - request, - task_id, - ) - except (TaskNoPermission, ConnectionError): - return Response(get_status_data('unknown', 'PENDING', {}),) - return Response(get_status_data(task_name, state, public_data, error),) + """Retrieve Celery task function state from frontend.""" + # HACK: always poll up to N times and after that return the sync has + # finished. This is a way to avoid re-enabling Celery result backend for now. + poll_n = cache.get(task_id, 0) + poll_n += 1 + cache.set(task_id, poll_n, 5 * 60) + finished = poll_n == 5 + + data = { + "name": "sync_remote_repositories", + "data": {}, + "started": True, + "finished": finished, + "success": finished, + } + return Response(data) @decorators.api_view(['POST']) @decorators.permission_classes((permissions.IsAuthenticated,)) @decorators.renderer_classes((JSONRenderer,)) def sync_remote_repositories(request): + """Trigger a re-sync of remote repositories for the user.""" result = tasks.sync_remote_repositories.delay(user_id=request.user.id,) task_id = result.task_id return Response({ diff --git a/readthedocs/core/utils/tasks/__init__.py b/readthedocs/core/utils/tasks/__init__.py index 1bd648a0796..8db5b6e3420 100644 --- a/readthedocs/core/utils/tasks/__init__.py +++ b/readthedocs/core/utils/tasks/__init__.py @@ -6,6 +6,4 @@ ) from .public import PublicTask # noqa from .public import TaskNoPermission # noqa -from .public import get_public_task_data # noqa from .retrieve import TaskNotFound # noqa -from .retrieve import get_task_data # noqa diff --git a/readthedocs/core/utils/tasks/public.py b/readthedocs/core/utils/tasks/public.py index 621e2798623..b19cef77002 100644 --- a/readthedocs/core/utils/tasks/public.py +++ b/readthedocs/core/utils/tasks/public.py @@ -1,22 +1,19 @@ -# -*- coding: utf-8 -*- - """Celery tasks with publicly viewable status.""" from celery import Task, states from django.conf import settings -from .retrieve import TaskNotFound, get_task_data - - __all__ = ( 'PublicTask', 'TaskNoPermission', - 'get_public_task_data', ) STATUS_UPDATES_ENABLED = not settings.CELERY_ALWAYS_EAGER +# pylint: disable=abstract-method +# pylint: disable=broad-except +# pylint: disable=invalid-name class PublicTask(Task): """ @@ -121,35 +118,3 @@ def __init__(self, task_id, *args, **kwargs): id=task_id, ) super().__init__(message, *args, **kwargs) - - -def get_public_task_data(request, task_id): - """ - Return task details as tuple. - - Will raise `TaskNoPermission` if `request` has no permission to access info - of the task with id `task_id`. This is also the case of no task with the - given id exists. - - :returns: (task name, task state, public data, error message) - :rtype: (str, str, dict, str) - """ - try: - task, state, info = get_task_data(task_id) - except TaskNotFound: - # No task info has been found act like we don't have permission to see - # the results. - raise TaskNoPermission(task_id) - - if not hasattr(task, 'check_permission'): - raise TaskNoPermission(task_id) - - context = info.get('context', {}) - if not task.check_permission(request, state, context): - raise TaskNoPermission(task_id) - return ( - task.name, - state, - info.get('public_data', {}), - info.get('error', None), - ) diff --git a/readthedocs/core/utils/tasks/retrieve.py b/readthedocs/core/utils/tasks/retrieve.py index 9281ad8a1af..8257ef9e386 100644 --- a/readthedocs/core/utils/tasks/retrieve.py +++ b/readthedocs/core/utils/tasks/retrieve.py @@ -1,12 +1,8 @@ -# -*- coding: utf-8 -*- """Utilities for retrieving task data.""" -from celery import states -from celery.result import AsyncResult - -__all__ = ('TaskNotFound', 'get_task_data') +__all__ = ("TaskNotFound",) class TaskNotFound(Exception): @@ -14,24 +10,3 @@ class TaskNotFound(Exception): def __init__(self, task_id, *args, **kwargs): message = 'No public task found with id {id}'.format(id=task_id) super().__init__(message, *args, **kwargs) - - -def get_task_data(task_id): - """ - Will raise `TaskNotFound` if the task is in state ``PENDING`` or the task. - - meta data has no ``'task_name'`` key set. - """ - from readthedocs.worker import app - - result = AsyncResult(task_id) - state, info = result.state, result.info - if state == states.PENDING: - raise TaskNotFound(task_id) - if 'task_name' not in info: - raise TaskNotFound(task_id) - try: - task = app.tasks[info['task_name']] - except KeyError: - raise TaskNotFound(task_id) - return task, state, info diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 29974c006f8..0063f76ab40 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -38,7 +38,6 @@ GitHubWebhookView, GitLabWebhookView, ) -from readthedocs.api.v2.views.task_views import get_status_data from readthedocs.builds.constants import ( BUILD_STATE_CLONING, BUILD_STATE_FINISHED, @@ -2545,24 +2544,3 @@ def test_modify_version(self): self.assertEqual(resp.data['has_pdf'], True) self.assertEqual(resp.data['has_epub'], False) self.assertEqual(resp.data['has_htmlzip'], False) - - -class TaskViewsTests(TestCase): - - def test_get_status_data(self): - data = get_status_data( - 'public_task_exception', - 'SUCCESS', - {'data': 'public'}, - 'Something bad happened', - ) - self.assertEqual( - data, { - 'name': 'public_task_exception', - 'data': {'data': 'public'}, - 'started': True, - 'finished': True, - 'success': False, - 'error': 'Something bad happened', - }, - ) diff --git a/readthedocs/rtd_tests/tests/test_privacy_urls.py b/readthedocs/rtd_tests/tests/test_privacy_urls.py index c351e305cee..a7ab1d43dbb 100644 --- a/readthedocs/rtd_tests/tests/test_privacy_urls.py +++ b/readthedocs/rtd_tests/tests/test_privacy_urls.py @@ -15,7 +15,6 @@ RegexAutomationRule, VersionAutomationRule, ) -from readthedocs.core.utils.tasks import TaskNoPermission from readthedocs.integrations.models import HttpExchange, Integration from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.projects.models import Domain, EnvironmentVariable, Project, WebHook @@ -452,21 +451,6 @@ def setUp(self): } -class APIUnauthAccessTest(APIMixin, TestCase): - - @mock.patch('readthedocs.api.v2.views.task_views.get_public_task_data') - def test_api_urls(self, get_public_task_data): - from readthedocs.api.v2.urls import urlpatterns - get_public_task_data.side_effect = TaskNoPermission('Nope') - self._test_url(urlpatterns) - - def login(self): - pass - - def is_admin(self): - return False - - class PublicUserProfileMixin(URLAccessMixin): def setUp(self): From 4859034678dff66b0ce15938bed1633ab7e8e27c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Mar 2023 18:36:22 +0100 Subject: [PATCH 2/2] Update readthedocs/api/v2/views/task_views.py Co-authored-by: Anthony --- readthedocs/api/v2/views/task_views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/api/v2/views/task_views.py b/readthedocs/api/v2/views/task_views.py index 13c9de5dc2c..7d893963973 100644 --- a/readthedocs/api/v2/views/task_views.py +++ b/readthedocs/api/v2/views/task_views.py @@ -19,6 +19,7 @@ def job_status(request, task_id): """Retrieve Celery task function state from frontend.""" # HACK: always poll up to N times and after that return the sync has # finished. This is a way to avoid re-enabling Celery result backend for now. + # TODO remove this API and RemoteRepo sync UI when we have better auto syncing poll_n = cache.get(task_id, 0) poll_n += 1 cache.set(task_id, poll_n, 5 * 60)