Unify how the GalaxyInteractorApi handles JSON requests

lib/galaxy/tool_util/verify/interactor.py

@@ -678,33 +678,89 @@ def api_key_header(self, key, admin, anon, headers):
         return header
 
     def _post(self, path, data=None, files=None, key=None, headers=None, admin=False, anon=False, json=False):
-        # If json=True, use post payload using request's json parameter instead of the data
-        # parameter (i.e. assume the contents is a jsonified blob instead of form parameters
-        # with individual parameters jsonified if needed).
         headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers)
-        url = f"{self.api_url}/{path}"
-        return galaxy_requests_post(url, data=data, files=files, as_json=json, headers=headers)
+        url = self.get_api_url(path)
+        kwd = self._prepare_request_params(data=data, files=files, as_json=json, headers=headers)
+        return requests.post(url, **kwd)
 
-    def _delete(self, path, data=None, key=None, headers=None, admin=False, anon=False):
+    def _delete(self, path, data=None, key=None, headers=None, admin=False, anon=False, json=False):
         headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers)
-        return requests.delete(f"{self.api_url}/{path}", params=data, headers=headers)
+        url = self.get_api_url(path)
+        kwd = self._prepare_request_params(data=data, as_json=json, headers=headers)
+        return requests.delete(url, **kwd)
 
-    def _patch(self, path, data=None, key=None, headers=None, admin=False, anon=False):
+    def _patch(self, path, data=None, key=None, headers=None, admin=False, anon=False, json=False):
         headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers)
-        return requests.patch(f"{self.api_url}/{path}", data=data, headers=headers)
+        url = self.get_api_url(path)
+        kwd = self._prepare_request_params(data=data, as_json=json, headers=headers)
+        return requests.patch(url, **kwd)
 
-    def _put(self, path, data=None, key=None, headers=None, admin=False, anon=False):
+    def _put(self, path, data=None, key=None, headers=None, admin=False, anon=False, json=False):
         headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers)
-        return requests.put(f"{self.api_url}/{path}", data=data, headers=headers)
+        url = self.get_api_url(path)
+        kwd = self._prepare_request_params(data=data, as_json=json, headers=headers)
+        return requests.put(url, **kwd)
 
     def _get(self, path, data=None, key=None, headers=None, admin=False, anon=False):
         headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers)
-        if path.startswith("/api"):
-            path = path[len("/api"):]
-        url = f"{self.api_url}/{path}"
+        url = self.get_api_url(path)
         # no data for GET
         return requests.get(url, params=data, headers=headers)
 
+    def get_api_url(self, path: str) -> str:
+        if path.startswith("http"):
+            return path
+        elif path.startswith("/api"):
+            path = path[len("/api"):]
+        return f"{self.api_url}/{path}"
+
+    def _prepare_request_params(self, data=None, files=None, as_json: bool = False, params: dict = None, headers: dict = None):
+        """Handle some Galaxy conventions and work around requests issues.
+
+        This is admittedly kind of hacky, so the interface may change frequently - be
+        careful on reuse.
+
+        If ``as_json`` is True, use post payload using request's json parameter instead
+        of the data parameter (i.e. assume the contents is a json-ified blob instead of
+        form parameters with individual parameters json-ified if needed). requests doesn't
+        allow files to be specified with the json parameter - so rewrite the parameters
+        to handle that if as_json is True with specified files.
+        """
+        params = params or {}
+        data = data or {}
+
+        # handle encoded files
+        if files is None:
+            # if not explicitly passed, check __files... convention used in tool testing
+            # and API testing code
+            files = data.get("__files", None)
+            if files is not None:
+                del data["__files"]
+
+        # files doesn't really work with json, so dump the parameters
+        # and do a normal POST with request's data parameter.
+        if bool(files) and as_json:
+            as_json = False
+            new_items = {}
+            for key, val in data.items():
+                if isinstance(val, dict) or isinstance(val, list):
+                    new_items[key] = dumps(val)
+            data.update(new_items)
+
+        kwd = {
+            'files': files,
+        }
+        if headers:
+            kwd['headers'] = headers
+        if as_json:
+            kwd['json'] = data or None
+            kwd['params'] = params
+        else:
+            data.update(params)
+            kwd['data'] = data
+
+        return kwd
+
 
 def ensure_tool_run_response_okay(submit_response_object, request_desc, inputs=None):
     if submit_response_object.status_code != 200:
@@ -1349,51 +1405,3 @@ def test_data_iter(required_files):
                 raise Exception(f"edit_attributes type ({edit_att.get('type', None)}) is unimplemented")
 
         yield data_dict
-
-
-def galaxy_requests_post(url, data=None, files=None, as_json=False, params=None, headers=None):
-    """Handle some Galaxy conventions and work around requests issues.
-
-    This is admittedly kind of hacky, so the interface may change frequently - be
-    careful on reuse.
-
-    If ``as_json`` is True, use post payload using request's json parameter instead
-    of the data parameter (i.e. assume the contents is a json-ified blob instead of
-    form parameters with individual parameters json-ified if needed). requests doesn't
-    allow files to be specified with the json parameter - so rewrite the parameters
-    to handle that if as_json is True with specified files.
-    """
-    params = params or {}
-    data = data or {}
-
-    # handle encoded files
-    if files is None:
-        # if not explicitly passed, check __files... convention used in tool testing
-        # and API testing code
-        files = data.get("__files", None)
-        if files is not None:
-            del data["__files"]
-
-    # files doesn't really work with json, so dump the parameters
-    # and do a normal POST with request's data parameter.
-    if bool(files) and as_json:
-        as_json = False
-        new_items = {}
-        for key, val in data.items():
-            if isinstance(val, dict) or isinstance(val, list):
-                new_items[key] = dumps(val)
-        data.update(new_items)
-
-    kwd = {
-        'files': files,
-    }
-    if headers:
-        kwd['headers'] = headers
-    if as_json:
-        kwd['json'] = data
-        kwd['params'] = params
-    else:
-        data.update(params)
-        kwd['data'] = data
-
-    return requests.post(url, **kwd)

lib/galaxy_test/api/test_datasets.py

@@ -1,4 +1,3 @@
-import json
 import textwrap
 
 from galaxy_test.base.populators import (
@@ -123,15 +122,13 @@ def test_display(self):
 
     def test_tag_change(self):
         hda_id = self.dataset_populator.new_dataset(self.history_id)['id']
-        payload = json.dumps({
+        payload = {
             'item_id': hda_id,
             'item_class': 'HistoryDatasetAssociation',
             'item_tags': ['cool:tag_a', 'cool:tag_b', 'tag_c', 'name:tag_d', '#tag_e'],
-        })
-
-        # TODO remove the headers here and add json parameter to _put method
-        put_response = self._put("tags", data=payload, headers={'Content-Type': 'application/json'})
+        }
 
+        put_response = self._put("tags", data=payload, json=True)
         self._assert_status_code_is_ok(put_response)
         updated_hda = self._get(
             f"histories/{self.history_id}/contents/{hda_id}").json()

lib/galaxy_test/api/test_groups.py

@@ -1,5 +1,3 @@
-import json
-
 from galaxy_test.base.populators import DatasetPopulator
 from ._framework import ApiTestCase
 
@@ -75,10 +73,10 @@ def test_update(self):
 
         group_id = group["id"]
         updated_name = "group-test-updated"
-        update_payload = json.dumps({
+        update_payload = {
             "name": updated_name,
-        })
-        update_response = self._put(f"groups/{group_id}", data=update_payload, admin=True)
+        }
+        update_response = self._put(f"groups/{group_id}", data=update_payload, admin=True, json=True)
         self._assert_status_code_is_ok(update_response)
 
     def test_update_only_admin(self):
@@ -94,10 +92,10 @@ def test_update_duplicating_name_raises_409(self):
         # Update group_b with the same name as group_a
         group_b_id = group_b["id"]
         updated_name = group_a["name"]
-        update_payload = json.dumps({
+        update_payload = {
             "name": updated_name,
-        })
-        update_response = self._put(f"groups/{group_b_id}", data=update_payload, admin=True)
+        }
+        update_response = self._put(f"groups/{group_b_id}", data=update_payload, admin=True, json=True)
         self._assert_status_code_is(update_response, 409)
 
     def _assert_valid_group(self, group, assert_id=None):

lib/galaxy_test/api/test_jobs.py

@@ -523,7 +523,7 @@ def test_resume_job(self, history_id):
         assert dataset_details['state'] == 'paused'
         # Undelete input dataset
         undelete_response = self._put(f"histories/{history_id}/contents/{hda1['id']}",
-                                      data=json.dumps({'deleted': False}))
+                                      data={'deleted': False}, json=True)
         self._assert_status_code_is(undelete_response, 200)
         resume_response = self._put(f"jobs/{job_id}/resume")
         self._assert_status_code_is(resume_response, 200)
@@ -574,7 +574,7 @@ def test_search_handle_identifiers(self, history_id):
         self._job_search(tool_id='identifier_single', history_id=history_id, inputs=inputs)
         dataset_details = self._get(f"histories/{history_id}/contents/{dataset_id}").json()
         dataset_details['name'] = 'Renamed Test Dataset'
-        dataset_update_response = self._put(f"histories/{history_id}/contents/{dataset_id}", data=dict(name='Renamed Test Dataset'))
+        dataset_update_response = self._put(f"histories/{history_id}/contents/{dataset_id}", data=dict(name='Renamed Test Dataset'), json=True)
         self._assert_status_code_is(dataset_update_response, 200)
         assert dataset_update_response.json()['name'] == 'Renamed Test Dataset'
         search_payload = self._search_payload(history_id=history_id, tool_id='identifier_single', inputs=inputs)

lib/galaxy_test/api/test_libraries.py

@@ -38,7 +38,7 @@ def test_delete(self):
         assert library["deleted"] is True
         # Test undeleting
         data = dict(undelete=True)
-        create_response = self._delete(f"libraries/{library['id']}", data=data, admin=True)
+        create_response = self._delete(f"libraries/{library['id']}", data=data, admin=True, json=True)
         library = create_response.json()
         self._assert_status_code_is(create_response, 200)
         assert library["deleted"] is False

lib/galaxy_test/api/test_workflows.py

@@ -4328,9 +4328,9 @@ def test_workflow_publishing(self):
         workflow_id = self.workflow_populator.simple_workflow("dummy")
         response = self._show_workflow(workflow_id)
         assert not response['published']
-        published_worklow = self._put(f'workflows/{workflow_id}', data=json.dumps({'published': True})).json()
+        published_worklow = self._put(f'workflows/{workflow_id}', data={'published': True}, json=True).json()
         assert published_worklow['published']
-        unpublished_worklow = self._put(f'workflows/{workflow_id}', data=json.dumps({'published': False})).json()
+        unpublished_worklow = self._put(f'workflows/{workflow_id}', data={'published': False}, json=True).json()
         assert not unpublished_worklow['published']
 
     def _invoke_paused_workflow(self, history_id):

lib/galaxy_test/base/populators.py

@@ -216,15 +216,15 @@ def _post(self, route, data=None, files=None, headers=None, admin=False, json: b
         """POST data to target Galaxy instance on specified route."""
 
     @abstractmethod
-    def _put(self, route, data=None, headers=None, admin=False) -> Response:
+    def _put(self, route, data=None, headers=None, admin=False, json: bool = False) -> Response:
         """PUT data to target Galaxy instance on specified route."""
 
     @abstractmethod
     def _get(self, route, data=None, headers=None, admin=False) -> Response:
         """GET data from target Galaxy instance on specified route."""
 
     @abstractmethod
-    def _delete(self, route, data=None, headers=None, admin=False) -> Response:
+    def _delete(self, route, data=None, headers=None, admin=False, json: bool = False) -> Response:
         """DELETE against target Galaxy instance on specified route."""
 
 
@@ -628,11 +628,11 @@ def make_private(self, history_id: str, dataset_id: str) -> dict:
         role_id = self.user_private_role_id()
         # Give manage permission to the user.
         payload = {
-            "access": json.dumps([role_id]),
-            "manage": json.dumps([role_id]),
+            "access": [role_id],
+            "manage": [role_id],
         }
         url = f"histories/{history_id}/contents/{dataset_id}/permissions"
-        update_response = self._put(url, payload, admin=True)
+        update_response = self._put(url, payload, admin=True, json=True)
         assert update_response.status_code == 200, update_response.content
         return update_response.json()
 
@@ -669,7 +669,7 @@ def setup_history_for_export_testing(self, history_name):
 
     def prepare_export(self, history_id, data):
         url = f"histories/{history_id}/exports"
-        put_response = self._put(url, data)
+        put_response = self._put(url, data, json=True)
         put_response.raise_for_status()
 
         if put_response.status_code == 202:
@@ -732,7 +732,7 @@ def has_history_with_name():
 
     def rename_history(self, history_id, new_name):
         update_url = f"histories/{history_id}"
-        put_response = self._put(update_url, {"name": new_name})
+        put_response = self._put(update_url, {"name": new_name}, json=True)
         return put_response
 
     def get_histories(self):
@@ -792,20 +792,20 @@ def _api_key(self):
     def _post(self, route, data=None, files=None, headers=None, admin=False, json: bool = False) -> Response:
         return self.galaxy_interactor.post(route, data, files=files, admin=admin, headers=headers, json=json)
 
-    def _put(self, route, data=None, headers=None, admin=False):
-        return self.galaxy_interactor.put(route, data, headers=headers, admin=admin)
+    def _put(self, route, data=None, headers=None, admin=False, json: bool = False):
+        return self.galaxy_interactor.put(route, data, headers=headers, admin=admin, json=json)
 
     def _get(self, route, data=None, headers=None, admin=False):
         if data is None:
             data = {}
 
         return self.galaxy_interactor.get(route, data=data, headers=headers, admin=admin)
 
-    def _delete(self, route, data=None, headers=None, admin=False):
+    def _delete(self, route, data=None, headers=None, admin=False, json: bool = False):
         if data is None:
             data = {}
 
-        return self.galaxy_interactor.delete(route, data=data, headers=headers, admin=admin)
+        return self.galaxy_interactor.delete(route, data=data, headers=headers, admin=admin, json=json)
 
 
 class DatasetPopulator(GalaxyInteractorHttpMixin, BaseDatasetPopulator):
@@ -978,7 +978,7 @@ def update_workflow(self, workflow_id: str, workflow_object: dict) -> Response:
             workflow=workflow_object
         )
         raw_url = f'workflows/{workflow_id}'
-        put_response = self._put(raw_url, json.dumps(data))
+        put_response = self._put(raw_url, data, json=True)
         return put_response
 
     def refactor_workflow(self, workflow_id: str, actions: list, dry_run: Optional[bool] = None, style: Optional[str] = None) -> Response:
@@ -990,7 +990,7 @@ def refactor_workflow(self, workflow_id: str, actions: list, dry_run: Optional[b
         if dry_run is not None:
             data["dry_run"] = dry_run
         raw_url = f'workflows/{workflow_id}/refactor'
-        put_response = self._put(raw_url, json.dumps(data))
+        put_response = self._put(raw_url, data, json=True)
         return put_response
 
     @contextlib.contextmanager
@@ -1772,14 +1772,14 @@ def _post(self, route, data=None, files=None, headers=None, admin=False, json: b
         data['key'] = self._gi.key
         return requests.post(self._url(route), data=data, headers=headers)
 
-    def _put(self, route, data=None, headers=None, admin=False):
+    def _put(self, route, data=None, headers=None, admin=False, json: bool = False):
         if data is None:
             data = {}
         data = data.copy()
         data['key'] = self._gi.key
         return requests.put(self._url(route), data=data, headers=headers)
 
-    def _delete(self, route, data=None, headers=None):
+    def _delete(self, route, data=None, headers=None, admin=False, json: bool = False):
         if data is None:
             data = {}
         data = data.copy()

lib/galaxy_test/selenium/framework.py

@@ -572,7 +572,7 @@ def _post(self, route, data=None, files=None, headers=None, admin=False, json: b
         response = requests.post(full_url, data=data, cookies=cookies, files=files, headers=headers)
         return response
 
-    def _delete(self, route, data=None, headers=None, admin=False) -> Response:
+    def _delete(self, route, data=None, headers=None, admin=False, json: bool = False) -> Response:
         data = data or {}
         full_url = self.selenium_context.build_url(f"api/{route}", for_selenium=False)
         cookies = None
@@ -583,7 +583,7 @@ def _delete(self, route, data=None, headers=None, admin=False) -> Response:
         response = requests.delete(full_url, data=data, cookies=cookies, headers=headers)
         return response
 
-    def _put(self, route, data=None, headers=None, admin=False) -> Response:
+    def _put(self, route, data=None, headers=None, admin=False, json: bool = False) -> Response:
         data = data or {}
         full_url = self.selenium_context.build_url(f"api/{route}", for_selenium=False)
         cookies = None