Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Reject duplicate submissions #876

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/pytest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

🤔

env:
DJANGO_SECRET_KEY: ${{ secrets.DJANGO_SECRET_KEY }}
TEST_DATABASE_URL: postgis://kobo:kobo@localhost:5432/kobocat_test
Expand Down
3 changes: 2 additions & 1 deletion .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ test:
POSTGRES_PASSWORD: kobo
POSTGRES_DB: kobocat_test
SERVICE_ACCOUNT_BACKEND_URL: redis://redis_cache:6379/4
GIT_LAB: "True"
Copy link
Member

Choose a reason for hiding this comment

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

maybe something more descriptive like SKIP_TESTS_WITH_CONCURRENCY

services:
- name: postgis/postgis:14-3.2
alias: postgres
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

🤔


deploy-beta:
stage: deploy
Expand Down
20 changes: 19 additions & 1 deletion conftest.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# coding: utf-8
import os
import pytest
import sys

import fakeredis
import pytest
from django.conf import settings
from mock import patch

from onadata.libs.utils.storage import rmdir, default_storage

Expand Down Expand Up @@ -78,6 +80,22 @@ def setup(request):
request.addfinalizer(_tear_down)


@pytest.fixture(scope='session', autouse=True)
def default_session_fixture(request):
"""
Globally patch redis_client with fakeredis
"""
with patch(
'kobo_service_account.models.ServiceAccountUser.redis_client',
fakeredis.FakeStrictRedis(),
):
with patch(
'onadata.apps.django_digest_backends.cache.RedisCacheNonceStorage._get_cache',
fakeredis.FakeStrictRedis,
Comment on lines +90 to +94
Copy link
Member

Choose a reason for hiding this comment

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

why is one instantiated and the other isn't?

):
yield


def _tear_down():
print("\nCleaning testing environment...")
print('Removing MongoDB...')
Expand Down
1 change: 1 addition & 0 deletions dependencies/pip/dev_requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ pytest-env
mongomock
mock
httmock
fakeredis[lua]
11 changes: 9 additions & 2 deletions dependencies/pip/dev_requirements.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#
# This file is autogenerated by pip-compile with python 3.10
# To update, run:
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
# pip-compile dependencies/pip/dev_requirements.in
#
Expand Down Expand Up @@ -166,6 +166,8 @@ et-xmlfile==1.1.0
# via openpyxl
executing==0.8.3
# via stack-data
fakeredis[lua]==2.11.2
# via -r dependencies/pip/dev_requirements.in
gdata-python3==3.0.1
# via -r dependencies/pip/requirements.in
httmock==1.4.0
Expand Down Expand Up @@ -194,6 +196,8 @@ jwcrypto==1.0
# via django-oauth-toolkit
kombu==5.2.4
# via celery
lupa==1.14.1
# via fakeredis
lxml==4.8.0
# via
# -r dependencies/pip/requirements.in
Expand Down Expand Up @@ -290,6 +294,7 @@ redis==4.2.2
# celery
# django-redis
# django-redis-sessions
# fakeredis
# kobo-service-account
requests==2.27.1
# via
Expand Down Expand Up @@ -320,6 +325,8 @@ six==1.16.0
# isodate
# mongomock
# python-dateutil
sortedcontainers==2.4.0
# via fakeredis
sqlparse==0.4.2
# via
# -r dependencies/pip/dev_requirements.in
Expand Down
4 changes: 2 additions & 2 deletions dependencies/pip/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#
# This file is autogenerated by pip-compile with python 3.10
# To update, run:
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
# pip-compile dependencies/pip/requirements.in
#
Expand Down
19 changes: 19 additions & 0 deletions onadata/apps/api/tests/fixtures/users.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[
{
"fields": {
"date_joined": "2015-02-12T19:52:14.406Z",
"email": "[email protected]",
"first_name": "bob",
"groups": [],
"is_active": true,
"is_staff": false,
"is_superuser": false,
"last_login": "2015-02-12T19:52:14.406Z",
"last_name": "bob",
"password": "pbkdf2_sha256$260000$jSfi1lb5FclOUV9ZodfCdP$Up19DmjLFtBh0VREyow/oduVkwEoqQftljfwq6b9vIo=",
"username": "bob"
},
"model": "auth.user",
"pk": 2
}
]
109 changes: 109 additions & 0 deletions onadata/apps/api/tests/viewsets/test_xform_submission_api.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
# coding: utf-8
import multiprocessing
import os
import uuid
from collections import defaultdict
from functools import partial

