Fix bugs and add integration tests for files operations (merge commit)

Merge branch 'feature/files-tests' into 'main'
* Test posting and getting files from multiple projects

* Add integration tests for file operations

* Fix multiple bugs in updating and removing submission files. Split putting files to a submission to a separate function.

* Fix operation bug in adding submission files to db. Fix typo

Closes #820 and #822
See merge request https://gitlab.ci.csc.fi/sds-dev/sd-submit/metadata-submitter/-/merge_requests/877

Reviewed-by: Joonatan Mäkinen <[email protected]>
Approved-by: Joonatan Mäkinen <[email protected]>
Merged by Monika Radaviciute <[email protected]>

metadata_backend/api/handlers/submission.py

@@ -291,7 +291,7 @@ async def put_submission_path(self, req: Request) -> Response:
         """Put or replace metadata to a submission.
 
         :param req: PUT request with metadata schema in the body
-        :returns: HTTP No Content response
+        :returns: updated submission object
         """
         submission_id = req.match_info["submissionId"]
         db_client = req.app["db_client"]
@@ -312,42 +312,60 @@ async def put_submission_path(self, req: Request) -> Response:
         elif req.path.endswith("rems"):
             schema = "rems"
             await self.check_rems_ok({"rems": data})
-        elif req.path.endswith("files"):
-            schema = "files"
         else:
             raise web.HTTPNotFound(reason=f"'{req.path}' does not exist")
 
-        if schema == "files":
-            file_operator = FileOperator(db_client)
-            # we expect to get a list of dict for files
-            # that matches the json schema when added to submission object
-            submission[schema] = data
-            JSONValidator(submission, "submission").validate
-            for file in data:
-                if "accessionId" not in file:
-                    reason = f"Updating {submission_id} failed. Files require an accessionId."
-                    LOG.error(reason)
-                    raise web.HTTPBadRequest(reason=reason)
-                _file_accessionId = data[file].pop("accessionId")
-                _file_update_op = {f"files.$.{k}": v for k, v in data[file].items()}
-                await file_operator.update_file_submission(_file_accessionId, submission_id, _file_update_op)
-        else:
-            op = "add"
-            if schema in submission:
-                op = "replace"
-            patch = [
-                {"op": op, "path": f"/{schema}", "value": data},
-            ]
-
-            submission[schema] = data
-            JSONValidator(submission, "submission").validate
+        op = "add"
+        if schema in submission:
+            op = "replace"
+        patch = [
+            {"op": op, "path": f"/{schema}", "value": data},
+        ]
 
-            upd_submission = await operator.update_submission(submission_id, patch)
+        submission[schema] = data
+        JSONValidator(submission, "submission").validate
 
+        upd_submission = await operator.update_submission(submission_id, patch)
         body = ujson.dumps({"submissionId": upd_submission}, escape_forward_slashes=False)
+
         LOG.info("PUT %r in submission with ID: %r was successful.", schema, submission_id)
         return web.Response(body=body, status=200, content_type="application/json")
 
+    async def put_submission_files(self, req: Request) -> Response:
+        """Put files to a submission.
+
+        :param req: PUT request with metadata schema in the body
+        :returns: HTTP No Content response
+        """
+        submission_id = req.match_info["submissionId"]
+        db_client = req.app["db_client"]
+        operator = SubmissionOperator(db_client)
+
+        # Check submission exists and is not already published
+        await operator.check_submission_exists(submission_id)
+        await operator.check_submission_published(submission_id, req.method)
+        await self._handle_check_ownership(req, "submission", submission_id)
+
+        submission = await operator.read_submission(submission_id)
+        data: list[dict[str, Any]] = await req.json()
+
+        file_operator = FileOperator(db_client)
+        # we expect to get a list of dict for files
+        # that matches the json schema when added to submission object
+        submission["files"] = data
+        JSONValidator(submission, "submission").validate
+        for file in data:
+            if "accessionId" not in file:
+                reason = f"Updating {submission_id} failed. Files require an accessionId."
+                LOG.error(reason)
+                raise web.HTTPBadRequest(reason=reason)
+            _file_accessionId = file.pop("accessionId")
+            _file_update_op = {f"files.$.{k}": v for k, v in file.items()}
+            await file_operator.update_file_submission(_file_accessionId, submission_id, _file_update_op)
+
+        LOG.info("PUT files in submission with ID: %r was successful.", submission_id)
+        return web.HTTPNoContent()
+
     async def get_submission_files(self, req: Request) -> Response:
         """Get files from a submission with the version present in submission.
 
@@ -409,11 +427,10 @@ async def add_submission_files(self, req: Request) -> Response:
         raise web.HTTPBadRequest(reason=reason)
 
     async def delete_submission_files(self, req: Request) -> Response:
-        """Remove files from a submission.
-
-        Body needs to contain a list of accessionId for files.
+        """Remove a file from a submission.
 
         :param req: DELETE request
+        :raises HTTP Not Found if file not associated with submission
         :returns: HTTP No Content response
         """
         submission_id = req.match_info["submissionId"]
