Fix extra DB read during Metax data update

Change operators functions responsible for metadata object replace to return full object data.

metadata_backend/api/handlers/object.py

@@ -285,8 +285,8 @@ async def put_object(self, req: Request) -> Response:
                 LOG.error(reason)
                 raise web.HTTPUnauthorized(reason=reason)
 
-        accession_id, title = await operator.replace_metadata_object(collection, accession_id, content)
-        patch = self.prepare_folder_patch_update_object(collection, accession_id, title, filename)
+        data = await operator.replace_metadata_object(collection, accession_id, content)
+        patch = self.prepare_folder_patch_update_object(collection, data, filename)
         await folder_op.update_folder(folder_id, patch)
 
         # Update draft dataset to Metax catalog
@@ -336,8 +336,8 @@ async def patch_object(self, req: Request) -> Response:
 
         # If there's changed title it will be updated to folder
         try:
-            title = content["descriptor"]["studyTitle"] if collection == "study" else content["title"]
-            patch = self.prepare_folder_patch_update_object(collection, accession_id, title)
+            _ = content["descriptor"]["studyTitle"] if collection == "study" else content["title"]
+            patch = self.prepare_folder_patch_update_object(collection, content)
             await folder_op.update_folder(folder_id, patch)
         except (TypeError, KeyError):
             pass
