diff --git a/src/dataworkbench/gateway.py b/src/dataworkbench/gateway.py index 14859a3..3d15098 100644 --- a/src/dataworkbench/gateway.py +++ b/src/dataworkbench/gateway.py @@ -11,9 +11,50 @@ logger = setup_logger(__name__) +def _parse_problem_details(response: requests.Response) -> dict[str, Any] | None: + """Parse a ProblemDetails body, returning None when the body is not a JSON object.""" + try: + body = json.loads(response.text) + except (ValueError, TypeError): + return None + return body if isinstance(body, dict) else None + + def _get_trace_id_from_response(response: requests.Response) -> str | None: - response_dict = json.loads(response.text) - return response_dict.get("traceId") + problem = _parse_problem_details(response) + return problem.get("traceId") if problem else None + + +def _format_validation_errors(errors: dict[str, Any]) -> str: + return "; ".join( + f"{field}: {', '.join(str(m) for m in messages)}" + if isinstance(messages, list) + else f"{field}: {messages}" + for field, messages in errors.items() + ) + + +def _describe_failure(response: requests.Response | None) -> str: + """Summarise why the API rejected the request, for the caller to act on.""" + if response is None: + return "no response received" + + problem = _parse_problem_details(response) + if problem is None: + text = (response.text or "").strip() + return f"HTTP {response.status_code}: {text[:200]}" if text else f"HTTP {response.status_code}" + + parts: list[str] = [] + + reason = problem.get("detail") or problem.get("title") + if reason: + parts.append(str(reason)) + + errors = problem.get("errors") + if isinstance(errors, dict) and errors: + parts.append(_format_validation_errors(errors)) + + return "\n".join(parts) if parts else f"HTTP {response.status_code}" class Gateway: @@ -153,8 +194,10 @@ def import_dataset( else None ) error_msg = ( - f"Failed to create data catalog entry. correlation-id: {trace_id}" + f"Failed to create data catalog entry: {_describe_failure(e.response)}" ) + if trace_id: + error_msg = f"{error_msg} (correlation-id: {trace_id})" logger.error(error_msg) raise type(e)(error_msg) from e diff --git a/tests/test_gateway.py b/tests/test_gateway.py index 0d5ec2d..3c7138e 100644 --- a/tests/test_gateway.py +++ b/tests/test_gateway.py @@ -40,7 +40,7 @@ def test_import_dataset_failure(mock_gateway, mock_post): response_body = {"type":"BusinessError","traceId":"8b01e7eb14484611add6138618daf112"} mock_response = MagicMock() - mock_response.return_value.status_code = 400 + mock_response.status_code = 400 mock_response.text = json.dumps(response_body) http_error = requests.exceptions.HTTPError() @@ -52,5 +52,88 @@ def test_import_dataset_failure(mock_gateway, mock_post): with pytest.raises(RequestException) as e: mock_gateway.import_dataset("dataset_name", "dataset_description", "schema_id", {"tag": "value"}, "folder_id") - assert e.value.args[0] == f"Failed to create data catalog entry. correlation-id: {response_body['traceId']}" + assert response_body["traceId"] in e.value.args[0] mock_post.assert_called_once() + + +def _failing_post(mock_post, *, text, status_code=400): + """Point requests.post at a response that raises HTTPError carrying `text`.""" + response = MagicMock() + response.status_code = status_code + response.text = text + + http_error = requests.exceptions.HTTPError() + http_error.response = response + response.raise_for_status.side_effect = http_error + mock_post.return_value = response + return response + + +def _import(gateway): + return gateway.import_dataset( + "dataset_name", "dataset_description", "schema_id", {"tag": "value"}, "folder_id" + ) + + +def test_import_dataset_failure_surfaces_problem_detail(mock_gateway, mock_post): + """The `detail` the API explains the failure with must reach the caller.""" + _failing_post(mock_post, text=json.dumps({ + "title": "BadRequest", + "status": 400, + "detail": "Ensure DatasetName is unique when creating Predefined Dataset.", + "traceId": "abc123", + })) + + with pytest.raises(RequestException) as e: + _import(mock_gateway) + + assert "Ensure DatasetName is unique when creating Predefined Dataset." in e.value.args[0] + assert "abc123" in e.value.args[0] + + +def test_import_dataset_failure_surfaces_validation_errors(mock_gateway, mock_post): + """Per-field validation errors are the actionable part of a 400.""" + _failing_post(mock_post, text=json.dumps({ + "title": "BadRequest", + "status": 400, + "errors": {"datasetName": ["must not be empty", "must be unique"]}, + "traceId": "def456", + })) + + with pytest.raises(RequestException) as e: + _import(mock_gateway) + + assert "datasetName" in e.value.args[0] + assert "must not be empty" in e.value.args[0] + # The summary and the field errors belong on separate lines so the message stays readable. + assert "BadRequest\ndatasetName: must not be empty, must be unique" in e.value.args[0] + + +def test_import_dataset_failure_falls_back_to_title(mock_gateway, mock_post): + """A body with no `detail` should still say something better than the trace id.""" + _failing_post(mock_post, text=json.dumps({"title": "Conflict", "status": 409, "traceId": "ghi789"})) + + with pytest.raises(RequestException) as e: + _import(mock_gateway) + + assert "Conflict" in e.value.args[0] + + +def test_import_dataset_failure_with_non_json_body(mock_gateway, mock_post): + """A gateway/proxy can answer with HTML; parsing it must not mask the real error.""" + _failing_post(mock_post, text="502 Bad Gateway", status_code=502) + + with pytest.raises(RequestException) as e: + _import(mock_gateway) + + assert "502" in e.value.args[0] + + +def test_import_dataset_failure_without_response(mock_gateway, mock_post): + """A connection error has no response at all.""" + mock_post.side_effect = requests.exceptions.ConnectionError("connection refused") + + with pytest.raises(RequestException) as e: + _import(mock_gateway) + + assert "Failed to create data catalog entry" in e.value.args[0]