From aa7195f880806b23ed1f7d8560012894105e9c0a Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 31 Jul 2024 11:40:02 -0400 Subject: [PATCH 1/6] Use HookUtils directly when submission comes in --- kobo/apps/hook/utils.py | 5 +++-- kobo/apps/hook/views/v2/hook_signal.py | 2 +- .../openrosa/apps/viewer/models/parsed_instance.py | 14 ++++++++++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/kobo/apps/hook/utils.py b/kobo/apps/hook/utils.py index 55fe5760e0..5bb6e1391c 100644 --- a/kobo/apps/hook/utils.py +++ b/kobo/apps/hook/utils.py @@ -1,4 +1,5 @@ # coding: utf-8 +from .models.hook import Hook from .models.hook_log import HookLog from .tasks import service_definition_task @@ -6,13 +7,13 @@ class HookUtils: @staticmethod - def call_services(asset: 'kpi.models.asset.Asset', submission_id: int): + def call_services(asset_uid: str, submission_id: int) -> bool: """ Delegates to Celery data submission to remote servers """ # Retrieve `Hook` ids, to send data to their respective endpoint. hooks_ids = ( - asset.hooks.filter(active=True) + Hook.objects.filter(asset__uid=asset_uid, active=True) .values_list('id', flat=True) .distinct() ) diff --git a/kobo/apps/hook/views/v2/hook_signal.py b/kobo/apps/hook/views/v2/hook_signal.py index 5bab71ef46..d9150f9fa5 100644 --- a/kobo/apps/hook/views/v2/hook_signal.py +++ b/kobo/apps/hook/views/v2/hook_signal.py @@ -63,7 +63,7 @@ def create(self, request, *args, **kwargs): if not (submission and int(submission['_id']) == submission_id): raise Http404 - if HookUtils.call_services(self.asset, submission_id): + if HookUtils.call_services(self.asset.uid, submission_id): # Follow Open Rosa responses by default response_status_code = status.HTTP_202_ACCEPTED response = { diff --git a/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py b/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py index 469fae597a..2427ab651e 100644 --- a/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py +++ b/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py @@ -12,7 +12,6 @@ from kobo.apps.openrosa.apps.api.mongo_helper import MongoHelper from kobo.apps.openrosa.apps.logger.models import Instance from kobo.apps.openrosa.apps.logger.models import Note -from kobo.apps.openrosa.apps.restservice.utils import call_service from kobo.apps.openrosa.libs.utils.common_tags import ( ID, UUID, @@ -27,6 +26,8 @@ ) from kobo.apps.openrosa.libs.utils.decorators import apply_form_field_names from kobo.apps.openrosa.libs.utils.model_tools import queryset_iterator +from kobo.apps.hook.utils import HookUtils +from kpi.utils.log import logging # this is Mongo Collection where we will store the parsed submissions xform_instances = settings.MONGO_DB.instances @@ -371,7 +372,16 @@ def save(self, asynchronous=False, *args, **kwargs): # Rest Services were called before data was saved in DB. success = self.update_mongo(asynchronous) if success and created: - call_service(self) + records = ParsedInstance.objects.filter( + instance_id=self.instance_id + ).values_list('instance__xform__kpi_asset_uid', flat=True) + if not (asset_uid := records[0]): + logging.warning( + f'ParsedInstance #: {self.pk} - XForm is not linked with Asset' + ) + else: + HookUtils.call_services(asset_uid, self.instance_id) + return success def add_note(self, note): From 11a0322906040e77f398d5f204cb581013fd4ede Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 31 Jul 2024 13:45:32 -0400 Subject: [PATCH 2/6] Remove back and forth API calls to activate Rest Services - Remove logger.XForm `has_kpi_hook` field - Remove logger.Instance `posted_to_kpi` field - Remove openrosa.rest_service django app - Remove hook signal viewset --- kobo/apps/hook/tasks.py | 32 +++---- kobo/apps/hook/tests/test_api_hook.py | 36 -------- kobo/apps/hook/tests/test_utils.py | 52 +++++++++++ kobo/apps/hook/views/v1/__init__.py | 1 - kobo/apps/hook/views/v1/hook_signal.py | 33 ------- kobo/apps/hook/views/v2/__init__.py | 1 - kobo/apps/hook/views/v2/hook_signal.py | 84 ------------------ .../api/tests/viewsets/test_xform_viewset.py | 1 - ...as_kpi_hooks_and_instance_posted_to_kpi.py | 27 ++++++ .../openrosa/apps/logger/models/instance.py | 4 - .../apps/openrosa/apps/logger/models/xform.py | 9 -- .../0015_drop_old_restservice_tables.py | 77 ++++++++++++++++ .../apps/restservice/RestServiceInterface.py | 4 - .../openrosa/apps/restservice/__init__.py | 9 -- kobo/apps/openrosa/apps/restservice/app.py | 12 --- .../apps/restservice/management/__init__.py | 1 - .../management/commands/__init__.py | 1 - .../commands/update_kpi_hooks_endpoint.py | 41 --------- .../restservice/migrations/0001_initial.py | 25 ------ ...add_related_name_with_delete_on_cascade.py | 22 ----- .../0003_remove_deprecated_services.py | 17 ---- .../apps/restservice/migrations/__init__.py | 1 - kobo/apps/openrosa/apps/restservice/models.py | 31 ------- .../apps/restservice/services/__init__.py | 2 - .../apps/restservice/services/kpi_hook.py | 43 --------- .../apps/openrosa/apps/restservice/signals.py | 36 -------- kobo/apps/openrosa/apps/restservice/tasks.py | 35 -------- .../restservice/templates/add-service.html | 11 --- .../apps/restservice/tests/__init__.py | 1 - .../restservice/tests/fixtures/dhisform.xls | Bin 6656 -> 0 bytes .../restservice/tests/test_restservice.py | 40 --------- kobo/apps/openrosa/apps/restservice/utils.py | 24 ----- kobo/apps/openrosa/libs/constants.py | 1 - .../libs/serializers/xform_serializer.py | 1 - kobo/settings/base.py | 1 - kpi/deployment_backends/base_backend.py | 4 - kpi/deployment_backends/mock_backend.py | 14 --- kpi/deployment_backends/openrosa_backend.py | 24 +---- kpi/signals.py | 10 --- .../v2/test_api_invalid_password_access.py | 17 ---- kpi/urls/router_api_v1.py | 6 -- kpi/urls/router_api_v2.py | 7 -- 42 files changed, 174 insertions(+), 624 deletions(-) create mode 100644 kobo/apps/hook/tests/test_utils.py delete mode 100644 kobo/apps/hook/views/v1/hook_signal.py delete mode 100644 kobo/apps/hook/views/v2/hook_signal.py create mode 100644 kobo/apps/openrosa/apps/logger/migrations/0035_remove_xform_has_kpi_hooks_and_instance_posted_to_kpi.py create mode 100644 kobo/apps/openrosa/apps/main/migrations/0015_drop_old_restservice_tables.py delete mode 100644 kobo/apps/openrosa/apps/restservice/RestServiceInterface.py delete mode 100644 kobo/apps/openrosa/apps/restservice/__init__.py delete mode 100644 kobo/apps/openrosa/apps/restservice/app.py delete mode 100644 kobo/apps/openrosa/apps/restservice/management/__init__.py delete mode 100644 kobo/apps/openrosa/apps/restservice/management/commands/__init__.py delete mode 100644 kobo/apps/openrosa/apps/restservice/management/commands/update_kpi_hooks_endpoint.py delete mode 100644 kobo/apps/openrosa/apps/restservice/migrations/0001_initial.py delete mode 100644 kobo/apps/openrosa/apps/restservice/migrations/0002_add_related_name_with_delete_on_cascade.py delete mode 100644 kobo/apps/openrosa/apps/restservice/migrations/0003_remove_deprecated_services.py delete mode 100644 kobo/apps/openrosa/apps/restservice/migrations/__init__.py delete mode 100644 kobo/apps/openrosa/apps/restservice/models.py delete mode 100644 kobo/apps/openrosa/apps/restservice/services/__init__.py delete mode 100644 kobo/apps/openrosa/apps/restservice/services/kpi_hook.py delete mode 100644 kobo/apps/openrosa/apps/restservice/signals.py delete mode 100644 kobo/apps/openrosa/apps/restservice/tasks.py delete mode 100644 kobo/apps/openrosa/apps/restservice/templates/add-service.html delete mode 100644 kobo/apps/openrosa/apps/restservice/tests/__init__.py delete mode 100755 kobo/apps/openrosa/apps/restservice/tests/fixtures/dhisform.xls delete mode 100644 kobo/apps/openrosa/apps/restservice/tests/test_restservice.py delete mode 100644 kobo/apps/openrosa/apps/restservice/utils.py diff --git a/kobo/apps/hook/tasks.py b/kobo/apps/hook/tasks.py index 6ff659961f..b1dfbf7a96 100644 --- a/kobo/apps/hook/tasks.py +++ b/kobo/apps/hook/tasks.py @@ -32,7 +32,7 @@ def service_definition_task(self, hook_id, submission_id): hook = Hook.objects.get(id=hook_id) # Use camelcase (even if it's not PEP-8 compliant) # because variable represents the class, not the instance. - ServiceDefinition = hook.get_service_definition() + ServiceDefinition = hook.get_service_definition() # noqa service_definition = ServiceDefinition(hook, submission_id) if not service_definition.send(): # Countdown is in seconds @@ -43,10 +43,7 @@ def service_definition_task(self, hook_id, submission_id): @shared_task -def retry_all_task(hooklogs_ids): - """ - :param list: . - """ +def retry_all_task(hooklogs_ids: int): hook_logs = HookLog.objects.filter(id__in=hooklogs_ids) for hook_log in hook_logs: hook_log.retry() @@ -71,22 +68,24 @@ def failures_reports(): if failures_reports_period_task: last_run_at = failures_reports_period_task.last_run_at - queryset = HookLog.objects.filter(hook__email_notification=True, - status=HOOK_LOG_FAILED) + queryset = HookLog.objects.filter( + hook__email_notification=True, status=HOOK_LOG_FAILED + ) if last_run_at: queryset = queryset.filter(date_modified__gte=last_run_at) - queryset = queryset.order_by('hook__asset__name', - 'hook__uid', - '-date_modified') + queryset = queryset.order_by( + 'hook__asset__name', 'hook__uid', '-date_modified' + ) # PeriodicTask are updated every 3 minutes (default). # It means, if this task interval is less than 3 minutes, some data can be duplicated in emails. # Setting `beat-sync-every` to 1, makes PeriodicTask to be updated before running the task. # So, we need to update it manually. # see: http://docs.celeryproject.org/en/latest/userguide/configuration.html#beat-sync-every - PeriodicTask.objects.filter(task=beat_schedule.get("task")). \ - update(last_run_at=timezone.now()) + PeriodicTask.objects.filter(task=beat_schedule.get('task')).update( + last_run_at=timezone.now() + ) records = {} max_length = 0 @@ -147,9 +146,12 @@ def failures_reports(): text_content = plain_text_template.render(variables) html_content = html_template.render(variables) - msg = EmailMultiAlternatives(translation.gettext('REST Services Failure Report'), text_content, - constance.config.SUPPORT_EMAIL, - [record.get('email')]) + msg = EmailMultiAlternatives( + translation.gettext('REST Services Failure Report'), + text_content, + constance.config.SUPPORT_EMAIL, + [record.get('email')], + ) msg.attach_alternative(html_content, 'text/html') email_messages.append(msg) diff --git a/kobo/apps/hook/tests/test_api_hook.py b/kobo/apps/hook/tests/test_api_hook.py index 511d6486f2..b8fbe5f7f8 100644 --- a/kobo/apps/hook/tests/test_api_hook.py +++ b/kobo/apps/hook/tests/test_api_hook.py @@ -56,42 +56,6 @@ def test_anonymous_access(self): def test_create_hook(self): self._create_hook() - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) - @responses.activate - def test_data_submission(self): - # Create first hook - first_hook = self._create_hook(name="dummy external service", - endpoint="http://dummy.service.local/", - settings={}) - responses.add(responses.POST, first_hook.endpoint, - status=status.HTTP_200_OK, - content_type="application/json") - hook_signal_url = reverse("hook-signal-list", kwargs={"parent_lookup_asset": self.asset.uid}) - - submissions = self.asset.deployment.get_submissions(self.asset.owner) - data = {'submission_id': submissions[0]['_id']} - response = self.client.post(hook_signal_url, data=data, format='json') - self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED) - - # Create second hook - second_hook = self._create_hook(name="other dummy external service", - endpoint="http://otherdummy.service.local/", - settings={}) - responses.add(responses.POST, second_hook.endpoint, - status=status.HTTP_200_OK, - content_type="application/json") - - response = self.client.post(hook_signal_url, data=data, format='json') - self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED) - - response = self.client.post(hook_signal_url, data=data, format='json') - self.assertEqual(response.status_code, status.HTTP_409_CONFLICT) - - data = {'submission_id': 4} # Instance doesn't belong to `self.asset` - response = self.client.post(hook_signal_url, data=data, format='json') - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_editor_access(self): hook = self._create_hook() diff --git a/kobo/apps/hook/tests/test_utils.py b/kobo/apps/hook/tests/test_utils.py new file mode 100644 index 0000000000..ad198a7b7c --- /dev/null +++ b/kobo/apps/hook/tests/test_utils.py @@ -0,0 +1,52 @@ +import responses +from mock import patch +from rest_framework import status + +from .hook_test_case import HookTestCase, MockSSRFProtect +from ..utils import HookUtils + + +class HookUtilsTestCase(HookTestCase): + + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) + @responses.activate + def test_data_submission(self): + # Create first hook + first_hook = self._create_hook( + name='dummy external service', + endpoint='http://dummy.service.local/', + settings={}, + ) + responses.add( + responses.POST, + first_hook.endpoint, + status=status.HTTP_200_OK, + content_type='application/json', + ) + + submissions = self.asset.deployment.get_submissions(self.asset.owner) + submission_id = submissions[0]['_id'] + assert HookUtils.call_services(self.asset.uid, submission_id) is True + + # Create second hook + second_hook = self._create_hook( + name='other dummy external service', + endpoint='http://otherdummy.service.local/', + settings={}, + ) + responses.add( + responses.POST, + second_hook.endpoint, + status=status.HTTP_200_OK, + content_type='application/json', + ) + # Since second hook hasn't received the submission, `call_services` + # should still return True + assert HookUtils.call_services(self.asset.uid, submission_id) is True + + # But if we try again, it should return False (we cannot send the same + # submission twice to the same external endpoint). + assert HookUtils.call_services(self.asset.uid, submission_id) is False diff --git a/kobo/apps/hook/views/v1/__init__.py b/kobo/apps/hook/views/v1/__init__.py index 66a9504388..c3bb54f968 100644 --- a/kobo/apps/hook/views/v1/__init__.py +++ b/kobo/apps/hook/views/v1/__init__.py @@ -1,4 +1,3 @@ # coding: utf-8 from .hook import HookViewSet from .hook_log import HookLogViewSet -from .hook_signal import HookSignalViewSet diff --git a/kobo/apps/hook/views/v1/hook_signal.py b/kobo/apps/hook/views/v1/hook_signal.py deleted file mode 100644 index 37b0a5c5b3..0000000000 --- a/kobo/apps/hook/views/v1/hook_signal.py +++ /dev/null @@ -1,33 +0,0 @@ -# coding: utf-8 -from kobo.apps.hook.views.v2.hook_signal import HookSignalViewSet as HookSignalViewSetV2 - - -class HookSignalViewSet(HookSignalViewSetV2): - """ - ## This document is for a deprecated version of kpi's API. - - **Please upgrade to latest release `/api/v2/assets/hook-signal/`** - - - This endpoint is only used to trigger asset's hooks if any. - - Tells the hooks to post an instance to external servers. - 
-    POST /api/v2/assets/{uid}/hook-signal/
-
- - - > Example - > - > curl -X POST https://[kpi-url]/assets/aSAvYreNzVEkrWg5Gdcvg/hook-signal/ - - - > **Expected payload** - > - > { - > "submission_id": {integer} - > } - - """ - - pass diff --git a/kobo/apps/hook/views/v2/__init__.py b/kobo/apps/hook/views/v2/__init__.py index 66a9504388..c3bb54f968 100644 --- a/kobo/apps/hook/views/v2/__init__.py +++ b/kobo/apps/hook/views/v2/__init__.py @@ -1,4 +1,3 @@ # coding: utf-8 from .hook import HookViewSet from .hook_log import HookLogViewSet -from .hook_signal import HookSignalViewSet diff --git a/kobo/apps/hook/views/v2/hook_signal.py b/kobo/apps/hook/views/v2/hook_signal.py deleted file mode 100644 index d9150f9fa5..0000000000 --- a/kobo/apps/hook/views/v2/hook_signal.py +++ /dev/null @@ -1,84 +0,0 @@ -# coding: utf-8 -from django.http import Http404 -from django.utils.translation import gettext_lazy as t -from rest_framework import status, viewsets, serializers -from rest_framework.response import Response -from rest_framework.pagination import _positive_int as positive_int -from rest_framework_extensions.mixins import NestedViewSetMixin - - -from kobo.apps.hook.utils import HookUtils -from kpi.models import Asset -from kpi.utils.viewset_mixins import AssetNestedObjectViewsetMixin - - -class HookSignalViewSet(AssetNestedObjectViewsetMixin, NestedViewSetMixin, - viewsets.ViewSet): - """ - ## - This endpoint is only used to trigger asset's hooks if any. - - Tells the hooks to post an instance to external servers. - 
-    POST /api/v2/assets/{uid}/hook-signal/
-
- - - > Example - > - > curl -X POST https://[kpi-url]/api/v2/assets/aSAvYreNzVEkrWg5Gdcvg/hook-signal/ - - - > **Expected payload** - > - > { - > "submission_id": {integer} - > } - - """ - - parent_model = Asset - - def create(self, request, *args, **kwargs): - """ - It's only used to trigger hook services of the Asset (so far). - - :param request: - :return: - """ - try: - submission_id = positive_int( - request.data.get('submission_id'), strict=True) - except ValueError: - raise serializers.ValidationError( - {'submission_id': t('A positive integer is required.')}) - - # Check if instance really belongs to Asset. - try: - submission = self.asset.deployment.get_submission(submission_id, - request.user) - except ValueError: - raise Http404 - - if not (submission and int(submission['_id']) == submission_id): - raise Http404 - - if HookUtils.call_services(self.asset.uid, submission_id): - # Follow Open Rosa responses by default - response_status_code = status.HTTP_202_ACCEPTED - response = { - "detail": t( - "We got and saved your data, but may not have " - "fully processed it. You should not try to resubmit.") - } - else: - # call_services() refused to launch any task because this - # instance already has a `HookLog` - response_status_code = status.HTTP_409_CONFLICT - response = { - "detail": t( - "Your data for instance {} has been already " - "submitted.".format(submission_id)) - } - - return Response(response, status=response_status_code) diff --git a/kobo/apps/openrosa/apps/api/tests/viewsets/test_xform_viewset.py b/kobo/apps/openrosa/apps/api/tests/viewsets/test_xform_viewset.py index 2831e70fb3..cd79a93c39 100644 --- a/kobo/apps/openrosa/apps/api/tests/viewsets/test_xform_viewset.py +++ b/kobo/apps/openrosa/apps/api/tests/viewsets/test_xform_viewset.py @@ -477,7 +477,6 @@ def test_xform_serializer_none(self): 'instances_with_geopoints': False, 'num_of_submissions': 0, 'attachment_storage_bytes': 0, - 'has_kpi_hooks': False, 'kpi_asset_uid': '', } self.assertEqual(data, XFormSerializer(None).data) diff --git a/kobo/apps/openrosa/apps/logger/migrations/0035_remove_xform_has_kpi_hooks_and_instance_posted_to_kpi.py b/kobo/apps/openrosa/apps/logger/migrations/0035_remove_xform_has_kpi_hooks_and_instance_posted_to_kpi.py new file mode 100644 index 0000000000..b6e48241c8 --- /dev/null +++ b/kobo/apps/openrosa/apps/logger/migrations/0035_remove_xform_has_kpi_hooks_and_instance_posted_to_kpi.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.11 on 2024-07-31 15:59 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import kobo.apps.openrosa.apps.logger.models.attachment +import kobo.apps.openrosa.apps.logger.models.xform +import kpi.deployment_backends.kc_access.storage + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('logger', '0034_set_require_auth_at_project_level'), + ] + + operations = [ + migrations.RemoveField( + model_name='xform', + name='has_kpi_hooks', + ), + migrations.RemoveField( + model_name='instance', + name='posted_to_kpi', + ), + ] diff --git a/kobo/apps/openrosa/apps/logger/models/instance.py b/kobo/apps/openrosa/apps/logger/models/instance.py index 86a52500f0..7b685d261b 100644 --- a/kobo/apps/openrosa/apps/logger/models/instance.py +++ b/kobo/apps/openrosa/apps/logger/models/instance.py @@ -106,10 +106,6 @@ class Instance(models.Model): # TODO Don't forget to update all records with command `update_is_sync_with_mongo`. is_synced_with_mongo = LazyDefaultBooleanField(default=False) - # If XForm.has_kpi_hooks` is True, this field should be True either. - # It tells whether the instance has been successfully sent to KPI. - posted_to_kpi = LazyDefaultBooleanField(default=False) - class Meta: app_label = 'logger' diff --git a/kobo/apps/openrosa/apps/logger/models/xform.py b/kobo/apps/openrosa/apps/logger/models/xform.py index c7fd168a59..ceb8614082 100644 --- a/kobo/apps/openrosa/apps/logger/models/xform.py +++ b/kobo/apps/openrosa/apps/logger/models/xform.py @@ -96,7 +96,6 @@ class XForm(BaseModel): tags = TaggableManager() - has_kpi_hooks = LazyDefaultBooleanField(default=False) kpi_asset_uid = models.CharField(max_length=32, null=True) pending_delete = models.BooleanField(default=False) @@ -166,14 +165,6 @@ def data_dictionary(self, use_cache: bool = False): def has_instances_with_geopoints(self): return self.instances_with_geopoints - @property - def kpi_hook_service(self): - """ - Returns kpi hook service if it exists. XForm should have only one occurrence in any case. - :return: RestService - """ - return self.restservices.filter(name="kpi_hook").first() - def _set_id_string(self): matches = self.instance_id_regex.findall(self.xml) if len(matches) != 1: diff --git a/kobo/apps/openrosa/apps/main/migrations/0015_drop_old_restservice_tables.py b/kobo/apps/openrosa/apps/main/migrations/0015_drop_old_restservice_tables.py new file mode 100644 index 0000000000..1b58d492d7 --- /dev/null +++ b/kobo/apps/openrosa/apps/main/migrations/0015_drop_old_restservice_tables.py @@ -0,0 +1,77 @@ +# Generated by Django 4.2.11 on 2024-07-31 15:59 + +from django.db import migrations, connections +from django.conf import settings + + +KC_REST_SERVICES_TABLES = [ + 'restservice_restservice', +] + + +def get_operations(): + if settings.TESTING or settings.SKIP_HEAVY_MIGRATIONS: + # Skip this migration if running in test environment or because we want + # to voluntarily skip it. + return [] + + tables = KC_REST_SERVICES_TABLES + operations = [] + + sql = """ + SELECT con.conname + FROM pg_catalog.pg_constraint con + INNER JOIN pg_catalog.pg_class rel + ON rel.oid = con.conrelid + INNER JOIN pg_catalog.pg_namespace nsp + ON nsp.oid = connamespace + WHERE nsp.nspname = 'public' + AND rel.relname = %s; + """ + with connections[settings.OPENROSA_DB_ALIAS].cursor() as cursor: + drop_table_queries = [] + for table in tables: + cursor.execute(sql, [table]) + drop_index_queries = [] + for row in cursor.fetchall(): + if not row[0].endswith('_pkey'): + drop_index_queries.append( + f'ALTER TABLE public.{table} DROP CONSTRAINT {row[0]};' + ) + drop_table_queries.append(f'DROP TABLE IF EXISTS {table};') + operations.append( + migrations.RunSQL( + sql=''.join(drop_index_queries), + reverse_sql=migrations.RunSQL.noop, + ) + ) + + operations.append( + migrations.RunSQL( + sql=''.join(drop_table_queries), + reverse_sql=migrations.RunSQL.noop, + ) + ) + + return operations + + +def print_migration_warning(apps, schema_editor): + if settings.TESTING or settings.SKIP_HEAVY_MIGRATIONS: + return + print( + """ + This migration might take a while. If it is too slow, you may want to + re-run migrations with SKIP_HEAVY_MIGRATIONS=True and apply this one + manually from the django shell. + """ + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('main', '0014_drop_old_formdisclaimer_tables'), + ] + + operations = [migrations.RunPython(print_migration_warning), *get_operations()] diff --git a/kobo/apps/openrosa/apps/restservice/RestServiceInterface.py b/kobo/apps/openrosa/apps/restservice/RestServiceInterface.py deleted file mode 100644 index 28495d5a5c..0000000000 --- a/kobo/apps/openrosa/apps/restservice/RestServiceInterface.py +++ /dev/null @@ -1,4 +0,0 @@ -# coding: utf-8 -class RestServiceInterface: - def send(self, url, data=None): - raise NotImplementedError diff --git a/kobo/apps/openrosa/apps/restservice/__init__.py b/kobo/apps/openrosa/apps/restservice/__init__.py deleted file mode 100644 index 7fe25636ee..0000000000 --- a/kobo/apps/openrosa/apps/restservice/__init__.py +++ /dev/null @@ -1,9 +0,0 @@ -# coding: utf-8 -SERVICE_KPI_HOOK = ("kpi_hook", "KPI Hook POST") - -SERVICE_CHOICES = ( - SERVICE_KPI_HOOK, -) - - -default_app_config = "kobo.apps.openrosa.apps.restservice.app.RestServiceConfig" diff --git a/kobo/apps/openrosa/apps/restservice/app.py b/kobo/apps/openrosa/apps/restservice/app.py deleted file mode 100644 index 32c379ee84..0000000000 --- a/kobo/apps/openrosa/apps/restservice/app.py +++ /dev/null @@ -1,12 +0,0 @@ -# coding: utf-8 -from django.apps import AppConfig - - -class RestServiceConfig(AppConfig): - name = 'kobo.apps.openrosa.apps.restservice' - verbose_name = 'restservice' - - def ready(self): - # Register RestService signals - from . import signals - super().ready() diff --git a/kobo/apps/openrosa/apps/restservice/management/__init__.py b/kobo/apps/openrosa/apps/restservice/management/__init__.py deleted file mode 100644 index 57d631c3f0..0000000000 --- a/kobo/apps/openrosa/apps/restservice/management/__init__.py +++ /dev/null @@ -1 +0,0 @@ -# coding: utf-8 diff --git a/kobo/apps/openrosa/apps/restservice/management/commands/__init__.py b/kobo/apps/openrosa/apps/restservice/management/commands/__init__.py deleted file mode 100644 index 57d631c3f0..0000000000 --- a/kobo/apps/openrosa/apps/restservice/management/commands/__init__.py +++ /dev/null @@ -1 +0,0 @@ -# coding: utf-8 diff --git a/kobo/apps/openrosa/apps/restservice/management/commands/update_kpi_hooks_endpoint.py b/kobo/apps/openrosa/apps/restservice/management/commands/update_kpi_hooks_endpoint.py deleted file mode 100644 index 64a3a79053..0000000000 --- a/kobo/apps/openrosa/apps/restservice/management/commands/update_kpi_hooks_endpoint.py +++ /dev/null @@ -1,41 +0,0 @@ -# coding: utf-8 -from django.core.management.base import BaseCommand - -from kobo.apps.openrosa.apps.restservice.models import RestService - - -class Command(BaseCommand): - """ - A faster method is available with PostgreSQL: - UPDATE restservice_restservice - SET service_url = REGEXP_REPLACE( - service_url, - '/assets/([^/]*)/submissions/', - '/api/v2/assets/\1/hook-signal/' - ) - WHERE service_url LIKE '/assets/%'; - """ - - help = 'Updates KPI rest service endpoint' - - def handle(self, *args, **kwargs): - - rest_services = RestService.objects.filter(name='kpi_hook').all() - for rest_service in rest_services: - service_url = rest_service.service_url - do_save = False - if service_url.endswith('/submissions/'): - service_url = service_url.replace('/submissions/', '/hook-signal/') - rest_service.service_url = service_url - do_save = True - rest_service.save(update_fields=["service_url"]) - - if service_url.startswith('/assets/'): - service_url = service_url.replace('/assets/', '/api/v2/assets/') - rest_service.service_url = service_url - do_save = True - - if do_save: - rest_service.save(update_fields=["service_url"]) - - print('Done!') diff --git a/kobo/apps/openrosa/apps/restservice/migrations/0001_initial.py b/kobo/apps/openrosa/apps/restservice/migrations/0001_initial.py deleted file mode 100644 index 0d68804e6d..0000000000 --- a/kobo/apps/openrosa/apps/restservice/migrations/0001_initial.py +++ /dev/null @@ -1,25 +0,0 @@ -# coding: utf-8 -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('logger', '0001_initial'), - ] - - operations = [ - migrations.CreateModel( - name='RestService', - fields=[ - ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), - ('service_url', models.URLField(verbose_name='Service URL')), - ('name', models.CharField(max_length=50, choices=[('f2dhis2', 'f2dhis2'), ('generic_json', 'JSON POST'), ('generic_xml', 'XML POST'), ('bamboo', 'bamboo')])), - ('xform', models.ForeignKey(to='logger.XForm', on_delete=models.CASCADE)), - ], - ), - migrations.AlterUniqueTogether( - name='restservice', - unique_together=set([('service_url', 'xform', 'name')]), - ), - ] diff --git a/kobo/apps/openrosa/apps/restservice/migrations/0002_add_related_name_with_delete_on_cascade.py b/kobo/apps/openrosa/apps/restservice/migrations/0002_add_related_name_with_delete_on_cascade.py deleted file mode 100644 index c2d6cf46c3..0000000000 --- a/kobo/apps/openrosa/apps/restservice/migrations/0002_add_related_name_with_delete_on_cascade.py +++ /dev/null @@ -1,22 +0,0 @@ -# coding: utf-8 -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('restservice', '0001_initial'), - ] - - operations = [ - migrations.AlterField( - model_name='restservice', - name='name', - field=models.CharField(max_length=50, choices=[('f2dhis2', 'f2dhis2'), ('generic_json', 'JSON POST'), ('generic_xml', 'XML POST'), ('bamboo', 'bamboo'), ('kpi_hook', 'KPI Hook POST')]), - ), - migrations.AlterField( - model_name='restservice', - name='xform', - field=models.ForeignKey(related_name='restservices', to='logger.XForm', on_delete=models.CASCADE), - ), - ] diff --git a/kobo/apps/openrosa/apps/restservice/migrations/0003_remove_deprecated_services.py b/kobo/apps/openrosa/apps/restservice/migrations/0003_remove_deprecated_services.py deleted file mode 100644 index 306e80da8f..0000000000 --- a/kobo/apps/openrosa/apps/restservice/migrations/0003_remove_deprecated_services.py +++ /dev/null @@ -1,17 +0,0 @@ -# coding: utf-8 -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('restservice', '0002_add_related_name_with_delete_on_cascade'), - ] - - operations = [ - migrations.AlterField( - model_name='restservice', - name='name', - field=models.CharField(max_length=50, choices=[('kpi_hook', 'KPI Hook POST')]), - ), - ] diff --git a/kobo/apps/openrosa/apps/restservice/migrations/__init__.py b/kobo/apps/openrosa/apps/restservice/migrations/__init__.py deleted file mode 100644 index 57d631c3f0..0000000000 --- a/kobo/apps/openrosa/apps/restservice/migrations/__init__.py +++ /dev/null @@ -1 +0,0 @@ -# coding: utf-8 diff --git a/kobo/apps/openrosa/apps/restservice/models.py b/kobo/apps/openrosa/apps/restservice/models.py deleted file mode 100644 index f93945a267..0000000000 --- a/kobo/apps/openrosa/apps/restservice/models.py +++ /dev/null @@ -1,31 +0,0 @@ -# coding: utf-8 -from django.db import models -from django.utils.translation import gettext_lazy - -from kobo.apps.openrosa.apps.logger.models.xform import XForm -from kobo.apps.openrosa.apps.restservice import SERVICE_CHOICES - - -class RestService(models.Model): - - class Meta: - app_label = 'restservice' - unique_together = ('service_url', 'xform', 'name') - - service_url = models.URLField(gettext_lazy("Service URL")) - xform = models.ForeignKey(XForm, related_name="restservices", on_delete=models.CASCADE) - name = models.CharField(max_length=50, choices=SERVICE_CHOICES) - - def __str__(self): - return "%s:%s - %s" % (self.xform, self.long_name, self.service_url) - - def get_service_definition(self): - m = __import__(''.join(['kobo.apps.openrosa.apps.restservice.services.', - self.name]), - globals(), locals(), ['ServiceDefinition']) - return m.ServiceDefinition - - @property - def long_name(self): - sv = self.get_service_definition() - return sv.verbose_name diff --git a/kobo/apps/openrosa/apps/restservice/services/__init__.py b/kobo/apps/openrosa/apps/restservice/services/__init__.py deleted file mode 100644 index f6b69c77ea..0000000000 --- a/kobo/apps/openrosa/apps/restservice/services/__init__.py +++ /dev/null @@ -1,2 +0,0 @@ -# coding: utf-8 -__all__ = ('kpi_hook') diff --git a/kobo/apps/openrosa/apps/restservice/services/kpi_hook.py b/kobo/apps/openrosa/apps/restservice/services/kpi_hook.py deleted file mode 100644 index 4d0f7127fe..0000000000 --- a/kobo/apps/openrosa/apps/restservice/services/kpi_hook.py +++ /dev/null @@ -1,43 +0,0 @@ -# coding: utf-8 -import logging -import re - -import requests -from django.conf import settings -from kobo.apps.openrosa.apps.restservice.RestServiceInterface import RestServiceInterface -from kobo.apps.openrosa.apps.logger.models import Instance - - -class ServiceDefinition(RestServiceInterface): - id = 'kpi_hook' - verbose_name = 'KPI Hook POST' - - def send(self, endpoint, data): - - # Will be used internally by KPI to fetch data with KoBoCatBackend - post_data = { - 'submission_id': data.get('instance_id') - } - headers = {'Content-Type': 'application/json'} - - # Verify if endpoint starts with `/assets/` before sending - # the request to KPI - pattern = r'{}'.format(settings.KPI_HOOK_ENDPOINT_PATTERN.replace( - '{asset_uid}', '[^/]*')) - - # Match v2 and v1 endpoints. - if re.match(pattern, endpoint) or re.match(pattern[7:], endpoint): - # Build the url in the service to avoid saving hardcoded - # domain name in the DB - url = f'{settings.KOBOFORM_INTERNAL_URL}{endpoint}' - response = requests.post(url, headers=headers, json=post_data) - response.raise_for_status() - - # Save successful - Instance.objects.filter(pk=data.get('instance_id')).update( - posted_to_kpi=True - ) - else: - logging.warning( - f'This endpoint: `{endpoint}` is not valid for `KPI Hook`' - ) diff --git a/kobo/apps/openrosa/apps/restservice/signals.py b/kobo/apps/openrosa/apps/restservice/signals.py deleted file mode 100644 index 80ae3b874c..0000000000 --- a/kobo/apps/openrosa/apps/restservice/signals.py +++ /dev/null @@ -1,36 +0,0 @@ -# coding: utf-8 -from django.conf import settings -from django.db.models.signals import post_save -from django.dispatch import receiver - -from kobo.apps.openrosa.apps.restservice import SERVICE_KPI_HOOK -from kobo.apps.openrosa.apps.logger.models import XForm -from kobo.apps.openrosa.apps.restservice.models import RestService - - -@receiver(post_save, sender=XForm) -def save_kpi_hook_service(sender, instance, **kwargs): - """ - Creates/Deletes Kpi hook Rest service related to XForm instance - :param sender: XForm class - :param instance: XForm instance - :param kwargs: dict - """ - kpi_hook_service = instance.kpi_hook_service - if instance.has_kpi_hooks: - # Only register the service if it hasn't been created yet. - if kpi_hook_service is None: - # For retro-compatibility, if `asset_uid` is null, fallback on - # `id_string` - asset_uid = instance.kpi_asset_uid if instance.kpi_asset_uid \ - else instance.id_string - kpi_hook_service = RestService( - service_url=settings.KPI_HOOK_ENDPOINT_PATTERN.format( - asset_uid=asset_uid), - xform=instance, - name=SERVICE_KPI_HOOK[0] - ) - kpi_hook_service.save() - elif kpi_hook_service is not None: - # Only delete the service if it already exists. - kpi_hook_service.delete() diff --git a/kobo/apps/openrosa/apps/restservice/tasks.py b/kobo/apps/openrosa/apps/restservice/tasks.py deleted file mode 100644 index ce67dfd5e2..0000000000 --- a/kobo/apps/openrosa/apps/restservice/tasks.py +++ /dev/null @@ -1,35 +0,0 @@ -# coding: utf-8 -import logging - -from celery import shared_task -from django.conf import settings - -from kobo.apps.openrosa.apps.restservice.models import RestService - - -@shared_task(bind=True) -def service_definition_task(self, rest_service_id, data): - """ - Tries to send data to the endpoint of the hook - It retries 3 times maximum. - - after 2 minutes, - - after 20 minutes, - - after 200 minutes - - :param self: Celery.Task. - :param rest_service_id: RestService primary key. - :param data: dict. - """ - try: - rest_service = RestService.objects.get(pk=rest_service_id) - service = rest_service.get_service_definition()() - service.send(rest_service.service_url, data) - except Exception as e: - logger = logging.getLogger("console_logger") - logger.error("service_definition_task - {}".format(str(e)), exc_info=True) - # Countdown is in seconds - countdown = 120 * (10 ** self.request.retries) - # Max retries is 3 by default. - raise self.retry(countdown=countdown, max_retries=settings.REST_SERVICE_MAX_RETRIES) - - return True diff --git a/kobo/apps/openrosa/apps/restservice/templates/add-service.html b/kobo/apps/openrosa/apps/restservice/templates/add-service.html deleted file mode 100644 index 717082f50e..0000000000 --- a/kobo/apps/openrosa/apps/restservice/templates/add-service.html +++ /dev/null @@ -1,11 +0,0 @@ -{% load i18n %} -{% block content %} -
-

