Code refactoring: Better error handling in new deployment back-end class

kobo/apps/hook/tests/hook_test_case.py

@@ -6,7 +6,6 @@
 import responses
 from django.conf import settings
 from django.urls import reverse
-from ipaddress import ip_address
 from rest_framework import status
 
 from kpi.constants import SUBMISSION_FORMAT_TYPE_JSON, SUBMISSION_FORMAT_TYPE_XML

kobo/apps/openrosa/apps/api/tests/viewsets/test_xform_submission_api.py

@@ -327,7 +327,7 @@ def test_post_submission_require_auth_other_user(self):
                 auth = DigestAuth('alice', 'alicealice')
                 request.META.update(auth(request.META, response))
                 response = self.view(request, username=self.user.username)
-                self.assertContains(response, 'Forbidden', status_code=403)
+                self.assertContains(response, 'Access denied', status_code=403)
 
     def test_post_submission_require_auth_data_entry_role(self):
 

kobo/apps/openrosa/apps/api/viewsets/data_viewset.py

@@ -324,7 +324,6 @@ class DataViewSet(AnonymousUserPublicFormsMixin, OpenRosaModelViewSet):
     >           "timestamp": 1513299978,
     >           "by_whom ": "John Doe",
     >           "uid": "validation_status_approved",
-    >           "color": "#00ff00",
     >           "label: "Approved"
     >       }
 
@@ -351,7 +350,6 @@ class DataViewSet(AnonymousUserPublicFormsMixin, OpenRosaModelViewSet):
     >           "timestamp": 1513299978,
     >           "by_whom ": "John Doe",
     >           "uid": "validation_status_not_approved",
-    >           "color": "#ff0000",
     >           "label": "Not Approved"
     >       }
 

kobo/apps/openrosa/apps/main/migrations/0015_drop_old_restservice_tables.py

@@ -56,6 +56,10 @@ def get_operations():
     return operations
 
 
+def noop(*args, **kwargs):
+    pass
+
+
 def print_migration_warning(apps, schema_editor):
     if settings.TESTING or settings.SKIP_HEAVY_MIGRATIONS:
         return
@@ -74,4 +78,7 @@ class Migration(migrations.Migration):
         ('main', '0014_drop_old_formdisclaimer_tables'),
     ]
 
-    operations = [migrations.RunPython(print_migration_warning), *get_operations()]
+    operations = [
+        migrations.RunPython(print_migration_warning, noop),
+        *get_operations(),
+    ]

kobo/apps/openrosa/libs/utils/logger_tools.py

@@ -6,6 +6,7 @@
 import re
 import sys
 import traceback
+from contextlib import contextmanager
 from datetime import date, datetime, timezone
 from typing import Generator, Optional, Union
 from xml.etree import ElementTree as ET
@@ -286,6 +287,61 @@ def get_xform_from_submission(xml, username, uuid=None):
     )
 
 
+@contextmanager
+def http_open_rosa_error_handler(func, request):
+    class _ContextResult:
+        def __init__(self):
+            self.func_return = None
+            self.error = None
+            self.http_error_response = None
+
+        @property
+        def status_code(self):
+            if self.http_error_response:
+                return self.http_error_response.status_code
+            return 200
+
+    result = _ContextResult()
+    try:
+        result.func_return = func()
+    except InstanceInvalidUserError:
+        result.error = t('Username or ID required.')
+        result.http_error_response = OpenRosaResponseBadRequest(result.error)
+    except InstanceEmptyError:
+        result.error = t('Received empty submission. No instance was created')
+        result.http_error_response = OpenRosaResponseBadRequest(result.error)
+    except FormInactiveError:
+        result.error = t('Form is not active')
+        result.http_error_response = OpenRosaResponseNotAllowed(result.error)
+    except TemporarilyUnavailableError:
+        result.error = t('Temporarily unavailable')
+        result.http_error_response = OpenRosaTemporarilyUnavailable(result.error)
+    except XForm.DoesNotExist:
+        result.error = t('Form does not exist on this account')
+        result.http_error_response = OpenRosaResponseNotFound(result.error)
+    except ExpatError:
+        result.error = t('Improperly formatted XML.')
+        result.http_error_response = OpenRosaResponseBadRequest(result.error)
+    except DuplicateInstance:
+        result.error = t('Duplicate submission')
+        response = OpenRosaResponse(result.error)
+        response.status_code = 202
+        response['Location'] = request.build_absolute_uri(request.path)
+        result.http_error_response = response
+    except PermissionDenied:
+        result.error = t('Access denied')
+        result.http_error_response = OpenRosaResponseForbidden(result.error)
+    except InstanceMultipleNodeError as e:
+        result.error = str(e)
+        result.http_error_response = OpenRosaResponseBadRequest(e)
+    except DjangoUnicodeDecodeError:
+        result.error = t(
+            'File likely corrupted during ' 'transmission, please try later.'
+        )
+        result.http_error_response = OpenRosaResponseBadRequest(result.error)
+    yield result
+
+
 def inject_instanceid(xml_str, uuid):
     if get_uuid_from_xml(xml_str) is None:
         xml = clean_and_parse_xml(xml_str)
