Repository: fractal-analytics-platform/fractal-client (language: python)

Issue:
In `task_collect_custom`, improve error for missing file
Let's check that the manifest file exists. If not, let's raise an appropriate error that clarifies that this is the client-side manifest file (that is, it will be read by the CLI client, and not on the server).
The current missing-file error may be confusing, when the user provides a path that is actually valid _on the server_.

(cc @mfranzon)
In `task_collect_custom`, improve error for missing file
Let's check that the manifest file exists. If not, let's raise an appropriate error that clarifies that this is the client-side manifest file (that is, it will be read by the CLI client, and not on the server).
The current missing-file error may be confusing, when the user provides a path that is actually valid _on the server_.

(cc @mfranzon)


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index acf09cec5..fc199f236 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 # 2.0.2 (unreleased)
 
+* Improve error handling in `task collect-custom` command (\#680).
 * Documentation
     * Bump `mkdocstrings[python]` to 0.25.2 (\#682).
 
diff --git a/fractal_client/cmd/_task.py b/fractal_client/cmd/_task.py
index ce7979158..6ca0c8cc8 100644
--- a/fractal_client/cmd/_task.py
+++ b/fractal_client/cmd/_task.py
@@ -1,9 +1,7 @@
 import json
 import logging
 import sys
-from typing import Any
 from typing import Optional
-from urllib.request import urlopen
 
 from ..authclient import AuthClient
 from ..config import settings
@@ -75,8 +73,15 @@ def task_collect_custom(
     batch: bool = False,
 ) -> Interface:
 
-    with open(manifest, "r") as f:
-        manifest_dict = json.load(f)
+    try:
+        with open(manifest, "r") as f:
+            manifest_dict = json.load(f)
+    except FileNotFoundError as e:
+        raise FileNotFoundError(
+            f"Fractal Client cannot find the file {manifest}. "
+            "Note that the file must be on the same machine where Fractal "
+            f"Client is running.\nOriginal error: {e}."
+        )
 
     task_collect = dict(
         source=source,
@@ -94,7 +99,10 @@ def task_collect_custom(
         f"{settings.BASE_URL}/task/collect/custom/", json=task_collect
     )
 
-    task_list = check_response(res, expected_status_code=201)
+    task_list = check_response(
+        res, expected_status_code=201, redact_long_payload=True
+    )
+
     if batch:
         task_ids = [str(task["id"]) for task in task_list]
         return Interface(retcode=0, data=" ".join(task_ids))
diff --git a/fractal_client/response.py b/fractal_client/response.py
index 5171f1789..10619b5b9 100644
--- a/fractal_client/response.py
+++ b/fractal_client/response.py
@@ -1,3 +1,4 @@
+import json
 import logging
 import sys
 from json.decoder import JSONDecodeError
@@ -6,10 +7,13 @@
 
 from httpx import Response
 
+LONG_PAYLOAD_VALUE_LIMIT = 300
+
 
 def check_response(
     res: Response,
     expected_status_code: Union[int, list[int]] = 200,
+    redact_long_payload: bool = False,
 ) -> Union[list[Any], dict[str, Any], str, int, float, bool]:
     """
     Check the validity of the http response from fractal server
@@ -22,6 +26,8 @@ def check_response(
     Args:
         res: Response from `fractal-server`.
         expected_status_code: Expected status code(s).
+        redact_long_payload: If `True`, redact payload values of more than
+            `LONG_PAYLOAD_VALUE_LIMIT` characters.
 
     Returns:
         The output of `res.json()`.
@@ -45,9 +51,16 @@ def check_response(
         logging.error(
             f"Original request: {res._request.method} {res._request.url}"
         )
-        logging.error(
-            f"Original payload: {res._request._content.decode('utf-8')}"
-        )
+
+        payload = res._request._content.decode("utf-8")
+        if redact_long_payload and len(payload) > 0:
+            payload_dict = json.loads(payload)
+            for key, value in payload_dict.items():
+                if len(str(value)) > LONG_PAYLOAD_VALUE_LIMIT:
+                    payload_dict[key] = "[value too long - redacted]"
+            payload = json.dumps(payload_dict)
+
+        logging.error(f"Original payload: {payload}")
 
         error_msg = data
 


Test patch:
diff --git a/tests/test_task.py b/tests/test_task.py
index 34e7e7263..695edb304 100644
--- a/tests/test_task.py
+++ b/tests/test_task.py
@@ -442,7 +442,7 @@ def test_task_delete(
     assert len(task_list) == 0
 
 
-def test_task_collection_custom(register_user, tmp_path, invoke):
+def test_task_collection_custom(register_user, tmp_path, invoke, caplog):
     python_interpreter = sys.executable
     package_name = "fractal-client"
     manifest = str(tmp_path / "manifest.json")
@@ -466,6 +466,22 @@ def test_task_collection_custom(register_user, tmp_path, invoke):
     assert res.retcode == 0
     assert isinstance(res.data, list)
 
+    # Second API call fails (tasks with the same sources already exist)
+    caplog.clear()
+    with pytest.raises(SystemExit):
+        res = invoke(cmd)
+    # Manifest was redacted, when logging the payload
+    assert '"manifest": "[value too long - redacted]"' in caplog.text
+
+    # Missing manifest file
+    cmd = (
+        f"task collect-custom --package-name {package_name} "
+        f"source {python_interpreter} /foo/bar"
+    )
+    res = invoke(cmd)
+    assert res.retcode == 1
+    assert "file must be on the same machine" in res.data
+
     cmd = (
         "--batch task collect-custom --package-root /tmp --version 2 "
         f"source2 {python_interpreter} {manifest}"