- Please manage REST Services within the new user interface. Go to the - Project Dashboard and navigate to - - Your Project > Settings > REST Services. - -

-{% endblock %} diff --git a/kobo/apps/openrosa/apps/restservice/tests/__init__.py b/kobo/apps/openrosa/apps/restservice/tests/__init__.py deleted file mode 100644 index 57d631c3f0..0000000000 --- a/kobo/apps/openrosa/apps/restservice/tests/__init__.py +++ /dev/null @@ -1 +0,0 @@ -# coding: utf-8 diff --git a/kobo/apps/openrosa/apps/restservice/tests/fixtures/dhisform.xls b/kobo/apps/openrosa/apps/restservice/tests/fixtures/dhisform.xls deleted file mode 100755 index d0b29d74b7c51a8b21223b776b499207ea71d6ed..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 6656 zcmeHLTWnNS6y5jEOu6ONX?a*c@Cud|T2Md)G05;JkEBS$BNAgQ!^~|vGMyQxw*(T2 zgUX*I8bACYAtd}5jgOe%ha|Rten>PVd}v})^urj93`Sxwz_`{qciPJ^Bh!XxAU!kd zoW1vX?DN?BJnpyO)DNHebnay-3C~DCZWU^!#wRyGPcHId5fe0Jw+e+qQ4De-+($ET z)9WkOFo;DA0bKJ*fCbb7%$wq+_tDTMx?05y?3OfsuGrEpDWq9Bij}TD%!bAI)l^f{ zzh6{S{%&O^^pDQK-R6q<{`u$r&3!xoRPulB`Sky(z%*bwPzT@$kb0m2m;uZLW&yK- zhk!<44loy(2RsbS2NnR205UF+cpTt3R(L7t zCqb|F_%&WSIx`bgxf!Unu!=Py`C{7F-161le;F3>NpMKn#gCA^C>h$u=4diz|5ss? z>O<6^Z4San*Be3HHB31LOqY+9{Je_SEBiySh}dh9@J_PdjG&yzNfj?t@pQz7e6H$z zuOusAnO~t);kCCtHwYOUu>&R~DoH8HMY(&OYsqIJP=%dvZw$ZJ&jJLOVF{;mm+&q;x4I2TZ9eXd5O4Y{C7 zXXGZSbY?D8H~_ygDhGL{cPLGpC*D~qMKr-qk(y6Y2RNTpS zt;YG-IU=<8QD|>X9A>pxZ#ofk`q6JU?M9Pbqm?1rOOZWVmA!Y4u%F3vOlbhw;gQIU zM_mUCk#Vw#bR0ydA}~rf;U*o-R3hGmF=07CZ`rK%U+b|aC#`{+&97oPjaH*=%s^ZsG+2ZBm4JCV1#fwq@%I^$6Z)o<>Eub$pAH)C3h9y4wj4Z^1 zCJvP}KU3O_He<@U1NaYbD5i$&Lrz$ZsbL$^RL`r9DBeU))2vZGfNPIGAAK|ydF93A zHAz3H^fSsbX_BPp(|RRh)Cpx|YGoa*dPGJhP$J_MtyG59xlMM{d!Xy((VT%x8FgsR z1jp7j7r(Oc&)n!xXu^#7>9Q)g-E})uYNzSb1Sdy1gavV#Yx*llG0LP1} z=1g$3s^6p&l$qZS8aA5eNYnzMdYuIFPk^tJV7^7kYx8;)!T5qHlT|&cv^DO$~HlJsiRd|+i7&Vzi&oaw+ zmg(~%t~)~`tXEqBSGrL>SeU16mrobXF^`WJzlGR-3&FyT*+Nif#6r-vNJPatQWm0& z51rvpU>*NzA->AvAq~8u0+^nO0s{s9y z`Rq7OJhcEcj1GXC(|&;4QXJrWHv>?^FE{numGmkg0?1EoFENQQ@/hook-signal/` - response = self.client.post( - reverse( - self._get_endpoint('hook-signal-list'), - kwargs={ - 'format': 'json', - 'parent_lookup_asset': self.asset.uid, - }, - ), - data=data, - **headers, - ) - # return a 202 first time but 409 other attempts. - assert response.status_code != status.HTTP_403_FORBIDDEN diff --git a/kpi/urls/router_api_v1.py b/kpi/urls/router_api_v1.py index 4e53c7ae8c..368101a9e3 100644 --- a/kpi/urls/router_api_v1.py +++ b/kpi/urls/router_api_v1.py @@ -3,7 +3,6 @@ from kobo.apps.hook.views.v1.hook import HookViewSet from kobo.apps.hook.views.v1.hook_log import HookLogViewSet -from kobo.apps.hook.views.v1.hook_signal import HookSignalViewSet from kobo.apps.reports.views import ReportsViewSet from kpi.views.v1 import ( @@ -29,11 +28,6 @@ basename='asset-version', parents_query_lookups=['asset'], ) -asset_routes.register(r'hook-signal', - HookSignalViewSet, - basename='hook-signal', - parents_query_lookups=['asset'], - ) asset_routes.register(r'submissions', SubmissionViewSet, basename='submission', diff --git a/kpi/urls/router_api_v2.py b/kpi/urls/router_api_v2.py index 9883884fc4..7c7dbd2575 100644 --- a/kpi/urls/router_api_v2.py +++ b/kpi/urls/router_api_v2.py @@ -5,7 +5,6 @@ from kobo.apps.audit_log.urls import router as audit_log_router from kobo.apps.hook.views.v2.hook import HookViewSet from kobo.apps.hook.views.v2.hook_log import HookLogViewSet -from kobo.apps.hook.views.v2.hook_signal import HookSignalViewSet from kobo.apps.languages.urls import router as language_router from kobo.apps.organizations.views import OrganizationViewSet from kobo.apps.project_ownership.urls import router as project_ownership_router @@ -104,12 +103,6 @@ def get_urls(self, *args, **kwargs): parents_query_lookups=['asset'], ) -asset_routes.register(r'hook-signal', - HookSignalViewSet, - basename='hook-signal', - parents_query_lookups=['asset'], - ) - asset_routes.register(r'paired-data', PairedDataViewset, basename='paired-data', From 5943b2451f6f585eb4a856a7348d43a16a43eabd Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Thu, 1 Aug 2024 12:23:29 -0400 Subject: [PATCH 3/6] Improve celery retries --- kobo/apps/hook/constants.py | 9 + kobo/apps/hook/exceptions.py | 3 + kobo/apps/hook/models/hook_log.py | 65 ++---- .../models/service_definition_interface.py | 214 +++++++++++------- kobo/apps/hook/tasks.py | 34 ++- kobo/apps/hook/tests/test_utils.py | 8 +- kobo/apps/hook/utils.py | 33 --- kobo/apps/hook/utils/__init__.py | 0 kobo/apps/hook/utils/lazy.py | 44 ++++ kobo/apps/hook/utils/services.py | 27 +++ kobo/apps/hook/views/v2/hook.py | 25 +- kobo/apps/hook/views/v2/hook_log.py | 2 +- .../apps/viewer/models/parsed_instance.py | 4 +- kobo/settings/base.py | 2 +- 14 files changed, 272 insertions(+), 198 deletions(-) create mode 100644 kobo/apps/hook/exceptions.py delete mode 100644 kobo/apps/hook/utils.py create mode 100644 kobo/apps/hook/utils/__init__.py create mode 100644 kobo/apps/hook/utils/lazy.py create mode 100644 kobo/apps/hook/utils/services.py diff --git a/kobo/apps/hook/constants.py b/kobo/apps/hook/constants.py index 00998fc333..7b84fa03bb 100644 --- a/kobo/apps/hook/constants.py +++ b/kobo/apps/hook/constants.py @@ -1,5 +1,6 @@ # coding: utf-8 from enum import Enum +from rest_framework import status HOOK_LOG_FAILED = 0 @@ -16,3 +17,11 @@ class HookLogStatus(Enum): KOBO_INTERNAL_ERROR_STATUS_CODE = None SUBMISSION_PLACEHOLDER = '%SUBMISSION%' + +# Status codes that trigger a retry +RETRIABLE_STATUS_CODES = [ + # status.HTTP_429_TOO_MANY_REQUESTS, + status.HTTP_502_BAD_GATEWAY, + status.HTTP_503_SERVICE_UNAVAILABLE, + status.HTTP_504_GATEWAY_TIMEOUT, +] diff --git a/kobo/apps/hook/exceptions.py b/kobo/apps/hook/exceptions.py new file mode 100644 index 0000000000..1997f47c2e --- /dev/null +++ b/kobo/apps/hook/exceptions.py @@ -0,0 +1,3 @@ + +class HookRemoteServerDownError(Exception): + pass diff --git a/kobo/apps/hook/models/hook_log.py b/kobo/apps/hook/models/hook_log.py index 51c1a8a2f9..2cba30815f 100644 --- a/kobo/apps/hook/models/hook_log.py +++ b/kobo/apps/hook/models/hook_log.py @@ -1,4 +1,3 @@ -# coding: utf-8 from datetime import timedelta import constance @@ -17,39 +16,44 @@ class HookLog(models.Model): - hook = models.ForeignKey("Hook", related_name="logs", on_delete=models.CASCADE) + hook = models.ForeignKey( + "Hook", related_name="logs", on_delete=models.CASCADE + ) uid = KpiUidField(uid_prefix="hl") - submission_id = models.IntegerField(default=0, db_index=True) # `KoBoCAT.logger.Instance.id` + submission_id = models.IntegerField( # `KoboCAT.logger.Instance.id` + default=0, db_index=True + ) tries = models.PositiveSmallIntegerField(default=0) status = models.PositiveSmallIntegerField( choices=[[e.value, e.name.title()] for e in HookLogStatus], - default=HookLogStatus.PENDING.value + default=HookLogStatus.PENDING.value, ) # Could use status_code, but will speed-up queries - status_code = models.IntegerField(default=KOBO_INTERNAL_ERROR_STATUS_CODE, null=True, blank=True) + status_code = models.IntegerField( + default=KOBO_INTERNAL_ERROR_STATUS_CODE, null=True, blank=True + ) message = models.TextField(default="") date_created = models.DateTimeField(auto_now_add=True) date_modified = models.DateTimeField(auto_now_add=True) class Meta: - ordering = ["-date_created"] + ordering = ['-date_created'] + @property def can_retry(self) -> bool: """ Return whether instance can be resent to external endpoint. Notice: even if False is returned, `self.retry()` can be triggered. """ if self.hook.active: - seconds = HookLog.get_elapsed_seconds( - constance.config.HOOK_MAX_RETRIES - ) - threshold = timezone.now() - timedelta(seconds=seconds) - # We can retry only if system has already tried 3 times. - # If log is still pending after 3 times, there was an issue, - # we allow the retry - return ( - self.status == HOOK_LOG_FAILED - or (self.date_modified < threshold and self.status == HOOK_LOG_PENDING) - ) + if self.tries >= constance.config.HOOK_MAX_RETRIES: + # If log is still pending after `constance.config.HOOK_MAX_RETRIES` + # times, there was an issue, we allow the retry. + threshold = timezone.now() - timedelta(seconds=120) + + return self.status == HOOK_LOG_FAILED or ( + self.date_modified < threshold + and self.status == HOOK_LOG_PENDING + ) return False @@ -66,29 +70,6 @@ def change_status( self.save(reset_status=True) - @staticmethod - def get_elapsed_seconds(retries_count: int) -> int: - """ - Calculate number of elapsed seconds since first try. - Return the number of seconds. - """ - # We need to sum all seconds between each retry - seconds = 0 - for retries_count in range(retries_count): - # Range is zero-indexed - seconds += HookLog.get_remaining_seconds(retries_count) - - return seconds - - @staticmethod - def get_remaining_seconds(retries_count): - """ - Calculate number of remaining seconds before next retry - :param retries_count: int. - :return: int. Number of seconds - """ - return 60 * (10 ** retries_count) - def retry(self): """ Retries to send data to external service @@ -100,7 +81,7 @@ def retry(self): service_definition.send() self.refresh_from_db() except Exception as e: - logging.error("HookLog.retry - {}".format(str(e)), exc_info=True) + logging.error('HookLog.retry - {}'.format(str(e)), exc_info=True) self.change_status(HOOK_LOG_FAILED) return False @@ -110,7 +91,7 @@ def save(self, *args, **kwargs): # Update date_modified each time object is saved self.date_modified = timezone.now() # We don't want to alter tries when we only change the status - if kwargs.pop("reset_status", False) is False: + if kwargs.pop('reset_status', False) is False: self.tries += 1 self.hook.reset_totals() super().save(*args, **kwargs) diff --git a/kobo/apps/hook/models/service_definition_interface.py b/kobo/apps/hook/models/service_definition_interface.py index e721bf45b7..9b2bd1a095 100644 --- a/kobo/apps/hook/models/service_definition_interface.py +++ b/kobo/apps/hook/models/service_definition_interface.py @@ -15,7 +15,9 @@ HOOK_LOG_SUCCESS, HOOK_LOG_FAILED, KOBO_INTERNAL_ERROR_STATUS_CODE, + RETRIABLE_STATUS_CODES, ) +from ..exceptions import HookRemoteServerDownError class ServiceDefinitionInterface(metaclass=ABCMeta): @@ -41,7 +43,8 @@ def _get_data(self): 'service_json.ServiceDefinition._get_data: ' f'Hook #{self._hook.uid} - Data #{self._submission_id} - ' f'{str(e)}', - exc_info=True) + exc_info=True, + ) return None @abstractmethod @@ -71,106 +74,141 @@ def _prepare_request_kwargs(self): """ pass - def send(self): + def send(self) -> bool: """ - Sends data to external endpoint - :return: bool + Sends data to external endpoint. + + Raise an exception if something is wrong. Retries are only allowed + when `HookRemoteServerDownError` is raised. """ - success = False + if not self._data: + self.save_log( + KOBO_INTERNAL_ERROR_STATUS_CODE, 'Submission has been deleted', allow_retries=False + ) + return False + # Need to declare response before requests.post assignment in case of # RequestException response = None - if self._data: - try: - request_kwargs = self._prepare_request_kwargs() - - # Add custom headers - request_kwargs.get("headers").update( - self._hook.settings.get("custom_headers", {})) - - # Add user agent - public_domain = "- {} ".format(os.getenv("PUBLIC_DOMAIN_NAME")) \ - if os.getenv("PUBLIC_DOMAIN_NAME") else "" - request_kwargs.get("headers").update({ - "User-Agent": "KoboToolbox external service {}#{}".format( - public_domain, - self._hook.uid) - }) - - # If the request needs basic authentication with username and - # password, let's provide them - if self._hook.auth_level == Hook.BASIC_AUTH: - request_kwargs.update({ - "auth": (self._hook.settings.get("username"), - self._hook.settings.get("password")) - }) - - ssrf_protect_options = {} - if constance.config.SSRF_ALLOWED_IP_ADDRESS.strip(): - ssrf_protect_options['allowed_ip_addresses'] = constance.\ - config.SSRF_ALLOWED_IP_ADDRESS.strip().split('\r\n') - - if constance.config.SSRF_DENIED_IP_ADDRESS.strip(): - ssrf_protect_options['denied_ip_addresses'] = constance.\ - config.SSRF_DENIED_IP_ADDRESS.strip().split('\r\n') - - SSRFProtect.validate(self._hook.endpoint, - options=ssrf_protect_options) - - response = requests.post(self._hook.endpoint, timeout=30, - **request_kwargs) - response.raise_for_status() - self.save_log(response.status_code, response.text, True) - success = True - except requests.exceptions.RequestException as e: - # If request fails to communicate with remote server. - # Exception is raised before request.post can return something. - # Thus, response equals None - status_code = KOBO_INTERNAL_ERROR_STATUS_CODE - text = str(e) - if response is not None: - text = response.text - status_code = response.status_code - self.save_log(status_code, text) - except SSRFProtectException as e: - logging.error( - 'service_json.ServiceDefinition.send: ' - f'Hook #{self._hook.uid} - ' - f'Data #{self._submission_id} - ' - f'{str(e)}', - exc_info=True) - self.save_log( - KOBO_INTERNAL_ERROR_STATUS_CODE, - f'{self._hook.endpoint} is not allowed') - except Exception as e: - logging.error( - 'service_json.ServiceDefinition.send: ' - f'Hook #{self._hook.uid} - ' - f'Data #{self._submission_id} - ' - f'{str(e)}', - exc_info=True) - self.save_log( - KOBO_INTERNAL_ERROR_STATUS_CODE, - "An error occurred when sending data to external endpoint") - else: - self.save_log( - KOBO_INTERNAL_ERROR_STATUS_CODE, - 'Submission has been deleted' + try: + request_kwargs = self._prepare_request_kwargs() + + # Add custom headers + request_kwargs.get('headers').update( + self._hook.settings.get('custom_headers', {}) ) - return success + # Add user agent + public_domain = ( + '- {} '.format(os.getenv('PUBLIC_DOMAIN_NAME')) + if os.getenv('PUBLIC_DOMAIN_NAME') + else '' + ) + request_kwargs.get('headers').update( + { + 'User-Agent': 'KoboToolbox external service {}#{}'.format( + public_domain, self._hook.uid + ) + } + ) - def save_log(self, status_code: int, message: str, success: bool = False): + # If the request needs basic authentication with username and + # password, let's provide them + if self._hook.auth_level == Hook.BASIC_AUTH: + request_kwargs.update( + { + 'auth': ( + self._hook.settings.get('username'), + self._hook.settings.get('password'), + ) + } + ) + + ssrf_protect_options = {} + if constance.config.SSRF_ALLOWED_IP_ADDRESS.strip(): + ssrf_protect_options[ + 'allowed_ip_addresses' + ] = constance.config.SSRF_ALLOWED_IP_ADDRESS.strip().split( + '\r\n' + ) + + if constance.config.SSRF_DENIED_IP_ADDRESS.strip(): + ssrf_protect_options[ + 'denied_ip_addresses' + ] = constance.config.SSRF_DENIED_IP_ADDRESS.strip().split( + '\r\n' + ) + + SSRFProtect.validate( + self._hook.endpoint, options=ssrf_protect_options + ) + + response = requests.post( + self._hook.endpoint, timeout=30, **request_kwargs + ) + response.raise_for_status() + self.save_log(response.status_code, response.text, success=True) + + return True + + except requests.exceptions.RequestException as e: + # If request fails to communicate with remote server. + # Exception is raised before request.post can return something. + # Thus, response equals None + status_code = KOBO_INTERNAL_ERROR_STATUS_CODE + text = str(e) + if response is not None: + text = response.text + status_code = response.status_code + + if status_code in RETRIABLE_STATUS_CODES: + self.save_log(status_code, text, allow_retries=True) + raise HookRemoteServerDownError + + self.save_log(status_code, text) + raise + except SSRFProtectException as e: + logging.error( + 'service_json.ServiceDefinition.send: ' + f'Hook #{self._hook.uid} - ' + f'Data #{self._submission_id} - ' + f'{str(e)}', + exc_info=True, + ) + self.save_log( + KOBO_INTERNAL_ERROR_STATUS_CODE, + f'{self._hook.endpoint} is not allowed' + ) + raise + except Exception as e: + logging.error( + 'service_json.ServiceDefinition.send: ' + f'Hook #{self._hook.uid} - ' + f'Data #{self._submission_id} - ' + f'{str(e)}', + exc_info=True, + ) + self.save_log( + KOBO_INTERNAL_ERROR_STATUS_CODE, + 'An error occurred when sending ' + f'data to external endpoint: {str(e)}', + ) + raise + + def save_log( + self, + status_code: int, + message: str, + success: bool = False, + allow_retries: bool = False, + ): """ Updates/creates log entry with: - `status_code` as the HTTP status code of the remote server response - `message` as the content of the remote server response """ - fields = { - 'hook': self._hook, - 'submission_id': self._submission_id - } + fields = {'hook': self._hook, 'submission_id': self._submission_id} try: # Try to load the log with a multiple field FK because # we don't know the log `uid` in this context, but we do know @@ -181,7 +219,7 @@ def save_log(self, status_code: int, message: str, success: bool = False): if success: log.status = HOOK_LOG_SUCCESS - elif log.tries >= constance.config.HOOK_MAX_RETRIES: + elif not allow_retries or log.tries >= constance.config.HOOK_MAX_RETRIES: log.status = HOOK_LOG_FAILED log.status_code = status_code diff --git a/kobo/apps/hook/tasks.py b/kobo/apps/hook/tasks.py index b1dfbf7a96..c87cd21076 100644 --- a/kobo/apps/hook/tasks.py +++ b/kobo/apps/hook/tasks.py @@ -9,37 +9,33 @@ from django.utils import translation, timezone from django_celery_beat.models import PeriodicTask +from kobo.celery import celery_app from kpi.utils.log import logging from .constants import HOOK_LOG_FAILED +from .exceptions import HookRemoteServerDownError from .models import Hook, HookLog - - -@shared_task(bind=True) -def service_definition_task(self, hook_id, submission_id): +from .utils.lazy import LazyMaxRetriesInt + + +@celery_app.task( + autoretry_for=(HookRemoteServerDownError,), + retry_backoff=60, + retry_backoff_max=1200, + max_retries=LazyMaxRetriesInt(), + retry_jitter=True, + queue='kpi_low_priority_queue', +) +def service_definition_task(hook_id: int, submission_id: int) -> bool: """ Tries to send data to the endpoint of the hook It retries n times (n = `constance.config.HOOK_MAX_RETRIES`) - - - after 1 minutes, - - after 10 minutes, - - after 100 minutes - etc ... - - :param self: Celery.Task. - :param hook_id: int. Hook PK - :param submission_id: int. Instance PK """ hook = Hook.objects.get(id=hook_id) # Use camelcase (even if it's not PEP-8 compliant) # because variable represents the class, not the instance. ServiceDefinition = hook.get_service_definition() # noqa service_definition = ServiceDefinition(hook, submission_id) - if not service_definition.send(): - # Countdown is in seconds - countdown = HookLog.get_remaining_seconds(self.request.retries) - raise self.retry(countdown=countdown, max_retries=constance.config.HOOK_MAX_RETRIES) - - return True + return service_definition.send() @shared_task diff --git a/kobo/apps/hook/tests/test_utils.py b/kobo/apps/hook/tests/test_utils.py index ad198a7b7c..94167f1886 100644 --- a/kobo/apps/hook/tests/test_utils.py +++ b/kobo/apps/hook/tests/test_utils.py @@ -3,7 +3,7 @@ from rest_framework import status from .hook_test_case import HookTestCase, MockSSRFProtect -from ..utils import HookUtils +from ..utils.services import call_services class HookUtilsTestCase(HookTestCase): @@ -29,7 +29,7 @@ def test_data_submission(self): submissions = self.asset.deployment.get_submissions(self.asset.owner) submission_id = submissions[0]['_id'] - assert HookUtils.call_services(self.asset.uid, submission_id) is True + assert call_services(self.asset.uid, submission_id) is True # Create second hook second_hook = self._create_hook( @@ -45,8 +45,8 @@ def test_data_submission(self): ) # Since second hook hasn't received the submission, `call_services` # should still return True - assert HookUtils.call_services(self.asset.uid, submission_id) is True + assert call_services(self.asset.uid, submission_id) is True # But if we try again, it should return False (we cannot send the same # submission twice to the same external endpoint). - assert HookUtils.call_services(self.asset.uid, submission_id) is False + assert call_services(self.asset.uid, submission_id) is False diff --git a/kobo/apps/hook/utils.py b/kobo/apps/hook/utils.py deleted file mode 100644 index 5bb6e1391c..0000000000 --- a/kobo/apps/hook/utils.py +++ /dev/null @@ -1,33 +0,0 @@ -# coding: utf-8 -from .models.hook import Hook -from .models.hook_log import HookLog -from .tasks import service_definition_task - - -class HookUtils: - - @staticmethod - def call_services(asset_uid: str, submission_id: int) -> bool: - """ - Delegates to Celery data submission to remote servers - """ - # Retrieve `Hook` ids, to send data to their respective endpoint. - hooks_ids = ( - Hook.objects.filter(asset__uid=asset_uid, active=True) - .values_list('id', flat=True) - .distinct() - ) - # At least, one of the hooks must not have a log that corresponds to - # `submission_id` - # to make success equal True - success = False - for hook_id in hooks_ids: - if not HookLog.objects.filter( - submission_id=submission_id, hook_id=hook_id - ).exists(): - success = True - service_definition_task.apply_async( - queue='kpi_low_priority_queue', args=(hook_id, submission_id) - ) - - return success diff --git a/kobo/apps/hook/utils/__init__.py b/kobo/apps/hook/utils/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/kobo/apps/hook/utils/lazy.py b/kobo/apps/hook/utils/lazy.py new file mode 100644 index 0000000000..58a64c9d1a --- /dev/null +++ b/kobo/apps/hook/utils/lazy.py @@ -0,0 +1,44 @@ +import constance + + +class LazyMaxRetriesInt: + """ + constance settings cannot be used as default parameters of a function. + This wrapper helps to return the value of `constance.config.HOOK_MAX_RETRIES` + on demand. + """ + def __call__(self, *args, **kwargs): + return constance.config.HOOK_MAX_RETRIES + + def __repr__(self): + return str(constance.config.HOOK_MAX_RETRIES) + + def __eq__(self, other): + if isinstance(other, int): + return self() == other + return NotImplemented + + def __ne__(self, other): + if isinstance(other, int): + return self() != other + return NotImplemented + + def __lt__(self, other): + if isinstance(other, int): + return self() < other + return NotImplemented + + def __le__(self, other): + if isinstance(other, int): + return self() <= other + return NotImplemented + + def __gt__(self, other): + if isinstance(other, int): + return self() > other + return NotImplemented + + def __ge__(self, other): + if isinstance(other, int): + return self() >= other + return NotImplemented diff --git a/kobo/apps/hook/utils/services.py b/kobo/apps/hook/utils/services.py new file mode 100644 index 0000000000..f0d05c7ad1 --- /dev/null +++ b/kobo/apps/hook/utils/services.py @@ -0,0 +1,27 @@ +from ..models.hook import Hook +from ..models.hook_log import HookLog +from ..tasks import service_definition_task + + +def call_services(asset_uid: str, submission_id: int) -> bool: + """ + Delegates to Celery data submission to remote servers + """ + # Retrieve `Hook` ids, to send data to their respective endpoint. + hooks_ids = ( + Hook.objects.filter(asset__uid=asset_uid, active=True) + .values_list('id', flat=True) + .distinct() + ) + # At least, one of the hooks must not have a log that corresponds to + # `submission_id` + # to make success equal True + success = False + + for hook_id in hooks_ids: + if not HookLog.objects.filter( + submission_id=submission_id, hook_id=hook_id + ).exists(): + success = True + service_definition_task.delay(hook_id, submission_id) + return success diff --git a/kobo/apps/hook/views/v2/hook.py b/kobo/apps/hook/views/v2/hook.py index 9c4f795b0d..1e2a975868 100644 --- a/kobo/apps/hook/views/v2/hook.py +++ b/kobo/apps/hook/views/v2/hook.py @@ -174,13 +174,20 @@ def retry(self, request, uid=None, *args, **kwargs): response = {"detail": t("Task successfully scheduled")} status_code = status.HTTP_200_OK if hook.active: - seconds = HookLog.get_elapsed_seconds(constance.config.HOOK_MAX_RETRIES) - threshold = timezone.now() - timedelta(seconds=seconds) - - records = hook.logs.filter(Q(date_modified__lte=threshold, - status=HOOK_LOG_PENDING) | - Q(status=HOOK_LOG_FAILED)). \ - values_list("id", "uid").distinct() + threshold = timezone.now() - timedelta(seconds=120) + + records = ( + hook.logs.filter( + Q( + date_modified__lte=threshold, + status=HOOK_LOG_PENDING, + tries__gte=constance.config.HOOK_MAX_RETRIES, + ) + | Q(status=HOOK_LOG_FAILED) + ) + .values_list('id', 'uid') + .distinct() + ) # Prepare lists of ids hooklogs_ids = [] hooklogs_uids = [] @@ -190,7 +197,9 @@ def retry(self, request, uid=None, *args, **kwargs): if len(records) > 0: # Mark all logs as PENDING - HookLog.objects.filter(id__in=hooklogs_ids).update(status=HOOK_LOG_PENDING) + HookLog.objects.filter(id__in=hooklogs_ids).update( + status=HOOK_LOG_PENDING + ) # Delegate to Celery retry_all_task.apply_async( queue='kpi_low_priority_queue', args=(hooklogs_ids,) diff --git a/kobo/apps/hook/views/v2/hook_log.py b/kobo/apps/hook/views/v2/hook_log.py index ab6e1e29c9..049047655c 100644 --- a/kobo/apps/hook/views/v2/hook_log.py +++ b/kobo/apps/hook/views/v2/hook_log.py @@ -108,7 +108,7 @@ def retry(self, request, uid=None, *args, **kwargs): status_code = status.HTTP_200_OK hook_log = self.get_object() - if hook_log.can_retry(): + if hook_log.can_retry: hook_log.change_status() success = hook_log.retry() if success: diff --git a/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py b/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py index 2427ab651e..4afd88a3f3 100644 --- a/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py +++ b/kobo/apps/openrosa/apps/viewer/models/parsed_instance.py @@ -26,7 +26,7 @@ ) from kobo.apps.openrosa.libs.utils.decorators import apply_form_field_names from kobo.apps.openrosa.libs.utils.model_tools import queryset_iterator -from kobo.apps.hook.utils import HookUtils +from kobo.apps.hook.utils.services import call_services from kpi.utils.log import logging # this is Mongo Collection where we will store the parsed submissions @@ -380,7 +380,7 @@ def save(self, asynchronous=False, *args, **kwargs): f'ParsedInstance #: {self.pk} - XForm is not linked with Asset' ) else: - HookUtils.call_services(asset_uid, self.instance_id) + call_services(asset_uid, self.instance_id) return success diff --git a/kobo/settings/base.py b/kobo/settings/base.py index b0ba92ff27..715d2badc3 100644 --- a/kobo/settings/base.py +++ b/kobo/settings/base.py @@ -1198,7 +1198,7 @@ def dj_stripe_request_callback_method(): # http://docs.celeryproject.org/en/latest/getting-started/brokers/redis.html#redis-visibility-timeout # TODO figure out how to pass `Constance.HOOK_MAX_RETRIES` or `HookLog.get_remaining_seconds() # Otherwise hardcode `HOOK_MAX_RETRIES` in Settings - "visibility_timeout": 60 * (10 ** 3) # Longest ETA for RestService (seconds) + "visibility_timeout": 60 * (10 ** 2) # Longest ETA for RestService (seconds) } CELERY_TASK_DEFAULT_QUEUE = "kpi_queue" From c8996f4beb31cd6e68dffb61766bad7d006c66a7 Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Thu, 1 Aug 2024 17:55:51 -0400 Subject: [PATCH 4/6] Fix tests --- kobo/apps/hook/models/hook_log.py | 18 ++--- kobo/apps/hook/tests/hook_test_case.py | 48 ++++++++----- kobo/apps/hook/tests/test_api_hook.py | 99 +++++++++++++++++++------- kobo/apps/hook/tests/test_ssrf.py | 12 ++-- kobo/apps/hook/views/v2/hook_log.py | 15 ++-- 5 files changed, 129 insertions(+), 63 deletions(-) diff --git a/kobo/apps/hook/models/hook_log.py b/kobo/apps/hook/models/hook_log.py index 2cba30815f..00ecd429e0 100644 --- a/kobo/apps/hook/models/hook_log.py +++ b/kobo/apps/hook/models/hook_log.py @@ -45,15 +45,15 @@ def can_retry(self) -> bool: Notice: even if False is returned, `self.retry()` can be triggered. """ if self.hook.active: - if self.tries >= constance.config.HOOK_MAX_RETRIES: - # If log is still pending after `constance.config.HOOK_MAX_RETRIES` - # times, there was an issue, we allow the retry. - threshold = timezone.now() - timedelta(seconds=120) - - return self.status == HOOK_LOG_FAILED or ( - self.date_modified < threshold - and self.status == HOOK_LOG_PENDING - ) + # If log is still pending after `constance.config.HOOK_MAX_RETRIES` + # times, there was an issue, we allow the retry. + threshold = timezone.now() - timedelta(seconds=120) + + return self.status == HOOK_LOG_FAILED or ( + self.date_modified < threshold + and self.status == HOOK_LOG_PENDING + and self.tries >= constance.config.HOOK_MAX_RETRIES + ) return False diff --git a/kobo/apps/hook/tests/hook_test_case.py b/kobo/apps/hook/tests/hook_test_case.py index b4ea20658e..c5c31d420a 100644 --- a/kobo/apps/hook/tests/hook_test_case.py +++ b/kobo/apps/hook/tests/hook_test_case.py @@ -1,6 +1,7 @@ # coding: utf-8 import json +import pytest import responses from django.conf import settings from django.urls import reverse @@ -11,6 +12,7 @@ from kpi.exceptions import BadFormatException from kpi.tests.kpi_test_case import KpiTestCase from ..constants import HOOK_LOG_FAILED +from ..exceptions import HookRemoteServerDownError from ..models import HookLog, Hook @@ -94,26 +96,45 @@ def _send_and_fail(self): :return: dict """ + first_hooklog_response = self._send_and_wait_for_retry() + + # Fakes Celery n retries by forcing status to `failed` + # (where n is `settings.HOOKLOG_MAX_RETRIES`) + first_hooklog = HookLog.objects.get( + uid=first_hooklog_response.get('uid') + ) + first_hooklog.change_status(HOOK_LOG_FAILED) + + return first_hooklog_response + + def _send_and_wait_for_retry(self): self.hook = self._create_hook() ServiceDefinition = self.hook.get_service_definition() submissions = self.asset.deployment.get_submissions(self.asset.owner) submission_id = submissions[0]['_id'] service_definition = ServiceDefinition(self.hook, submission_id) - first_mock_response = {'error': 'not found'} + first_mock_response = {'error': 'gateway timeout'} # Mock first request's try - responses.add(responses.POST, self.hook.endpoint, - json=first_mock_response, status=status.HTTP_404_NOT_FOUND) + responses.add( + responses.POST, + self.hook.endpoint, + json=first_mock_response, + status=status.HTTP_504_GATEWAY_TIMEOUT, + ) # Mock next requests' tries - responses.add(responses.POST, self.hook.endpoint, - status=status.HTTP_200_OK, - content_type='application/json') + responses.add( + responses.POST, + self.hook.endpoint, + status=status.HTTP_200_OK, + content_type='application/json', + ) # Try to send data to external endpoint - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(HookRemoteServerDownError): + service_definition.send() # Retrieve the corresponding log url = reverse('hook-log-list', kwargs={ @@ -126,20 +147,13 @@ def _send_and_fail(self): # Result should match first try self.assertEqual( - first_hooklog_response.get('status_code'), status.HTTP_404_NOT_FOUND + first_hooklog_response.get('status_code'), + status.HTTP_504_GATEWAY_TIMEOUT, ) self.assertEqual( json.loads(first_hooklog_response.get('message')), first_mock_response, ) - - # Fakes Celery n retries by forcing status to `failed` - # (where n is `settings.HOOKLOG_MAX_RETRIES`) - first_hooklog = HookLog.objects.get( - uid=first_hooklog_response.get('uid') - ) - first_hooklog.change_status(HOOK_LOG_FAILED) - return first_hooklog_response def __prepare_submission(self): diff --git a/kobo/apps/hook/tests/test_api_hook.py b/kobo/apps/hook/tests/test_api_hook.py index b8fbe5f7f8..b5a62c1623 100644 --- a/kobo/apps/hook/tests/test_api_hook.py +++ b/kobo/apps/hook/tests/test_api_hook.py @@ -1,6 +1,7 @@ # coding: utf-8 import json +import pytest import responses from constance.test import override_config from django.urls import reverse @@ -22,6 +23,7 @@ ) from kpi.utils.datetime import several_minutes_from_now from .hook_test_case import HookTestCase, MockSSRFProtect +from ..exceptions import HookRemoteServerDownError class ApiHookTestCase(HookTestCase): @@ -169,18 +171,20 @@ def test_partial_update_hook(self): self.assertFalse(hook.active) self.assertEqual(hook.name, "some disabled external service") - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) @responses.activate def test_send_and_retry(self): first_log_response = self._send_and_fail() # Let's retry through API call - retry_url = reverse("hook-log-retry", kwargs={ - "parent_lookup_asset": self.asset.uid, - "parent_lookup_hook": self.hook.uid, - "uid": first_log_response.get("uid") + retry_url = reverse('hook-log-retry', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') }) # It should be a success @@ -188,17 +192,49 @@ def test_send_and_retry(self): self.assertEqual(response.status_code, status.HTTP_200_OK) # Let's check if logs has 2 tries - detail_url = reverse("hook-log-detail", kwargs={ - "parent_lookup_asset": self.asset.uid, - "parent_lookup_hook": self.hook.uid, - "uid": first_log_response.get("uid") + detail_url = reverse('hook-log-detail', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') }) response = self.client.get(detail_url, format=SUBMISSION_FORMAT_TYPE_JSON) - self.assertEqual(response.data.get("tries"), 2) + self.assertEqual(response.data.get('tries'), 2) - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) + @responses.activate + def test_send_and_cannot_retry(self): + + first_log_response = self._send_and_wait_for_retry() + + # Let's retry through API call + retry_url = reverse('hook-log-retry', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') + }) + + # It should be a failure. The hook log is going to be retried + response = self.client.patch(retry_url, format=SUBMISSION_FORMAT_TYPE_JSON) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # Let's check if logs has 2 tries + detail_url = reverse('hook-log-detail', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') + }) + + response = self.client.get(detail_url, format=SUBMISSION_FORMAT_TYPE_JSON) + self.assertEqual(response.data.get('tries'), 1) + + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) @responses.activate def test_payload_template(self): @@ -321,12 +357,17 @@ def test_hook_log_filter_success(self): @responses.activate def test_hook_log_filter_failure(self): # Create failing hook - hook = self._create_hook(name="failing hook", - endpoint="http://failing.service.local/", - settings={}) - responses.add(responses.POST, hook.endpoint, - status=status.HTTP_500_INTERNAL_SERVER_ERROR, - content_type="application/json") + hook = self._create_hook( + name='failing hook', + endpoint='http://failing.service.local/', + settings={}, + ) + responses.add( + responses.POST, + hook.endpoint, + status=status.HTTP_504_GATEWAY_TIMEOUT, + content_type="application/json", + ) # simulate a submission ServiceDefinition = hook.get_service_definition() @@ -334,8 +375,8 @@ def test_hook_log_filter_failure(self): submission_id = submissions[0]['_id'] service_definition = ServiceDefinition(hook, submission_id) - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(HookRemoteServerDownError): + service_definition.send() # Get log for the failing hook hook_log_url = reverse('hook-log-list', kwargs={ @@ -344,18 +385,24 @@ def test_hook_log_filter_failure(self): }) # There should be no success log for the failing hook - response = self.client.get(f'{hook_log_url}?status={HOOK_LOG_SUCCESS}', format='json') + response = self.client.get( + f'{hook_log_url}?status={HOOK_LOG_SUCCESS}', format='json' + ) self.assertEqual(response.data.get('count'), 0) # There should be a pending log for the failing hook - response = self.client.get(f'{hook_log_url}?status={HOOK_LOG_PENDING}', format='json') + response = self.client.get( + f'{hook_log_url}?status={HOOK_LOG_PENDING}', format='json' + ) self.assertEqual(response.data.get('count'), 1) def test_hook_log_filter_validation(self): # Create hook - hook = self._create_hook(name="success hook", - endpoint="http://hook.service.local/", - settings={}) + hook = self._create_hook( + name='success hook', + endpoint='http://hook.service.local/', + settings={}, + ) # Get log for the success hook hook_log_url = reverse('hook-log-list', kwargs={ diff --git a/kobo/apps/hook/tests/test_ssrf.py b/kobo/apps/hook/tests/test_ssrf.py index 89a71f4d4c..3d28e4a766 100644 --- a/kobo/apps/hook/tests/test_ssrf.py +++ b/kobo/apps/hook/tests/test_ssrf.py @@ -1,12 +1,13 @@ -# coding: utf-8 +import pytest import responses from constance.test import override_config from mock import patch from rest_framework import status +from ssrf_protect.exceptions import SSRFProtectException from kobo.apps.hook.constants import ( - HOOK_LOG_PENDING, + HOOK_LOG_FAILED, KOBO_INTERNAL_ERROR_STATUS_CODE ) from .hook_test_case import HookTestCase, MockSSRFProtect @@ -34,9 +35,10 @@ def test_send_with_ssrf_options(self): content_type='application/json') # Try to send data to external endpoint - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(SSRFProtectException): + service_definition.send() + hook_log = hook.logs.all()[0] self.assertEqual(hook_log.status_code, KOBO_INTERNAL_ERROR_STATUS_CODE) - self.assertEqual(hook_log.status, HOOK_LOG_PENDING) + self.assertEqual(hook_log.status, HOOK_LOG_FAILED) self.assertTrue('is not allowed' in hook_log.message) diff --git a/kobo/apps/hook/views/v2/hook_log.py b/kobo/apps/hook/views/v2/hook_log.py index 049047655c..6b5a8874c1 100644 --- a/kobo/apps/hook/views/v2/hook_log.py +++ b/kobo/apps/hook/views/v2/hook_log.py @@ -114,15 +114,18 @@ def retry(self, request, uid=None, *args, **kwargs): if success: # Return status_code of remote server too. # `response["status_code"]` is not the same as `status_code` - response["detail"] = hook_log.message - response["status_code"] = hook_log.status_code + response['detail'] = hook_log.message + response['status_code'] = hook_log.status_code else: - response["detail"] = t( - "An error has occurred when sending the data. Please try again later.") + response['detail'] = t( + 'An error has occurred when sending the data. ' + 'Please try again later.' + ) status_code = status.HTTP_500_INTERNAL_SERVER_ERROR else: - response["detail"] = t( - "Data is being or has already been processed") + response['detail'] = t( + 'Data is being or has already been processed' + ) status_code = status.HTTP_400_BAD_REQUEST return Response(response, status=status_code) From 3e1d9d13305493cf20e49aa3e5412a4c68365064 Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Tue, 6 Aug 2024 09:01:04 -0400 Subject: [PATCH 5/6] Fix bug on redeploy --- kpi/deployment_backends/openrosa_backend.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/kpi/deployment_backends/openrosa_backend.py b/kpi/deployment_backends/openrosa_backend.py index 97e8d8d6e7..bc95174140 100644 --- a/kpi/deployment_backends/openrosa_backend.py +++ b/kpi/deployment_backends/openrosa_backend.py @@ -835,11 +835,9 @@ def redeploy(self, active=None): XForm.objects.filter(pk=self.xform.id).update( downloadable=active, title=self.asset.name, - has_kpi_hooks=self.asset.has_active_hooks, ) self.xform.downloadable = active self.xform.title = self.asset.name - self.xform.has_kpi_hooks = self.asset.has_active_hooks publish_xls_form(xlsx_file, self.asset.owner, self.xform.id_string) From 7697d683893f60053d1479530b843e47aeb5ec5d Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Fri, 23 Aug 2024 06:56:18 -0400 Subject: [PATCH 6/6] Use MagicMock instead of MockSSRFProtect --- kobo/apps/hook/tests/hook_test_case.py | 7 ----- kobo/apps/hook/tests/test_api_hook.py | 36 ++++++++++++++++---------- kobo/apps/hook/tests/test_email.py | 11 +++++--- kobo/apps/hook/tests/test_ssrf.py | 14 ++++++---- kobo/apps/hook/tests/test_utils.py | 7 ++--- 5 files changed, 42 insertions(+), 33 deletions(-) diff --git a/kobo/apps/hook/tests/hook_test_case.py b/kobo/apps/hook/tests/hook_test_case.py index c5c31d420a..bbf8799806 100644 --- a/kobo/apps/hook/tests/hook_test_case.py +++ b/kobo/apps/hook/tests/hook_test_case.py @@ -16,13 +16,6 @@ from ..models import HookLog, Hook -class MockSSRFProtect: - - @staticmethod - def _get_ip_address(url): - return ip_address('1.2.3.4') - - class HookTestCase(KpiTestCase): def setUp(self): diff --git a/kobo/apps/hook/tests/test_api_hook.py b/kobo/apps/hook/tests/test_api_hook.py index b5a62c1623..c6368f568d 100644 --- a/kobo/apps/hook/tests/test_api_hook.py +++ b/kobo/apps/hook/tests/test_api_hook.py @@ -5,9 +5,11 @@ import responses from constance.test import override_config from django.urls import reverse -from mock import patch +from ipaddress import ip_address +from mock import patch, MagicMock from rest_framework import status + from kobo.apps.hook.constants import ( HOOK_LOG_FAILED, HOOK_LOG_PENDING, @@ -22,7 +24,7 @@ PERM_CHANGE_ASSET ) from kpi.utils.datetime import several_minutes_from_now -from .hook_test_case import HookTestCase, MockSSRFProtect +from .hook_test_case import HookTestCase from ..exceptions import HookRemoteServerDownError @@ -173,7 +175,7 @@ def test_partial_update_hook(self): @patch( 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address + new=MagicMock(return_value=ip_address('1.2.3.4')) ) @responses.activate def test_send_and_retry(self): @@ -203,7 +205,7 @@ def test_send_and_retry(self): @patch( 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address + new=MagicMock(return_value=ip_address('1.2.3.4')) ) @responses.activate def test_send_and_cannot_retry(self): @@ -233,7 +235,7 @@ def test_send_and_cannot_retry(self): @patch( 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address + new=MagicMock(return_value=ip_address('1.2.3.4')) ) @responses.activate def test_payload_template(self): @@ -317,8 +319,10 @@ def test_payload_template_validation(self): } self.assertEqual(response.data, expected_response) - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MagicMock(return_value=ip_address('1.2.3.4')) + ) @responses.activate def test_hook_log_filter_success(self): # Create success hook @@ -352,8 +356,10 @@ def test_hook_log_filter_success(self): response = self.client.get(f'{hook_log_url}?status={HOOK_LOG_FAILED}', format='json') self.assertEqual(response.data.get('count'), 0) - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MagicMock(return_value=ip_address('1.2.3.4')) + ) @responses.activate def test_hook_log_filter_failure(self): # Create failing hook @@ -414,14 +420,16 @@ def test_hook_log_filter_validation(self): response = self.client.get(f'{hook_log_url}?status=abc', format='json') self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MagicMock(return_value=ip_address('1.2.3.4')) + ) @responses.activate def test_hook_log_filter_date(self): # Create success hook - hook = self._create_hook(name="date hook", - endpoint="http://date.service.local/", - settings={}) + hook = self._create_hook( + name="date hook", endpoint="http://date.service.local/", settings={} + ) responses.add(responses.POST, hook.endpoint, status=status.HTTP_200_OK, content_type="application/json") diff --git a/kobo/apps/hook/tests/test_email.py b/kobo/apps/hook/tests/test_email.py index 2b1f7d0fdf..2a587340cb 100644 --- a/kobo/apps/hook/tests/test_email.py +++ b/kobo/apps/hook/tests/test_email.py @@ -5,9 +5,10 @@ from django.template.loader import get_template from django.utils import translation, dateparse from django_celery_beat.models import PeriodicTask, CrontabSchedule -from mock import patch +from ipaddress import ip_address +from mock import patch, MagicMock -from .hook_test_case import HookTestCase, MockSSRFProtect +from .hook_test_case import HookTestCase from ..tasks import failures_reports @@ -28,8 +29,10 @@ def _create_periodic_task(self): return periodic_task - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MagicMock(return_value=ip_address('1.2.3.4')) + ) @responses.activate def test_notifications(self): self._create_periodic_task() diff --git a/kobo/apps/hook/tests/test_ssrf.py b/kobo/apps/hook/tests/test_ssrf.py index 3d28e4a766..df3ebd193f 100644 --- a/kobo/apps/hook/tests/test_ssrf.py +++ b/kobo/apps/hook/tests/test_ssrf.py @@ -1,8 +1,8 @@ - import pytest import responses from constance.test import override_config -from mock import patch +from ipaddress import ip_address +from mock import patch, MagicMock from rest_framework import status from ssrf_protect.exceptions import SSRFProtectException @@ -10,13 +10,15 @@ HOOK_LOG_FAILED, KOBO_INTERNAL_ERROR_STATUS_CODE ) -from .hook_test_case import HookTestCase, MockSSRFProtect +from .hook_test_case import HookTestCase class SSRFHookTestCase(HookTestCase): - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MagicMock(return_value=ip_address('1.2.3.4')) + ) @override_config(SSRF_DENIED_IP_ADDRESS='1.2.3.4') @responses.activate def test_send_with_ssrf_options(self): @@ -35,6 +37,8 @@ def test_send_with_ssrf_options(self): content_type='application/json') # Try to send data to external endpoint + # Note: it should failed because we explicitly deny 1.2.3.4 and + # SSRFProtect._get_ip_address is mocked to return 1.2.3.4 with pytest.raises(SSRFProtectException): service_definition.send() diff --git a/kobo/apps/hook/tests/test_utils.py b/kobo/apps/hook/tests/test_utils.py index 94167f1886..931f66ce1e 100644 --- a/kobo/apps/hook/tests/test_utils.py +++ b/kobo/apps/hook/tests/test_utils.py @@ -1,8 +1,9 @@ import responses -from mock import patch +from ipaddress import ip_address +from mock import patch, MagicMock from rest_framework import status -from .hook_test_case import HookTestCase, MockSSRFProtect +from .hook_test_case import HookTestCase from ..utils.services import call_services @@ -10,7 +11,7 @@ class HookUtilsTestCase(HookTestCase): @patch( 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address + new=MagicMock(return_value=ip_address('1.2.3.4')) ) @responses.activate def test_data_submission(self):