@@ -393,9 +393,7 @@ def prepare_folder_patch_new_object(self, schema: str, objects: List, params: Di
             patch.append(patch_ops)
         return patch
 
-    def prepare_folder_patch_update_object(
-        self, schema: str, accession_id: str, title: str, filename: str = ""
-    ) -> List:
+    def prepare_folder_patch_update_object(self, schema: str, data: Dict, filename: str = "") -> List:
         """Prepare patch operation for updating object's title in a folder.
 
         :param schema: schema of object to be updated
@@ -410,8 +408,13 @@ def prepare_folder_patch_update_object(
 
         patch_op = {
             "op": "replace",
-            "match": {path.replace("/", ""): {"$elemMatch": {"schema": schema, "accessionId": accession_id}}},
+            "match": {path.replace("/", ""): {"$elemMatch": {"schema": schema, "accessionId": data["accessionId"]}}},
         }
+        try:
+            title = data["descriptor"]["studyTitle"] if schema in ["study", "draft-study"] else data["title"]
+        except (TypeError, KeyError):
+            title = ""
+
         if not filename:
             patch_op.update(
                 {

metadata_backend/api/operators.py

@@ -55,9 +55,7 @@ async def create_metadata_object(self, schema_type: str, data: Union[Dict, str])
         )
         return data
 
-    async def replace_metadata_object(
-        self, schema_type: str, accession_id: str, data: Union[Dict, str]
-    ) -> Tuple[str, str]:
+    async def replace_metadata_object(self, schema_type: str, accession_id: str, data: Union[Dict, str]) -> Dict:
         """Replace metadata object from database.
 
         Data formatting and addition step for JSON or XML must be implemented
@@ -68,9 +66,9 @@ async def replace_metadata_object(
         :param data: Data to be saved to database.
         :returns: Accession id for the object replaced to database
         """
-        accession_id, title = await self._format_data_to_replace_and_add_to_db(schema_type, accession_id, data)
+        data = await self._format_data_to_replace_and_add_to_db(schema_type, accession_id, data)
         LOG.info(f"Replacing object with schema {schema_type} to database succeeded with accession id: {accession_id}")
-        return accession_id, title
+        return data
 
     async def update_metadata_object(self, schema_type: str, accession_id: str, data: Union[Dict, str]) -> str:
         """Update metadata object from database.
@@ -152,7 +150,7 @@ async def _insert_formatted_object_to_db(self, schema_type: str, data: Dict) ->
             raise web.HTTPBadRequest(reason=reason)
         return True
 
-    async def _replace_object_from_db(self, schema_type: str, accession_id: str, data: Dict) -> Tuple[str, str]:
+    async def _replace_object_from_db(self, schema_type: str, accession_id: str, data: Dict) -> bool:
         """Replace formatted metadata object in database.
 
         :param schema_type: Schema type of the object to replace.
@@ -172,16 +170,11 @@ async def _replace_object_from_db(self, schema_type: str, accession_id: str, dat
             reason = f"Error happened while getting object: {error}"
             LOG.error(reason)
             raise web.HTTPBadRequest(reason=reason)
-        if replace_success:
-            try:
-                title = data["descriptor"]["studyTitle"] if schema_type in ["study", "draft-study"] else data["title"]
-            except (TypeError, KeyError):
-                title = ""
-            return accession_id, title
-        else:
+        if not replace_success:
             reason = "Replacing object to database failed for some reason."
             LOG.error(reason)
             raise web.HTTPBadRequest(reason=reason)
+        return True
 
     async def _update_object_from_db(self, schema_type: str, accession_id: str, data: Dict) -> str:
         """Update formatted metadata object in database.
@@ -264,9 +257,7 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: Any
         """
 
     @abstractmethod
-    async def _format_data_to_replace_and_add_to_db(
-        self, schema_type: str, accession_id: str, data: Any
-    ) -> Tuple[str, str]:
+    async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: Any) -> Dict:
         """Format and replace data in database.
 
         Must be implemented by subclass.
@@ -410,13 +401,10 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: Dic
         if schema_type == "study":
             data["publishDate"] = datetime.utcnow() + relativedelta(months=2)
         LOG.debug(f"Operator formatted data for {schema_type} to add to DB.")
-        # return await self._insert_formatted_object_to_db(schema_type, data), data
         await self._insert_formatted_object_to_db(schema_type, data)
         return data
 
-    async def _format_data_to_replace_and_add_to_db(
-        self, schema_type: str, accession_id: str, data: Dict
-    ) -> Tuple[str, str]:
+    async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: Dict) -> Dict:
         """Format JSON metadata object and replace it in db.
 
         Replace information in object before adding to db.
@@ -462,11 +450,9 @@ async def _format_data_to_update_and_add_to_db(self, schema_type: str, accession
         """
         forbidden_keys = ["accessionId", "publishDate", "dateCreated"]
         # check if object already has metax id or is it first time writing it
-        if schema_type in {"study", "dataset"}:  # and data.get("metaxIdentifier", None):
+        if schema_type in {"study", "dataset"}:
             read_data = await self.db_service.read(schema_type, accession_id)
-            # on firs write db doesnt have yet metaxIdentifier and
-            # on publish metax status inside metaxIdentifier is changed
-            # so we are checking that metax id is still the same
+            # on firs write db doesnt have yet metaxIdentifier
             if read_data.get("metaxIdentifier", None):
                 forbidden_keys.extend(["metaxIdentifier"])
         if any(i in data for i in forbidden_keys):
@@ -570,9 +556,7 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str
 
         return data_with_id
 
-    async def _format_data_to_replace_and_add_to_db(
-        self, schema_type: str, accession_id: str, data: str
-    ) -> Tuple[str, str]:
+    async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> Dict:
         """Format XML metadata object and add it to db.
 
         XML is validated, then parsed to JSON, which is added to database.
@@ -587,13 +571,14 @@ async def _format_data_to_replace_and_add_to_db(
         # remove `draft-` from schema type
         schema = schema_type[6:] if schema_type.startswith("draft") else schema_type
         data_as_json = XMLToJSONParser().parse(schema, data)
-        accession_id, title = await Operator(db_client)._format_data_to_replace_and_add_to_db(
+        data_with_id = await Operator(db_client)._format_data_to_replace_and_add_to_db(
             schema_type, accession_id, data_as_json
         )
         LOG.debug(f"XMLOperator formatted data for xml-{schema_type} to add to DB")
-        return await self._replace_object_from_db(
+        await self._replace_object_from_db(
             f"xml-{schema_type}", accession_id, {"accessionId": accession_id, "content": data}
         )
+        return data_with_id
 
     async def _format_data_to_update_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str:
         """Raise not implemented.

tests/test_handlers.py

@@ -160,7 +160,7 @@ async def fake_xmloperator_create_metadata_object(self, schema_type, content):
 
     async def fake_xmloperator_replace_metadata_object(self, schema_type, accession_id, content):
         """Fake replace operation to return mocked accessionId."""
-        return self.test_ega_string, "title"
+        return {"accessionId": self.test_ega_string, "title": "title"}
 
     async def fake_operator_create_metadata_object(self, schema_type, content):
         """Fake create operation to return mocked accessionId."""
@@ -172,7 +172,7 @@ async def fake_operator_update_metadata_object(self, schema_type, accession_id,
 
     async def fake_operator_replace_metadata_object(self, schema_type, accession_id, content):
         """Fake replace operation to return mocked accessionId."""
-        return self.test_ega_string, "title"
+        return {"accessionId": self.test_ega_string, "title": "title"}
 
     async def fake_operator_delete_metadata_object(self, schema_type, accession_id):
         """Fake delete operation to await successful operation indicator."""

tests/test_operators.py

@@ -198,9 +198,9 @@ async def test_json_replace_passes_and_returns_accessionId(self):
         operator = Operator(self.client)
         operator.db_service.exists.return_value = True
         operator.db_service.replace.return_value = True
-        accession, _ = await operator.replace_metadata_object("study", self.accession_id, data)
+        data = await operator.replace_metadata_object("study", self.accession_id, data)
         operator.db_service.replace.assert_called_once()
-        self.assertEqual(accession, self.accession_id)
+        self.assertEqual(data["accessionId"], self.accession_id)
 
     async def test_json_replace_raises_if_not_exists(self):
         """Test replace method raises error."""
@@ -328,7 +328,7 @@ async def test_correct_data_is_set_to_json_when_replacing(self):
                         "metaxIdentifier": {"identifier": 12345},
                     },
                 )
-            self.assertEqual(acc, self.accession_id)
+            self.assertEqual(acc["accessionId"], self.accession_id)
 
     async def test_correct_data_is_set_to_json_when_updating(self):
         """Test operator updates object and adds necessary info."""
@@ -397,7 +397,7 @@ async def test_correct_data_is_set_to_xml_when_replacing(self):
         xml_data = "<MOCK_ELEM></MOCK_ELEM>"
         with patch(
             "metadata_backend.api.operators.Operator._format_data_to_replace_and_add_to_db",
-            return_value=(self.accession_id, "title"),
+            return_value={"accessionId": self.accession_id},
         ):
             with patch(
                 "metadata_backend.api.operators.XMLOperator._replace_object_from_db",
@@ -410,7 +410,7 @@ async def test_correct_data_is_set_to_xml_when_replacing(self):
                         self.accession_id,
                         {"accessionId": self.accession_id, "content": xml_data},
                     )
-                    self.assertEqual(acc, self.accession_id)
+                    self.assertEqual(acc["accessionId"], self.accession_id)
 
     async def test_deleting_metadata_deletes_json_and_xml(self):
         """Test metadata is deleted."""