Upload to existing indexd record #1147

BinamB · 2024-05-21T18:44:43Z

Change reflected in gen3-client uc-cdis/cdis-data-client#131

Improvements

/upload endpoint accepts optional guid in request body. If the guid exists in indexd, it does not create a new record, else it creates a blank record with the provided guid.
/multipart/init endpoint accepts optional guid in request body. If the guid exists in indexd, it does not create a new record, else it creates a blank record with the provided guid.

Avantol13 · 2024-05-24T14:12:33Z

fence/blueprints/data/blueprint.py

@@ -134,6 +134,8 @@ def upload_data_file():
    authz = params.get("authz")
    uploader = None

+    guid = params.get("did")


I would prefer we leave did an implementation detail in indexd. Can we call this guid?

Avantol13 · 2024-05-24T14:16:53Z

fence/blueprints/data/blueprint.py

+        file_name=params["file_name"],
+        authz=params.get("authz"),
+        uploader=uploader,
+        guid=guid,


What happened if an invalid or non-existent GUID is specified? Should the behavior be to 400 or move forward creating a blank record? I think we need to consider things like this. If I supply a GUID to the public API, I probably don't want a blank record. So if I mistyped the GUID I'd actually expect an error, not for it to move forward and create a blank record anyway

Avantol13 · 2024-05-24T14:21:16Z

fence/blueprints/data/indexd.py

+                document = indexd_response.json()
+                self.guid = document["did"]
+                self.logger.info(f"Record with {self.guid} id found in Indexd.")
+                return document


I'm not a huge fan of early returns in a longer function like this since at a glance it's unclear where things end. Maybe just breaking some of this into a helper function(s) would help. document = _create_blank_record() and put everything after 288 in there basically. This is nitpicky so you don't need to do this, I just think the readability suffers with early returns unless the function itself is pretty small

Great updates, looks much cleaner now, thanks!

Avantol13 · 2024-05-24T14:23:08Z

tests/data/test_data.py

@@ -916,6 +960,26 @@ def test_non_public_authz_and_public_acl_object_upload_file(
    Test that a user can successfully generate an upload url for an Indexd
    record with a non-public authz field and a public acl field.
    """
+    did = str(uuid.uuid4())


I think you need to modify these to ensure you capture these test cases:

When the guid is provided and it already exists

When the guid is provided and it doesn't exist

When the guid is not provided

I believe you're only capturing the second one right now

Avantol13 · 2024-06-03T16:12:43Z

fence/blueprints/data/blueprint.py

@@ -165,7 +167,10 @@ def upload_data_file():
        )

    blank_index = BlankIndex(
-        file_name=params["file_name"], authz=params.get("authz"), uploader=uploader
+        file_name=params["file_name"],
+        authz=params.get("authz"),


this was already there, but I think this can just be authz

Avantol13 · 2024-06-03T16:16:54Z

fence/blueprints/data/indexd.py

@@ -266,6 +272,21 @@ def index_document(self):
                response from indexd (the contents of the record), containing ``guid``
                and ``url``
        """
+
+        if self.guid:
+            index_url = self.indexd.rstrip("/") + "/index/" + self.guid


We may need to percent-encode the guid b/c it's user input, so I'm wondering if there's some security concerns here (like what if I pass in slashes, ../admin/ as the guid. I don't know if requests does that by default and this might get flagged as unsanitized user input in our security scans if it hasn't already. Not sure if/how we handle that elsewhere, just a thought

fence/blueprints/data/indexd.py

Avantol13 · 2024-06-03T16:23:05Z

fence/blueprints/data/indexd.py

+            indexd_response = requests.get(index_url)
+            if indexd_response.status_code == 200:
+                document = indexd_response.json()
+                self.guid = document["did"]


this is also set outside this cached_property.. I'd remove this and mirror the way the blank ID works (it just returns the whole document and let's the init set the guid)

