Conversation
The failure path read the response body only to pull out traceId and threw the rest away, so a caller was told nothing beyond a correlation id and had to ask someone to look up the trace before they could act. The raised error now carries the reason the API gave: the ProblemDetails detail or title, plus any per-field validation errors, with the correlation id kept at the end. Parsing is also no longer assumed to succeed. A gateway answering with HTML made json.loads raise inside the except block, replacing the real failure with a JSONDecodeError; a non-JSON body now degrades to the status code and a short excerpt.
There was a problem hiding this comment.
🟡 Changes recommended
The no-response error path loses actionable exception details, and the fallback excerpt needs stronger test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves dataset import failures by surfacing API error details and safely handling non-JSON responses.
Changes:
- Formats ProblemDetails and validation errors.
- Preserves correlation IDs and adds fallback descriptions.
- Adds coverage for API, proxy, and connection failures.
File summaries
| File | Summary | Review feedback |
|---|---|---|
src/dataworkbench/gateway.py |
Formats and surfaces detailed import failures. | Moderate (2 votes): Preserve the original exception text when no response is available. |
tests/test_gateway.py |
Tests detailed, validation, fallback, and connection errors. | Nit (1 vote): Assert the non-JSON fallback includes 502 Bad Gateway. |
Review details
Suppressed comments (1)
tests/test_gateway.py:129
- This assertion only checks the status code, so the test would still pass if the non-JSON fallback regressed to
HTTP 502and dropped the body excerpt promised by the new implementation. Assert that502 Bad Gatewayis also present to cover the excerpt behavior.
assert "502" in e.value.args[0]
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+197
to
198
| f"Failed to create data catalog entry: {_describe_failure(e.response)}" | ||
| ) |
dorislinda
approved these changes
Sep 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Gateway.import_datasetparsed the error response only to pull outtraceIdand discarded everything else, so a failedDataCatalogue.save(...)told the user:The API already returns a ProblemDetails body explaining the failure (
detail,title, per-fielderrors) — none of it reached the caller, who had to ask someone to look up the trace before they could act.There was also a second failure mode:
_get_trace_id_from_responsecalledjson.loads(response.text)unguarded. When a gateway or proxy answered with HTML (e.g. a 502), that raisedJSONDecodeErrorinside the except block, replacing the real error with a parse error.Change
The raised error now carries the reason the API gave, with the correlation id kept at the end:
detail…correlation-id: 0af412aa……: Ensure DatasetName is unique when creating Predefined Dataset. (correlation-id: 0af412aa…)errors…correlation-id: abc…: BadRequestdatasetName: must not be empty; schemaId: not found (correlation-id: abc)JSONDecodeError…: HTTP 502: <html>…502 Bad Gateway…</html>JSON parsing is now tolerant, and a non-JSON body degrades to the status code plus a short excerpt rather than masking the failure.
Tests
5 new tests in
tests/test_gateway.pycoveringdetail, per-fielderrors,titlefallback, a non-JSON body, and a connection error with no response at all.The existing
test_import_dataset_failureasserted the old exact string, so it now asserts the correlation id is still present rather than pinning the message.Suite: 45 → 50 passing.