@@ -550,48 +606,18 @@ def safe_create_instance(
     :returns: A list [error, instance] where error is None if there was no
         error.
     """
-    error = instance = None
-
-    try:
-        instance = create_instance(
+    with http_open_rosa_error_handler(
+        lambda: create_instance(
             username,
             xml_file,
             media_files,
             uuid=uuid,
             date_created_override=date_created_override,
             request=request,
-        )
-    except InstanceInvalidUserError:
-        error = OpenRosaResponseBadRequest(t("Username or ID required."))
-    except InstanceEmptyError:
-        error = OpenRosaResponseBadRequest(
-            t("Received empty submission. No instance was created")
-        )
-    except FormInactiveError:
-        error = OpenRosaResponseNotAllowed(t("Form is not active"))
-    except TemporarilyUnavailableError:
-        error = OpenRosaTemporarilyUnavailable(t("Temporarily unavailable"))
-    except XForm.DoesNotExist:
-        error = OpenRosaResponseNotFound(
-            t("Form does not exist on this account")
-        )
-    except ExpatError as e:
-        error = OpenRosaResponseBadRequest(t("Improperly formatted XML."))
-    except DuplicateInstance:
-        response = OpenRosaResponse(t("Duplicate submission"))
-        response.status_code = 202
-        response['Location'] = request.build_absolute_uri(request.path)
-        error = response
-    except PermissionDenied as e:
-        error = OpenRosaResponseForbidden(e)
-    except InstanceMultipleNodeError as e:
-        error = OpenRosaResponseBadRequest(e)
-    except DjangoUnicodeDecodeError:
-        error = OpenRosaResponseBadRequest(t("File likely corrupted during "
-                                             "transmission, please try later."
-                                             ))
-
-    return [error, instance]
+        ),
+        request,
+    ) as handler:
+        return [handler.http_error_response, handler.func_return]
 
 
 def save_attachments(

kobo/apps/trash_bin/utils.py

@@ -15,10 +15,12 @@
     PeriodicTask,
     PeriodicTasks,
 )
-from rest_framework import status
 
 from kobo.apps.audit_log.models import AuditLog, AuditAction
-from kpi.exceptions import KobocatCommunicationError
+from kpi.exceptions import (
+    InvalidXFormException,
+    MissingXFormException,
+)
 from kpi.models import Asset, ExportTask, ImportTask
 from kpi.utils.mongo_helper import MongoHelper
 from kpi.utils.storage import rmdir
@@ -28,7 +30,6 @@
     TrashNotImplementedError,
     TrashMongoDeleteOrphansError,
     TrashTaskInProgressError,
-    TrashUnknownKobocatError,
 )
 from .models import TrashStatus
 from .models.account import AccountTrash
@@ -323,24 +324,16 @@ def _delete_submissions(request_author: settings.AUTH_USER_MODEL, asset: 'kpi.As
             ))
             submission_ids.append(submission['_id'])
 
-        json_response = asset.deployment.delete_submissions(
-            {'submission_ids': submission_ids, 'query': ''}, request_author
-        )
-
-        if json_response['status'] in [
-            status.HTTP_502_BAD_GATEWAY,
-            status.HTTP_504_GATEWAY_TIMEOUT,
-        ]:
-            raise KobocatCommunicationError
-
-        if json_response['status'] not in [
-            status.HTTP_404_NOT_FOUND,
-            status.HTTP_200_OK,
-        ]:
-            raise TrashUnknownKobocatError(response=json_response)
+        try:
+            deleted = asset.deployment.delete_submissions(
+                {'submission_ids': submission_ids, 'query': ''}, request_author
+            )
+        except (MissingXFormException, InvalidXFormException):
+            # XForm is invalid or gone
+            deleted = 0
 
         if audit_logs:
-            if json_response['status'] == status.HTTP_404_NOT_FOUND:
+            if not deleted:
                 # Submissions are lingering in MongoDB but XForm has been
                 # already deleted
                 if not MongoHelper.delete(

kpi/deployment_backends/base_backend.py

@@ -22,6 +22,9 @@
 from rest_framework.pagination import _positive_int as positive_int
 from shortuuid import ShortUUID
 
+from kobo.apps.openrosa.libs.utils.logger_tools import (
+    http_open_rosa_error_handler,
+)
 from kpi.constants import (
     SUBMISSION_FORMAT_TYPE_XML,
     SUBMISSION_FORMAT_TYPE_JSON,
@@ -149,7 +152,7 @@ def bulk_update_submissions(
             )
         }
 
-        backend_responses = []
+        backend_results = []
         for submission in submissions:
             xml_parsed = fromstring_preserve_root_xmlns(submission)
 
@@ -177,19 +180,24 @@ def bulk_update_submissions(
             for path, value in update_data.items():
                 edit_submission_xml(xml_parsed, path, value)
 
-            backend_response = self.store_submission(
-                user,
-                xml_tostring(xml_parsed),
-                _uuid,
-                request=kwargs.get('request'),
-            )
-            backend_responses.append(
-                {
-                    'uuid': _uuid,
-                    'response': backend_response,
-                }
-            )
-        return self.prepare_bulk_update_response(backend_responses)
+            request = kwargs.get('request')
+            with http_open_rosa_error_handler(
+                lambda: self.store_submission(
+                    user,
+                    xml_tostring(xml_parsed),
+                    _uuid,
+                    request=request,
+                ),
+                request,
+            ) as handler:
+                backend_results.append(
+                    {
+                        'uuid': _uuid,
+                        'error': handler.error,
+                        'result': handler.func_return
+                    }
+                )
+        return self.prepare_bulk_update_response(backend_results)
 
     @abc.abstractmethod
     def calculated_submission_count(self, user: settings.AUTH_USER_MODEL, **kwargs):
@@ -384,6 +392,10 @@ def last_submission_time(self):
     def mongo_userform_id(self):
         return None
 
+    @abc.abstractmethod
+    def prepare_bulk_update_response(self, backend_results: list[dict]) -> dict:
+        pass
+
     @abc.abstractmethod
     def redeploy(self, active: bool = None):
         pass
@@ -673,11 +685,11 @@ def validate_access_with_partial_perms(
         allowed_submission_ids = []
 
         if not submission_ids:
-            # if no submission ids are provided, the back end must rebuild the
+            # If no submission ids are provided, the back end must rebuild the
             # query to retrieve the related submissions. Unfortunately, the
             # current back end (KoBoCAT) does not support row level permissions.
             # Thus, we need to fetch all the submissions the user is allowed to
-            # see in order to to compare the requested subset of submissions to
+            # see in order to compare the requested subset of submissions to
             # all
             all_submissions = self.get_submissions(
                 user=user,
@@ -692,8 +704,8 @@ def validate_access_with_partial_perms(
             if not allowed_submission_ids:
                 raise PermissionDenied
 
-            # if `query` is not provided, the action is performed on all
-            # submissions. There are no needs to go further.
+            # If `query` is not provided, the action is performed on all
+            # submissions. There is no need to go further.
             if not query:
                 return allowed_submission_ids
 
@@ -719,7 +731,7 @@ def validate_access_with_partial_perms(
              and set(requested_submission_ids).issubset(allowed_submission_ids))
             or sorted(requested_submission_ids) == sorted(submission_ids)
         ):
-            # Regardless of whether or not the request contained a query or a
+            # Regardless of whether the request contained a query or a
             # list of IDs, always return IDs here because the results of a
             # query may contain submissions that the requesting user is not
             # allowed to access. For example,

kpi/deployment_backends/mock_backend.py

@@ -12,7 +12,7 @@
 from kobo.apps.kobo_auth.shortcuts import User
 from kobo.apps.openrosa.libs.utils.logger_tools import (
     dict2xform,
-    safe_create_instance,
+    create_instance,
 )
 from kpi.constants import PERM_ADD_SUBMISSIONS, SUBMISSION_FORMAT_TYPE_JSON
 from kpi.tests.utils.dicts import convert_hierarchical_keys_to_nested_dict
@@ -122,7 +122,8 @@ class FakeRequest:
 
             xml_string = dict2xform(sub_copy, self.xform.id_string)
             xml_file = io.StringIO(xml_string)
-            error, instance = safe_create_instance(
+
+            instance = create_instance(
                 owner_username,
                 xml_file,
                 media_files,
@@ -131,8 +132,6 @@ class FakeRequest:
                 ),
                 request=request,
             )
-            if error:
-                raise Exception(error)
 
             # Inject (or update) real PKs in submission…
             submission['_id'] = instance.pk