Avantol13 · 2024-06-03T16:24:28Z

fence/blueprints/data/indexd.py

+                document = indexd_response.json()
+                self.guid = document["did"]
+                self.logger.info(f"Record with {self.guid} id found in Indexd.")
+                return document


Great updates, looks much cleaner now, thanks!

Avantol13 · 2024-06-03T16:24:41Z

openapis/swagger.yaml

@@ -565,6 +565,8 @@ paths:
        Previous authorization check requires a more general, global upload permission:
        "file_upload" on "/data_file" resource. When "authz" is *not* provided, this
        endpoint will check for that permission for your user.
+
+        Accepts a "did" field in the request body. If "did" is provided, it checks indexd for an existing record. If not found, a new blank record will be created.


guid not did

Avantol13 · 2024-06-03T16:24:49Z

openapis/swagger.yaml

+        the GUID for this new record and an uploadId for multipart upload presigned url.
+
+
+        Accepts a "did" field in the request body. If "did" is provided, it checks indexd for an existing record. If not found, a new blank record will be created.


tests/data/test_data.py

Avantol13 · 2024-06-03T16:26:51Z

tests/data/test_data.py

+
+        if indexd_200_response:
+            assert response.status_code == 201, response
+            assert "guid" in response.json


can you also check that the content of the guid is the same value as the one passed into the endpoint?

Avantol13 · 2024-06-03T16:29:19Z

fence/blueprints/data/indexd.py

+                self.logger.info(f"Record with {self.guid} id found in Indexd.")
+                return document
+            else:
+                raise NotFound("No indexed document found with id {}".format(self.guid))


Nice, yeah I think this is the right logic if I pass a guid 👍

Any reason you're using .format over f-strings here? There are other places with f-strings, I tend to prefer those. Not a big deal, just some inconsistency

Avantol13 · 2024-07-01T15:56:03Z

fence/blueprints/data/indexd.py

+        self.guid = guid
+        self.guid = self.index_document[
+            "did"
+        ]  # .index_document is a cached property with code below, it creates/retrieves the actual record and this line updates the stored GUID to the returned record


Python comments should be on the line above, try to avoid in-line unless required by static code checking exclusions etc

Avantol13 · 2024-07-01T16:00:10Z

openapis/swagger.yaml

@@ -565,6 +565,8 @@ paths:
        Previous authorization check requires a more general, global upload permission:
        "file_upload" on "/data_file" resource. When "authz" is *not* provided, this
        endpoint will check for that permission for your user.
+
+        Accepts a "guid" field in the request body. If "guid" is provided, it checks indexd for an existing record. If not found, a new blank record will be created.


It doesn't seem like this is the behavior anymore? If it's not found, it raises a 404

Avantol13 · 2024-07-01T16:00:42Z

openapis/swagger.yaml

@@ -1686,6 +1691,10 @@ components:
          description: >-
            the requested bucket to upload to.
            If not provided, defaults to the configured DATA_UPLOAD_BUCKET.
+        guid:


In addition to this new param, we also need to update the example responses to include the 404 that can now happen

Avantol13 · 2024-07-01T16:01:43Z

tests/data/test_data.py

@@ -1664,6 +1730,76 @@ def json(self):
        assert "uploadId" in response.json


+@pytest.mark.parametrize("indexd_200_response", [True, False])


these should be separate unit tests. One is testing when the GUID exists, one is testing when it doesn't. If you want to share some code, you can create helper functions, but I'd rather keep the positive/negative testing as separate unit tests

coveralls · 2024-07-02T16:10:21Z

Pull Request Test Coverage Report for Build 9764333492

Details

0 of 0 changed or added relevant lines in 0 files are covered.
41 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.03%) to 75.083%

Files with Coverage Reduction	New Missed Lines	%
resources/ga4gh/passports.py	2	87.43%
blueprints/data/blueprint.py	9	86.75%
blueprints/data/indexd.py	30	94.65%