@@ -429,6 +446,13 @@ async def delete_submission_files(self, req: Request) -> Response:
 
         file_operator = FileOperator(db_client)
 
+        file_in_submission = await file_operator.check_submission_has_file(submission_id, file_accession_id)
+
+        if not file_in_submission:
+            reason = f"File with accession id {file_accession_id} not found in submission {submission_id}"
+            LOG.error(reason)
+            raise web.HTTPNotFound(reason=reason)
+
         await file_operator.remove_file_submission(file_accession_id, "accessionId", submission_id)
         LOG.info(
             "Removing file: %r from submission with ID: %r was successful.",

metadata_backend/api/operators/file.py

@@ -320,48 +320,47 @@ async def flag_file_deleted(self, file_path: str, deleted: bool = True) -> None:
         LOG.info("Flagging file with file_path: %r as Deleted succeeded.", file_path)
 
     async def remove_file_submission(
-        self, accession_id: str, id_type: Optional[str] = None, submission_id: Optional[str] = None
+        self, id_or_path: str, id_type: Optional[str] = None, submission_id: Optional[str] = None
     ) -> None:
-        """Flag file as deleted.
-
-        If we flag the file as deleted we should remove it from any submission it has been attached to
+        """Remove file from a submission or all submissions.
 
-        :param accession_id: Accession ID of the file to read
+        :param id_or_path: Accession ID or path of the file to remove from submission
         :param id_type: depending on the file id this can be either ``path`` or ``accessionId``.
         :param submission_id: Submission ID to remove file associated with it
         :raises: HTTPBadRequest if deleting was not successful
         :raises: HTTPInternalServerError if db operation failed because of connection
         or other db issue
         """
+        _file: dict[str, Any] = {}
         try:
             if id_type == "path":
-                _fileId = await self.db_service.read_by_key_value("file", {"path": accession_id}, {"accessionId": 1})
+                _file = await self.db_service.read_by_key_value("file", {"path": id_or_path}, {"accessionId": 1})
             elif id_type == "accessionId":
-                _fileId["accessionId"] = accession_id
+                _file["accessionId"] = id_or_path
             else:
                 reason = f"Cannot recognize '{id_type}' as a type of id for file deletion from submission."
                 LOG.error(reason)
                 raise web.HTTPBadRequest(reason=reason)
             if submission_id:
                 delete_success = await self.db_service.remove(
-                    "submission", submission_id, {"files": {"accession_id": _fileId["accessionId"]}}
+                    "submission", submission_id, {"files": {"accessionId": _file["accessionId"]}}
                 )
-                LOG.info("Removing file: %r from submission: %r succeeded.", accession_id, submission_id)
+                LOG.info("Removing file: %r from submission: %r succeeded.", id_or_path, submission_id)
             else:
                 delete_success = await self.db_service.remove_many(
-                    "submission", {"files": {"accession_id": _fileId["accessionId"]}}
+                    "submission", {"files": {"accessionId": _file["accessionId"]}}
                 )
                 LOG.info(
                     "Removing file with path: %r from submissions, by accessionID: %r succeeded.",
-                    accession_id,
-                    _fileId["accessionId"],
+                    id_or_path,
+                    _file["accessionId"],
                 )
         except (ConnectionFailure, OperationFailure) as error:
             reason = f"Error happened while removing file from submission, err: {error}"
             LOG.exception(reason)
             raise web.HTTPInternalServerError(reason=reason) from error
         if not delete_success:
-            reason = f"Removing file identified via '{id_type}': '{accession_id}' from submission failed."
+            reason = f"Removing file identified via '{id_type}': '{id_or_path}' from submission failed."
             LOG.error(reason)
             raise web.HTTPBadRequest(reason=reason)
 
@@ -407,7 +406,18 @@ async def add_files_submission(self, files: list[dict[str, Any]], submission_id:
         :param submission_id: Submission ID to add files to
         :returns: True if operation to append was successful
         """
-        success: bool = await self.db_service.append(
-            "submission", submission_id, {"$addToSet": {"files": {"$each": files}}}
-        )
+        success: bool = await self.db_service.append("submission", submission_id, {"files": {"$each": files}})
         return success
+
+    async def check_submission_has_file(self, submission_id: str, file_id: str) -> bool:
+        """Check if submission has a file with given accession id.
+
+        :param submission_id: submission ID to check files of
+        :param file_id: accession ID of file
+        :returns: True if file found
+        """
+        submission = await self.db_service.read("submission", submission_id)
+        for file in submission["files"]:
+            if file["accessionId"] == file_id:
+                return True
+        return False

metadata_backend/database/db_service.py

@@ -315,13 +315,13 @@ async def remove_many(self, collection: str, data_to_be_removed: str | dict[str,
 
     @auto_reconnect
     async def append(
-        self, collection: str, accession_id: str, data_to_be_addded: str | dict[str, Any], upsert: bool = False
+        self, collection: str, accession_id: str, data_to_be_added: str | dict[str, Any], upsert: bool = False
     ) -> dict[str, Any]:
         """Append data by to object with accessionId in collection.
 
         :param collection: Collection where document should be searched from
         :param accession_id: ID of the object/submission/user to be appended to
-        :param data_to_be_addded: str or JSON representing the data that should be
+        :param data_to_be_added: str or JSON representing the data that should be
         updated to removed.
         :param upsert: If the document does not exist add it
         :returns: JSON after remove if operation was successful
@@ -331,14 +331,14 @@ async def append(
         # push vs addtoSet
         # push allows us to specify the postion but it does not check the items are unique
         # addToSet cannot easily specify position
-        append_op = {"$push": data_to_be_addded}
+        append_op = {"$push": data_to_be_added}
         result: dict[str, Any] = await self.database[collection].find_one_and_update(
             find_by_id, append_op, projection={"_id": False}, upsert=upsert, return_document=ReturnDocument.AFTER
         )
         LOG.debug(
             "DB doc in collection: %r with data: %r appeneded for accession ID: %r.",
             collection,
-            data_to_be_addded,
+            data_to_be_added,
             accession_id,
         )
         return result

metadata_backend/server.py

@@ -130,7 +130,7 @@ async def on_prepare(_: web.Request, response: web.StreamResponse) -> None:
         web.post("/submissions/{submissionId}/files", _submission.add_submission_files),
         web.put("/submissions/{submissionId}/doi", _submission.put_submission_path),
         web.put("/submissions/{submissionId}/rems", _submission.put_submission_path),
-        web.put("/submissions/{submissionId}/files", _submission.put_submission_path),
+        web.put("/submissions/{submissionId}/files", _submission.put_submission_files),
         web.patch("/submissions/{submissionId}", _submission.patch_submission),
         web.delete("/submissions/{submissionId}", _submission.delete_submission),
         web.delete("/submissions/{submissionId}/files/{fileId}", _submission.delete_submission_files),

tests/integration/helpers.py

@@ -649,12 +649,12 @@ async def check_submissions_object_patch(sess, submission_id, schema, accession_
         return schema
 
 
-async def post_project_file(sess, file_data):
-    """Post one file within session, returns created file id.
+async def post_project_files(sess, file_data):
+    """Post files within session, returns created file ids.
 
     :param sess: HTTP session in which request call is made
     :param file_data: new file data containing userId, projectId, file info
-    :return: file id of created file
+    :return: list of file ids of created files
     """
 
     async with sess.post(
@@ -666,6 +666,19 @@ async def post_project_file(sess, file_data):
         return ans["fileIds"]
 
 
+async def get_project_files(sess, project_id):
+    """Get files within session.
+
+    :param sess: HTTP session in which request call is made
+    :return: list of files
+    """
+    params = {"projectId": project_id}
+    async with sess.get(files_url, params=params) as resp:
+        ans = await resp.json()
+        assert resp.status == 200, f"HTTP Status code error, got {resp.status}: {ans}"
+        return ans
+
+
 async def find_project_file(sess, projectId, fileId):
     """Check if file with the given id is in project files.
 
@@ -686,3 +699,60 @@ async def find_project_file(sess, projectId, fileId):
                 return True
 
         return False
+
+
+async def add_submission_files(sess, file_data, submission_id):
+    """Add files to an existing submission.
+
+    :param sess: HTTP session in which request call is made
+    :param file_data: details of files to add to a submission
+    :param submission_id: id of submission to add files to
+    """
+    url = f"{submissions_url}/{submission_id}/files"
+
+    async with sess.post(
+        url,
+        data=ujson.dumps(file_data),
+    ) as resp:
+        assert resp.status == 204, f"HTTP Status code error, got {resp.status}"
+
+
+async def get_submission(sess, submission_id):
+    """Get submission with the given id.
+
+    :param sess: HTTP session in which request call is made
+    :param submission_id: id of submission to return
+    :returns: submission object
+    """
+    url = f"{submissions_url}/{submission_id}"
+
+    async with sess.get(url) as resp:
+        ans = await resp.json()
+        assert resp.status == 200, f"HTTP Status code error {resp.status}"
+        return ans
+
+
+async def update_submission_files(sess, submission_id, files_data):
+    """Update submission files.
+
+    :param sess: HTTP session in which request call is made
+    :param submission_id: id of submission to update files for
+    :param files_data: list of dict with accessionId
+    """
+    url = f"{submissions_url}/{submission_id}/files"
+
+    async with sess.put(url, data=ujson.dumps(files_data)) as resp:
+        assert resp.status == 204, f"HTTP Status code error {resp.status}"
+
+
+async def remove_submission_file(sess, submission_id, file_id):
+    """Remove file from an existing submission.
+
+    :param sess: HTTP session in which request call is made
+    :param submission_id: id of submission the file of which to remove
+    :param file_id: id of file to remove
+    """
+    url = f"{submissions_url}/{submission_id}/files/{file_id}"
+
+    async with sess.delete(url) as resp:
+        assert resp.status == 204, f"HTTP Status code error {resp.status}"