WIP - Reject duplicate submissions #876

noliveleger · 2023-05-08T19:45:26Z

Description

TBC

Additional info

would supersede #859

jnm · 2023-05-30T13:38:20Z

.github/workflows/pytest.yml

@@ -52,7 +52,7 @@ jobs:
    - name: Install Python dependencies
      run: pip-sync dependencies/pip/dev_requirements.txt
    - name: Run pytest
-      run: pytest -vv -rf
+      run: pytest -vv -rf --disable-warnings


jnm · 2023-05-30T13:41:04Z

.gitlab-ci.yml

@@ -32,6 +32,7 @@ test:
    POSTGRES_PASSWORD: kobo
    POSTGRES_DB: kobocat_test
    SERVICE_ACCOUNT_BACKEND_URL: redis://redis_cache:6379/4
+    GIT_LAB: "True"


maybe something more descriptive like SKIP_TESTS_WITH_CONCURRENCY

jnm · 2023-05-30T13:41:11Z

.gitlab-ci.yml

@@ -40,7 +41,7 @@ test:
  script:
    - apt-get update && apt-get install -y ghostscript gdal-bin libproj-dev gettext openjdk-11-jre
    - pip install -r dependencies/pip/dev_requirements.txt
-    - pytest -vv -rf
+    - pytest -vv -rf --disable-warnings


jnm · 2023-05-30T13:41:57Z

conftest.py

+        fakeredis.FakeStrictRedis(),
+    ):
+        with patch(
+            'onadata.apps.django_digest_backends.cache.RedisCacheNonceStorage._get_cache',
+            fakeredis.FakeStrictRedis,


why is one instantiated and the other isn't?

jnm · 2023-05-30T13:43:43Z

onadata/apps/api/tests/viewsets/test_xform_submission_api.py

+                results[result] += 1
+
+        assert results[status.HTTP_201_CREATED] == 1
+        assert results[status.HTTP_409_CONFLICT] == DUPLICATE_SUBMISSIONS_COUNT - 1


does the OpenRosa spec allow returning a 409? and do Enketo and Collect handle a 409 properly? i can't find the code, but i remember wanting to return a 40x that wasn't 400 but being forced to use 400 only because without it Collect wouldn't display the error message i was sending. could've been Enketo, though, or i might be misremembering entirely

jnm · 2023-05-30T14:00:13Z

onadata/libs/utils/logger_tools.py

+        # The start-time requirement protected submissions with identical responses
+        # from being rejected as duplicates *before* KoBoCAT had the concept of
+        # submission UUIDs. Nowadays, OpenRosa requires clients to send a UUID (in
+        # `<instanceID>`) within every submission; if the incoming XML has a UUID
+        # and still exactly matches an existing submission, it's certainly a
+        # duplicate (https://docs.opendatakit.org/openrosa-metadata/#fields).


We should reject outright any submission without a UUID

jnm · 2023-06-14T05:33:02Z

onadata/libs/utils/logger_tools.py

+        if existing_instance:
+            existing_instance.check_active(force=False)
+            # ensure we have saved the extra attachments
+            new_attachments = save_attachments(existing_instance, media_files)
+            if not new_attachments:
+                raise DuplicateInstanceError()


~~Wouldn't ConflictingXMLHashInstanceError be raised already and block us from getting here?~~ Nevermind; I misunderstood, and that exception only has to do with concurrent processing of submissions with identical XML

jnm · 2023-06-14T05:36:26Z

onadata/libs/utils/logger_tools.py

+            if is_postgresql:
+                cur = connection.cursor()
+                cur.execute('SELECT pg_try_advisory_lock(%s::bigint);', (int_lock,))
+                acquired = cur.fetchone()[0]
+            else:
+                prefix = os.getenv('KOBOCAT_REDIS_LOCK_PREFIX', 'kc-lock')
+                key_ = f'{prefix}:{int_lock}'
+                redis_lock = settings.REDIS_LOCK_CLIENT.lock(key_, timeout=60)
+                acquired = redis_lock.acquire(blocking=False)
+            yield acquired


I don't think support for something other than PostgreSQL is needed. We already depend on Postgres for many things.

Moot point, then, but just to say it: I think all os.getenv() for configuration (and default values) should be in the settings files

jnm · 2023-06-14T05:39:20Z

onadata/libs/utils/logger_tools.py

-    except DuplicateInstance:
-        response = OpenRosaResponse(t("Duplicate submission"))
+    except ConflictingXMLHashInstanceError:
+        response = OpenRosaResponse(t('Conflict with already existing instance'))
+        response.status_code = 409
+        response['Location'] = request.build_absolute_uri(request.path)
+        error = response
+    except DuplicateInstanceError:
+        response = OpenRosaResponse(t('Duplicate instance'))


A conflicting XML hash with no additional attachments is the same thing as a duplicate instance. If the XML is the same but there are additional attachments, no exception should be raised; this is normal operation. I don't think a new exception class is needed. ~~I also don't think that DuplicateInstanceError can be reached anymore, but that's addressed by a different comment~~ [was based on a misunderstanding]

The thing that confused me about this is that the verbiage is wrong. It's not a really a conflict with an existing instance, it's unwanted concurrent processing of two (or more) instances with identical XML.

I think ideally we should wait (a short amount of time) to try to acquire the lock before returning an error. I don't know what happens in Enketo now, but it's easy to imagine that a client would asynchronously send the following requests concurrently:

Submission XML (hash abc123) + dog.jpg

Submission XML (hash abc123) + cat.jpg

Submission XML (hash abc123) + gecko.jpg

Ideally, all three requests would succeed. Let's assume request 1 arrives first and is still processing while requests 2 and 3 are received by the server: what I understand this PR would do is reject requests 2 and 3 immediately with an error code. I think we should wait (again, briefly) for request 1 to finish¹ before returning an immediate rejection.

If the waiting takes too long, then we do have to reject in order to avoid sapping the worker pool with useless spinning. The message we return should be effectively "try again later", not something about a conflict with an existing instance. The HTTP code has to be dependent on the OpenRosa specification and compatible with Enketo and ODK Collect. Hopefully, those requirements are one and the same, but we'll have to test.

Footnotes

Addendum: Whoops, we don't really need to spin until request 1 finishes, we just need to wait until the row has been created in logger_instance. Imagine one POST has 50 files: locking while all 50 are written to storage is not what we want. We should still avoid storing the same attachment multiple times for a submission, so within a single submission, we should have some kind of attachment uniqueness constraint (if we don't already). ↩

