Fix 462 #463

gusmith · 2019-11-11T05:24:04Z

Closes #462 which describes the fact that a dataprovider was able to upload clks multiple time to the same project using the same token.
In this PR, we solve this issue by modifying the uploaded value in the table dataproviders in the database from a boolean to an enum which can have the following values:

not_started
in_progress
done
error

When posting clks to a project, we check that the token is not already in use (i.e. uploaded set to in_progress or done), and post if possible. In an error case, uploaded will be set to error, which will enable the owner of the token to try uploading their data again, not to block a project if an upload failed.

Main reason to have a single method to do both is against race condition: we are relying on postgres for the lock, ensuring that we cannot update twice this value to true.

- not_started when we haven't started uploading the clks - in_progress when we start uploading them (lock the resource) - done when completed - error when an error occured.

One checking that re-using a token fails. One checking that uploading failing data does not block the project (can re-upload).

…t them or lock the resource. If an error occurs, free back the resource for later upload.

wilko77

I really don't like that we have UPLOADSTATE and UPLOADEDSTATE in the db now.
It is really hard to understand what they are for without spending quite some time reading through code. I made a suggestion how we can resolve that in the comments.

wilko77 · 2019-11-11T23:09:30Z

backend/entityservice/database/insertions.py

+
+def get_and_set_dataprovider_upload_state_in_progress(db, dp_id):
+    """
+    This method returns true if it was able to update the uploaded status of this dataprovider from false to true.


the status of the dataproviders is not true/false any more.

Well seen. I started by using the boolean value, but added more states to have an in_progress one. But I forgot the comments...

wilko77 · 2019-11-11T23:09:42Z

backend/entityservice/database/insertions.py

+    This method returns true if it was able to update the uploaded status of this dataprovider from false to true.
+    It return false otherwise (i.e. the state was already set to true).
+    """
+    logger.debug("Setting dataprovider {} upload state to True".format(dp_id))


I re-phrased the whole docstring.

wilko77 · 2019-11-12T00:03:46Z

backend/entityservice/views/project.py

@@ -121,57 +121,69 @@ def project_clks_post(project_id):
    with DBConn() as conn:
        dp_id = db.get_dataprovider_id(conn, token)
        project_encoding_size = db.get_project_schema_encoding_size(conn, project_id)
+        upload_state_updated = db.get_and_set_dataprovider_upload_state_in_progress(conn, dp_id)


this is a bit cryptic. The function get_and_set_dataprovider_upload_state_in_progress mixes two tasks.
How about if we split this to make it clearer:

if not db.dataprovider_allowed_to_upload(conn, project_id, dp_id, ...): return safe_fail_request(403, "This token has already been used to upload clks.") db.set_dp_upload_state_in_progress(...) ...

The main point is to do both actions at once not to have race conditions. Otherwise we would first assess it, and then setting it which should return an error if already set. I thus rename the method is_dataprovider_allowed_to_upload_and_lock.

wilko77 · 2019-11-12T00:10:15Z

backend/entityservice/init-db-schema.sql

@@ -85,6 +85,12 @@ RETURNS bool AS $$
  SELECT $1.state = 'completed'
 $$ STABLE LANGUAGE SQL;

+CREATE TYPE UPLOADEDSTATE AS ENUM (


wow, now we have UPLOADEDSTATE and UPLOADSTATE. This is very confusing.
After looking at those for a bit, I think we should rename UPLOADSTATE to PROCESSEDSTATE.
It would also be nice to add a bit of an explanation what all those states are for.
In my view,
UPLOADSTATE describes the state of the processing of the uploaded clks. This is only set after CLKs have been uploaded. 'pending' means we are waiting for some processing to be done, 'ready' means done and 'error' if something fails.
UPLOADEDSTATE describes the state of the CLK upload.

That was indeed a pickle. I will add comments on the UPLOADEDSTATE.
I didn't want to touch the UPLOADSTATE as I wasn't sure what it was made for (as you mentioned, this is not well documented). But I'll also add some documentation around it and rename it. In which case I will also rename UPLOADEDSTATE to UPLOADSTATE which sounds better :)

I'll finally keep UPLOADEDSTATE instead of renaming it because the value in the table is uploaded. But renamed the UPLOADSTATE to PROCESSEDSTATE, adding some documentation.

wilko77 · 2019-11-12T00:13:04Z

backend/entityservice/tests/test_project_uploads.py

@@ -235,3 +235,62 @@ def test_project_single_party_empty_data_upload(
    )
    assert r.status_code == 400

+
+def test_project_upload_wrong_authentication(requests, valid_project_params):


you can be more specific here. You are testing that you cannot upload twice.

wilko77 · 2019-11-12T00:14:04Z

backend/entityservice/tests/test_project_uploads.py

+        small_file_path, new_project_data['project_id'], token_to_reuse, 1000, expected_status_code=403)
+
+
+def test_project_upload_fail_then_works(requests, valid_project_params):