Totals
Change from base Build 9713845019:	0.03%
Covered Lines:	7720
Relevant Lines:	10282

💛 - Coveralls

coveralls · 2024-07-31T16:49:53Z

Pull Request Test Coverage Report for Build 10184257323

Details

0 of 0 changed or added relevant lines in 0 files are covered.
41 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.03%) to 75.172%

Files with Coverage Reduction	New Missed Lines	%
resources/ga4gh/passports.py	2	87.43%
blueprints/data/blueprint.py	9	86.75%
blueprints/data/indexd.py	30	94.69%

Totals
Change from base Build 10164514863:	0.03%
Covered Lines:	7763
Relevant Lines:	10327

💛 - Coveralls

BinamB added 5 commits May 13, 2024 10:36

Add parameter to init endpoint

24f2950

add conditions

f4bd84a

print things

110ef82

simplify

b175896

add did for upload request

60a932e

github-actions bot added the @requires-fence label May 21, 2024

BinamB added 2 commits May 22, 2024 14:14

test fixes

46f1f94

Fix index get

ae584c5

BinamB mentioned this pull request May 22, 2024

Add Param to not create blank records for upload uc-cdis/cdis-data-client#131

Open

Merge branch 'master' into chore/multpart-init-change

6641fb5

BinamB marked this pull request as ready for review May 23, 2024 18:44

Update Swagger

b79cc73

Avantol13 requested changes May 24, 2024

View reviewed changes

BinamB added 2 commits May 30, 2024 16:54

Add tests

5cd0b7d

update swagger

e749870

Avantol13 requested changes Jun 3, 2024

View reviewed changes

PR review resolve

71ec4e7

Avantol13 requested changes Jul 1, 2024

View reviewed changes

BinamB and others added 2 commits July 2, 2024 11:04

Review changes

391865b

Merge branch 'master' into chore/multpart-init-change

c551947

PlanXCyborg added test-apis-centralizedAuth test-apis-dbgapTest and removed @requires-fence labels Jul 17, 2024

BinamB added 5 commits July 18, 2024 10:21

Update version

e9d51be

lock file

60f257c

revert

caabf4d

update version number

77d2ca7

Merge branch 'master' into chore/multpart-init-change

c3f7490

Avantol13 approved these changes Jul 31, 2024

View reviewed changes

BinamB merged commit 4d98387 into master Aug 7, 2024
9 of 10 checks passed

BinamB deleted the chore/multpart-init-change branch August 7, 2024 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload to existing indexd record #1147

Upload to existing indexd record #1147

BinamB commented May 21, 2024 •

edited

Loading

Avantol13 May 24, 2024

Avantol13 May 24, 2024

Avantol13 May 24, 2024

Avantol13 Jun 3, 2024

Avantol13 May 24, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jun 3, 2024

Avantol13 Jul 1, 2024

Avantol13 Jul 1, 2024

Avantol13 Jul 1, 2024

Avantol13 Jul 1, 2024

coveralls commented Jul 2, 2024 •

edited

Loading

coveralls commented Jul 31, 2024

		the GUID for this new record and an uploadId for multipart upload presigned url.


		Accepts a "did" field in the request body. If "did" is provided, it checks indexd for an existing record. If not found, a new blank record will be created.

		@@ -1664,6 +1730,76 @@ def json(self):
		assert "uploadId" in response.json


		@pytest.mark.parametrize("indexd_200_response", [True, False])

Upload to existing indexd record #1147

Upload to existing indexd record #1147

Conversation

BinamB commented May 21, 2024 • edited Loading

Improvements

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

coveralls commented Jul 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9764333492

Details

💛 - Coveralls

coveralls commented Jul 31, 2024

Pull Request Test Coverage Report for Build 10184257323

Details

💛 - Coveralls

BinamB commented May 21, 2024 •

edited

Loading

coveralls commented Jul 2, 2024 •

edited

Loading