jnm · 2023-06-14T05:41:45Z

onadata/settings/testing.py

@@ -1,6 +1,7 @@
 # coding: utf-8
 import os

+from fakeredis import FakeConnection, FakeStrictRedis, FakeServer


Ah, was the support for locking without PostgreSQL exclusively for unit testing?

Unit testing, at least on GitLab, is already using PostgreSQL, so I don't think we need to support any non-Postgres locking mechanisms for this

noliveleger added 2 commits May 3, 2023 15:54

Update "clean_duplicated_submissions' management command

ba4cfbf

Avoid duplicate submissions to be saved

32e38a7

noliveleger requested a review from jnm May 8, 2023 19:46

noliveleger force-pushed the duplicate-submissions branch 3 times, most recently from 9b39237 to 53c6320 Compare May 9, 2023 18:45

Add unit test for concurrent duplicate data

019e7bf

noliveleger force-pushed the duplicate-submissions branch from 53c6320 to 019e7bf Compare May 9, 2023 18:47

Merge branch 'beta' into duplicate-submissions

fae554a

jnm requested changes Jun 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Reject duplicate submissions #876

WIP - Reject duplicate submissions #876

noliveleger commented May 8, 2023 •

edited

jnm May 30, 2023

jnm May 30, 2023

jnm May 30, 2023

jnm May 30, 2023

jnm May 30, 2023

jnm May 30, 2023

jnm Jun 14, 2023 •

edited

jnm Jun 14, 2023

jnm Jun 14, 2023 •

edited

jnm Jul 6, 2023 •

edited

jnm Jun 14, 2023

jnm Jun 14, 2023

WIP - Reject duplicate submissions #876

Are you sure you want to change the base?

WIP - Reject duplicate submissions #876

Conversation

noliveleger commented May 8, 2023 • edited

Description

Additional info

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm Jun 14, 2023 • edited

Choose a reason for hiding this comment

jnm Jul 6, 2023 • edited

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noliveleger commented May 8, 2023 •

edited

jnm Jun 14, 2023 •

edited

jnm Jun 14, 2023 •

edited

jnm Jul 6, 2023 •

edited