that's a great name for this test

I was inspired :)

I was not happy with the previous name...

wilko77 · 2019-11-12T04:33:35Z

backend/entityservice/database/insertions.py

+def is_dataprovider_allowed_to_upload_and_lock(db, dp_id):
+    """
+    This method returns true if the dataprovider is allowed to upload her clks.
+    A data provider is allowed to upload clks if they have not yet been uploaded or if the upload is already in progress.


I think you mean something different with the second if.
A DP is not allowed to upload if data has been uploaded, or if an upload is currently underway.

wilko77 · 2019-11-12T04:35:43Z

backend/entityservice/database/insertions.py

+    if length < 1:
+        return False
+    elif length > 1:
+        raise ValueError("Houston, we have a problem!!! This dataprovider can upload multiple times her clks.")


it would be good to write a message to the logger at high severity level as well here.

wilko77 · 2019-11-12T04:36:34Z

backend/entityservice/init-db-schema.sql

+  'in_progress', -- the upload is in progress, so no-one else should be able to upload at the same time
+  'done', -- the clks have been uploaded, it should stay this way
+  'error' -- the dataprovider has tried to upload the clks but an error occurred, having this state allows a dataprovider to try again.
+);


wilko77 · 2019-11-12T04:38:28Z

backend/entityservice/views/project.py

@@ -121,57 +121,69 @@ def project_clks_post(project_id):
    with DBConn() as conn:
        dp_id = db.get_dataprovider_id(conn, token)
        project_encoding_size = db.get_project_schema_encoding_size(conn, project_id)
+        upload_state_updated = db.get_and_set_dataprovider_upload_state_in_progress(conn, dp_id)


gusmith added 7 commits November 11, 2019 16:06

Single method to get and set the upload status in db.

13bdbc0

Main reason to have a single method to do both is against race condition: we are relying on postgres for the lock, ensuring that we cannot update twice this value to true.

4 states for uploaded in the dataprovider table.

bbadadd

- not_started when we haven't started uploading the clks - in_progress when we start uploading them (lock the resource) - done when completed - error when an error occured.

Use the state done instead of the boolean TRUE.

784ef41

Two new tests related to this new feature.

3d8890b

One checking that re-using a token fails. One checking that uploading failing data does not block the project (can re-upload).

The main change when posting clks to a project, check that we can pos…

56b5fc1

…t them or lock the resource. If an error occurs, free back the resource for later upload.

Tiny cleanup of a variable.

104f49e

Some comments in the tests.

08fc85b

gusmith requested a review from wilko77 November 11, 2019 05:24

gusmith added 3 commits November 11, 2019 16:24

Update changelo

db74fc0

Typo in changelog

0680f36

Clean test and modifying variable name.

83263ba

wilko77 requested changes Nov 12, 2019

View reviewed changes

gusmith added 6 commits November 12, 2019 14:04

Rename method to update upload state and add nicer documentation.

5474008

Change dur to remanimg

ce02a82

Rename the test methods to be more explicit.

13e65da

Rename an enum and add documentation about them.

e5a7b7b

Typos.

3a30ad2

Rename again a test method.

7216f31

I was not happy with the previous name...

gusmith requested a review from wilko77 November 12, 2019 03:22

wilko77 reviewed Nov 12, 2019

View reviewed changes

wilko77 approved these changes Nov 12, 2019

View reviewed changes

Documentation correction from PR review.

6a68f18

gusmith merged commit 84eff7c into develop Nov 12, 2019

gusmith deleted the fix-462 branch November 12, 2019 06:21

hardbyte mentioned this pull request Feb 10, 2020

Release: app version 1.13.0-beta, frontend version 1.4.6-beta #498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 462 #463

Fix 462 #463

gusmith commented Nov 11, 2019

wilko77 left a comment

wilko77 Nov 11, 2019

gusmith Nov 12, 2019

wilko77 Nov 11, 2019

gusmith Nov 12, 2019

wilko77 Nov 12, 2019

gusmith Nov 12, 2019

wilko77 Nov 12, 2019

wilko77 Nov 12, 2019

gusmith Nov 12, 2019

gusmith Nov 12, 2019

wilko77 Nov 12, 2019

gusmith Nov 12, 2019

wilko77 Nov 12, 2019

gusmith Nov 12, 2019

wilko77 Nov 12, 2019

wilko77 Nov 12, 2019

wilko77 Nov 12, 2019

wilko77 Nov 12, 2019

		small_file_path, new_project_data['project_id'], token_to_reuse, 1000, expected_status_code=403)


		def test_project_upload_fail_then_works(requests, valid_project_params):

Fix 462 #463

Fix 462 #463

Conversation

gusmith commented Nov 11, 2019

wilko77 left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment