Make API endpoints set_user_props and delete_user_props as JSON PUT r…

…equests.

clockwork_tools/clockwork_tools/client.py

@@ -60,7 +60,7 @@ def _get_headers(self):
         encoded_s = str(encoded_bytes, "utf-8")
         return {"Authorization": f"Basic {encoded_s}"}
 
-    def _request(self, endpoint, params, method="GET"):
+    def _request(self, endpoint, params, method="GET", send_json=False):
         """Helper method for REST API calls.
 
         Internal helper method to make the calls to the REST API endpoints
@@ -69,6 +69,9 @@ def _request(self, endpoint, params, method="GET"):
         Args:
             endpoint (str): The REST endpoint, omitting the server address.
             params (dict): Arguments to be provided to the REST endpoint.
+            send_json (bool): Optional. If True and if method is PUT,
+                then request will be sent as a JSON request.
+
 
         Returns:
             Depends on the call made.
@@ -86,9 +89,14 @@ def _request(self, endpoint, params, method="GET"):
                 complete_address, params=params, headers=self._get_headers()
             )
         elif method == "PUT":
-            response = requests.put(
-                complete_address, data=params, headers=self._get_headers()
-            )
+            if send_json:
+                headers = self._get_headers()
+                headers["Content-type"] = "application/json"
+                response = requests.put(complete_address, json=params, headers=headers)
+            else:
+                response = requests.put(
+                    complete_address, data=params, headers=self._get_headers()
+                )
 
         # Check code instead and raise exception if it's the wrong one.
         if response.status_code == 200:
@@ -186,14 +194,11 @@ def set_user_props(
             dict[any,any]: Returns the updated props.
         """
         endpoint = "api/v1/clusters/jobs/user_props/set"
-        params = {"job_id": job_id, "cluster_name": cluster_name}
-        # Due to current constraints, we have to pass "updates"
-        # as a string representing a structure in json.
-        params["updates"] = json.dumps(updates)
-        return self._request(endpoint, params, method="PUT")
+        params = {"job_id": job_id, "cluster_name": cluster_name, "updates": updates}
+        return self._request(endpoint, params, method="PUT", send_json=True)
 
     def delete_user_props(
-        self, job_id: str, cluster_name: str, keys: list
+        self, job_id: str, cluster_name: str, keys: str | list
     ) -> dict[str, any]:
         """REST call to api/v1/clusters/jobs/user_props/delete.
 
@@ -208,11 +213,8 @@ def delete_user_props(
             dict[any,any]: Returns the updated props.
         """
         endpoint = "api/v1/clusters/jobs/user_props/delete"
-        params = {"job_id": job_id, "cluster_name": cluster_name}
-        # Due to current constraints, we have to pass "keys"
-        # as a string representing a structure in json.
-        params["keys"] = json.dumps(keys)
-        return self._request(endpoint, params, method="PUT")
+        params = {"job_id": job_id, "cluster_name": cluster_name, "keys": keys}
+        return self._request(endpoint, params, method="PUT", send_json=True)
 
     def jobs_user_dict_update(
         self, job_id: str = None, cluster_name: str = None, update_pairs: dict = {}

clockwork_tools_test/test_mt_job_user_props.py

@@ -32,9 +32,27 @@ def test_cw_tools_set_user_props(client):
 def test_cw_tools_delete_user_props(client):
     job_id = "795002"
     cluster_name = "mila"
-    props = client.set_user_props(job_id, cluster_name, {"a new name": "a new prop"})
-    assert props == {"name": "je suis une user prop 1", "a new name": "a new prop"}
+    props = client.set_user_props(
+        job_id,
+        cluster_name,
+        {"a new name": "a new prop", "aa": "aa", "bb": "bb", "cc": "cc"},
+    )
+    assert props == {
+        "name": "je suis une user prop 1",
+        "a new name": "a new prop",
+        "aa": "aa",
+        "bb": "bb",
+        "cc": "cc",
+    }
+
+    assert client.delete_user_props(job_id, cluster_name, "name") == ""
+    props = client.get_user_props(job_id, cluster_name)
+    assert props == {"a new name": "a new prop", "aa": "aa", "bb": "bb", "cc": "cc"}
+
+    assert client.delete_user_props(job_id, cluster_name, ["aa"]) == ""
+    props = client.get_user_props(job_id, cluster_name)
+    assert props == {"a new name": "a new prop", "bb": "bb", "cc": "cc"}
 
-    assert client.delete_user_props(job_id, cluster_name, ["name"]) == ""
+    assert client.delete_user_props(job_id, cluster_name, ["bb", "cc"]) == ""
     props = client.get_user_props(job_id, cluster_name)
     assert props == {"a new name": "a new prop"}

clockwork_web/rest_routes/jobs.py

@@ -167,7 +167,7 @@ def route_user_props_set():
     """
     Endpoint to set user props
 
-    Parameters: job_id (str), cluster_name (str), updates (JSON-string of a dict)
+    Parameters: job_id (str), cluster_name (str), updates (dict)
 
     Return: updated user props
     """
@@ -176,32 +176,25 @@ def route_user_props_set():
         f"clockwork REST route: /jobs/user_props/set - current_user_with_rest_auth={current_user_id}"
     )
 
+    if not request.is_json:
+        return jsonify("Expected a JSON request"), 400  # bad request
+
+    params = request.get_json()
+
     # Retrieve the requested job ID
-    job_id = request.values.get("job_id", None)
+    job_id = params.get("job_id", None)
     if job_id is None:
         return jsonify("Missing argument job_id."), 400  # bad request
 
     # Retrieve the requested cluster name
-    cluster_name = request.values.get("cluster_name", None)
+    cluster_name = params.get("cluster_name", None)
     if cluster_name is None:
         return jsonify("Missing argument cluster_name."), 400  # bad request
 
     # Retrieve the requested updates.
-    updates = request.values.get("updates", None)
+    updates = params.get("updates", None)
     if updates is None:
         return jsonify(f"Missing argument 'updates'."), 500
-    elif isinstance(updates, str):
-        try:
-            updates = json.loads(updates)
-        except Exception:
-            return jsonify("Failed to get `updates` dict."), 500
-    else:
-        return (
-            jsonify(
-                f"Field 'updates' was not a string (encoding a json structure). {updates}"
-            ),
-            500,
-        )
     if not isinstance(updates, dict):
         return (
             jsonify(
@@ -225,7 +218,7 @@ def route_user_props_delete():
     """
     Endpoint to delete user props.
 
-    Parameters: job_id (str), cluster_name (str), keys (JSON-string of a list)
+    Parameters: job_id (str), cluster_name (str), keys (string or list of strings)
 
     Return: updated user props
     """
@@ -234,36 +227,29 @@ def route_user_props_delete():
         f"clockwork REST route: /jobs/user_props/delete - current_user_with_rest_auth={current_user_id}"
     )
 
+    if not request.is_json:
+        return jsonify("Expected a JSON request"), 400  # bad request
+
+    params = request.get_json()
+
     # Retrieve the requested job ID
-    job_id = request.values.get("job_id", None)
+    job_id = params.get("job_id", None)
     if job_id is None:
         return jsonify("Missing argument job_id."), 400  # bad request
 
     # Retrieve the requested cluster name
-    cluster_name = request.values.get("cluster_name", None)
+    cluster_name = params.get("cluster_name", None)
     if cluster_name is None:
         return jsonify("Missing argument cluster_name."), 400  # bad request
 
     # Retrieve the requested keys.
-    keys = request.values.get("keys", None)
+    keys = params.get("keys", None)
     if keys is None:
         return jsonify(f"Missing argument 'keys'."), 500
-    elif isinstance(keys, str):
-        try:
-            keys = json.loads(keys)
-        except Exception:
-            return jsonify("Failed to get `keys` dict."), 500
-    else:
-        return (
-            jsonify(
-                f"Field 'keys' was not a string (encoding a json structure). {keys}"
-            ),
-            500,
-        )
-    if not isinstance(keys, list):
+    if not isinstance(keys, (str, list)):
         return (
             jsonify(
-                f"Expected `keys` to be a list, instead it is of type {type(keys)}: {keys}."
+                f"Expected `keys` to be a string or list of strings, instead it is of type {type(keys)}: {keys}."
             ),
             500,
         )