From e1dfcc036dcafd6ec7cd9df5e37218c0a94f0fbe Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 11:15:19 +0200 Subject: [PATCH 1/7] fix handling of error messages --- mergin/client.py | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/mergin/client.py b/mergin/client.py index 9e70a93c..fc940907 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -228,25 +228,23 @@ def _do_request(self, request): try: return self.opener.open(request) except urllib.error.HTTPError as e: - server_response = json.load(e) - - err_detail = None - server_code = None - # Try to get error detail - if isinstance(server_response, dict): - server_code = server_response.get("code") - err_detail = server_response.get("detail") - if not err_detail: - # Extract all field-specific errors and format them - err_detail = "\n".join( - f"{key}: {', '.join(map(str, value))}" - for key, value in server_response.items() - if isinstance(value, list) - ) or str( - server_response - ) # Fallback to raw response if structure is unexpected - else: - err_detail = str(server_response) + + server_code = e.code + err_detail = "Unknown error" + server_response = None + + if e.fp: + server_response = e.fp.read().decode("utf-8") + if ( + e.headers.get("Content-Type", "") == "application/problem+json" + or e.headers.get("Content-Type", "") == "application/json" + ): + json_response = json.loads(server_response) + err_detail = json_response.get("detail", None) # `detail` should be present in MM server response + if err_detail is None: + err_detail = server_response + else: + err_detail = server_response raise ClientError( detail=err_detail, @@ -256,6 +254,7 @@ def _do_request(self, request): http_error=e.code, http_method=request.get_method(), ) + except urllib.error.URLError as e: # e.g. when DNS resolution fails (no internet connection?) raise ClientError("Error requesting " + request.full_url + ": " + str(e)) From 7e0754de78a3c2784755bd9d8ea47017675e88ce Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 11:15:26 +0200 Subject: [PATCH 2/7] add test cases --- mergin/test/test_client.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index f3d4c4b1..e159da64 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -20,6 +20,7 @@ decode_token_data, TokenError, ServerType, + WorkspaceRole, ) from ..client_push import push_project_async, push_project_cancel from ..client_pull import ( @@ -2886,3 +2887,24 @@ def test_mc_without_login(): # without login should not be able to access workspaces with pytest.raises(ClientError, match="Authentication information is missing or invalid."): mc.workspaces_list() + + +def test_do_request_error_handling(mc: MerginClient): + + try: + mc.get("/v2/sso/connections?email=bad@email.com") + except ClientError as e: + assert e.http_error == 404 + assert ( + e.detail + == "The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again." + ) + assert ": 404," in e.server_response + + workspaces = mc.workspaces_list() + + try: + mc.create_user("test@email.com", "123", workspace_id=workspaces[0]["id"], workspace_role=WorkspaceRole.GUEST) + except ClientError as e: + assert e.http_error == 400 + assert "Passwords must be at least 8 characters long." in e.detail From 8e5b2f77b251a02c529a8683d8425e59cc331324 Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 12:46:47 +0200 Subject: [PATCH 3/7] check if structure is read using json.loads() --- mergin/client.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mergin/client.py b/mergin/client.py index fc940907..085c3e83 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -240,7 +240,10 @@ def _do_request(self, request): or e.headers.get("Content-Type", "") == "application/json" ): json_response = json.loads(server_response) - err_detail = json_response.get("detail", None) # `detail` should be present in MM server response + if isinstance(json_response, dict): + err_detail = json_response.get( + "detail", None + ) # `detail` should be present in MM server response if err_detail is None: err_detail = server_response else: From 63d9fc612cce6bd1b784d2b5288b1189a4f3dcb8 Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 13:25:25 +0200 Subject: [PATCH 4/7] default is none --- mergin/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mergin/client.py b/mergin/client.py index 085c3e83..b6308db5 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -230,7 +230,7 @@ def _do_request(self, request): except urllib.error.HTTPError as e: server_code = e.code - err_detail = "Unknown error" + err_detail = None server_response = None if e.fp: From 8abe7c4627c399d5b977cca844368618341b90d2 Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 13:25:38 +0200 Subject: [PATCH 5/7] update error handling in tests --- mergin/test/test_client.py | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index e159da64..b5de8a6e 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -2885,26 +2885,29 @@ def test_mc_without_login(): assert config["server_configured"] # without login should not be able to access workspaces - with pytest.raises(ClientError, match="Authentication information is missing or invalid."): + with pytest.raises(ClientError) as e: mc.workspaces_list() + assert e.value.http_error == 401 + assert e.value.detail == '"Authentication information is missing or invalid."\n' + def test_do_request_error_handling(mc: MerginClient): - try: + with pytest.raises(ClientError) as e: mc.get("/v2/sso/connections?email=bad@email.com") - except ClientError as e: - assert e.http_error == 404 - assert ( - e.detail - == "The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again." - ) - assert ": 404," in e.server_response + + assert e.value.http_error == 404 + assert ( + e.value.detail + == "The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again." + ) + assert ": 404," in e.value.server_response workspaces = mc.workspaces_list() - try: + with pytest.raises(ClientError) as e: mc.create_user("test@email.com", "123", workspace_id=workspaces[0]["id"], workspace_role=WorkspaceRole.GUEST) - except ClientError as e: - assert e.http_error == 400 - assert "Passwords must be at least 8 characters long." in e.detail + + assert e.value.http_error == 400 + assert "Passwords must be at least 8 characters long." in e.value.detail From bbfa7964baa27960147552d99f34de398f74b724 Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 13:53:59 +0200 Subject: [PATCH 6/7] pass directly --- mergin/client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mergin/client.py b/mergin/client.py index b6308db5..008c7f73 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -229,7 +229,6 @@ def _do_request(self, request): return self.opener.open(request) except urllib.error.HTTPError as e: - server_code = e.code err_detail = None server_response = None @@ -252,7 +251,7 @@ def _do_request(self, request): raise ClientError( detail=err_detail, url=request.get_full_url(), - server_code=server_code, + server_code=e.code, server_response=server_response, http_error=e.code, http_method=request.get_method(), From 4be1454ef97931fbf732cefaed8455fd66b8e8e8 Mon Sep 17 00:00:00 2001 From: Jan Caha Date: Thu, 3 Jul 2025 14:23:05 +0200 Subject: [PATCH 7/7] pass server_code --- mergin/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mergin/client.py b/mergin/client.py index 008c7f73..6456c1bd 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -231,6 +231,7 @@ def _do_request(self, request): err_detail = None server_response = None + server_code = None if e.fp: server_response = e.fp.read().decode("utf-8") @@ -243,6 +244,7 @@ def _do_request(self, request): err_detail = json_response.get( "detail", None ) # `detail` should be present in MM server response + server_code = json_response.get("code", None) if err_detail is None: err_detail = server_response else: @@ -251,7 +253,7 @@ def _do_request(self, request): raise ClientError( detail=err_detail, url=request.get_full_url(), - server_code=e.code, + server_code=server_code, server_response=server_response, http_error=e.code, http_method=request.get_method(),