import pytest
import requests
import simplejson as json
from django.conf import settings
from django.contrib.auth.models import AnonymousUser
from django.core.files.uploadedfile import InMemoryUploadedFile
from django.test.testcases import LiveServerTestCase
from django.urls import reverse
from django_digest.test import DigestAuth
from guardian.shortcuts import assign_perm
from kobo_service_account.utils import get_request_headers
Expand All @@ -15,6 +22,7 @@
TestAbstractViewSet
from onadata.apps.api.viewsets.xform_submission_api import XFormSubmissionApi
from onadata.apps.logger.models import Attachment
from onadata.apps.main import tests as main_tests
from onadata.libs.constants import (
CAN_ADD_SUBMISSIONS
)
Expand Down Expand Up @@ -441,6 +449,7 @@ def test_edit_submission_with_service_account(self):
self.assertEqual(
response['Location'], 'http://testserver/submission'
)

def test_submission_blocking_flag(self):
# Set 'submissions_suspended' True in the profile metadata to test if
# submission do fail with the flag set
Expand Down Expand Up @@ -488,3 +497,103 @@ def test_submission_blocking_flag(self):
)
response = self.view(request, username=username)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)


class ConcurrentSubmissionTestCase(LiveServerTestCase):
"""
Inherit from LiveServerTestCase to be able to test concurrent requests
to submission endpoint in different transactions (and different processes).
Otherwise, DB is populated only on the first request but still empty on
subsequent ones.
"""

fixtures = ['onadata/apps/api/tests/fixtures/users']

def publish_xls_form(self):

path = os.path.join(
settings.ONADATA_DIR,
'apps',
'main',
'tests',
'fixtures',
'transportation',
'transportation.xls',
)

xform_list_url = reverse('xform-list')
self.client.login(username='bob', password='bob')
with open(path, 'rb') as xls_file:
post_data = {'xls_file': xls_file}
response = self.client.post(xform_list_url, data=post_data)

assert response.status_code == status.HTTP_201_CREATED

@pytest.mark.skipif(
settings.GIT_LAB, reason='GitLab does not seem to support multi-processes'
)
def test_post_concurrent_same_submissions(self):

DUPLICATE_SUBMISSIONS_COUNT = 2 # noqa

self.publish_xls_form()
username = 'bob'
survey = 'transport_2011-07-25_19-05-49'
results = defaultdict(int)

with multiprocessing.Pool() as pool:
for result in pool.map(
partial(
submit_data,
live_server_url=self.live_server_url,
survey_=survey,
username_=username,
),
range(DUPLICATE_SUBMISSIONS_COUNT),
):
results[result] += 1

assert results[status.HTTP_201_CREATED] == 1
assert results[status.HTTP_409_CONFLICT] == DUPLICATE_SUBMISSIONS_COUNT - 1
Copy link
Member

Choose a reason for hiding this comment

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

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



def submit_data(identifier, survey_, username_, live_server_url):
"""
Submit data to live server.

It has to be outside `ConcurrentSubmissionTestCase` class to be pickled by
`multiprocessing.Pool().map()`.
"""
media_file = '1335783522563.jpg'
main_directory = os.path.dirname(main_tests.__file__)
path = os.path.join(
main_directory,
'fixtures',
'transportation',
'instances',
survey_,
media_file,
)
with open(path, 'rb') as f:
f = InMemoryUploadedFile(
f,
'media_file',
media_file,
'image/jpg',
os.path.getsize(path),
None,
)
Comment on lines +578 to +585
Copy link
Member

Choose a reason for hiding this comment

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

do we have a test somewhere for:

  1. client uploads XML with no attachment -> accept
  2. client uploads same XML again -> reject
  3. client uploads same XML but now with an attached file -> accept; attach file to existing submission

submission_path = os.path.join(
main_directory,
'fixtures',
'transportation',
'instances',
survey_,
f'{survey_}.xml',
)
with open(submission_path) as sf:
files = {'xml_submission_file': sf, 'media_file': f}
response = requests.post(
f'{live_server_url}/{username_}/submission', files=files
)
return response.status_code
2 changes: 1 addition & 1 deletion onadata/apps/django_digest_backends/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
NONCE_NO_COUNT = '' # Needs to be something other than None to determine not set vs set to null


class RedisCacheNonceStorage():
class RedisCacheNonceStorage:
_blocking_timeout = 30

def _get_cache(self):
Expand Down
35 changes: 34 additions & 1 deletion onadata/apps/logger/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# coding: utf-8
from django.utils.translation import gettext as t

class ConflictingXMLHashInstanceError(Exception):
pass


class DuplicateInstanceError(Exception):

def __init__(self, message='Duplicate Instance'):
super().__init__(message)


class DuplicateUUIDError(Exception):
Expand All @@ -10,5 +18,30 @@ class FormInactiveError(Exception):
pass


class InstanceEmptyError(Exception):

def __init__(self, message='Empty instance'):
super().__init__(message)


class InstanceInvalidUserError(Exception):
def __init__(self, message='Could not determine the user'):
super().__init__(message)


class InstanceMultipleNodeError(Exception):
pass


class InstanceParseError(Exception):

def __init__(self, message='The instance could not be parsed'):
super().__init__(message)


class TemporarilyUnavailableError(Exception):
pass


class XLSFormError(Exception):
pass
Loading