Copilot commented on code in PR #63368:
URL: https://github.com/apache/airflow/pull/63368#discussion_r3066472812
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1062,6 +1062,12 @@ def __init__(self, message: str, *, request:
httpx.Request, response: httpx.Resp
detail: list[RemoteValidationError] | str | dict[str, Any] | None
+ def __str__(self) -> str:
+ base = super().__str__()
+ if detail := getattr(self, "detail", None):
Review Comment:
The `if detail := ...` check is truthiness-based, so empty-but-present
details (e.g., `{}` / `[]`) will be omitted even though `detail` exists. If the
intent is to include details whenever the attribute is present (and not
`None`), assign first and check `detail is not None` (or alternatively check
`hasattr(self, 'detail')` and `self.detail is not None`).
```suggestion
detail = getattr(self, "detail", None)
if detail is not None:
```
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -175,6 +175,45 @@ def test_server_response_error_pickling(self):
assert unpickled.response.status_code == 404
assert unpickled.request.url == "http://error";
+ def test_server_response_error_str_includes_detail(self):
+ """Test that ServerResponseError string representation includes error
details."""
+ responses = [
+ httpx.Response(
+ 409,
+ json={"detail": {"reason": "invalid_state", "message": "TI was
not in the running state"}},
+ )
+ ]
+ client = make_client_w_responses(responses)
+
+ with pytest.raises(ServerResponseError) as exc_info:
+ client.get("http://error";)
+
+ err = exc_info.value
+ error_str = str(err)
+ assert "Detail:" in error_str
+ assert "invalid_state" in error_str
+
+ def test_server_response_error_str_without_detail(self):
+ """Test that ServerResponseError string without detail doesn't include
Detail section."""
+ # 422 with a string detail: the string becomes the message, detail
attr is None
+ responses = [httpx.Response(422, json={"detail": "Simple error"})]
+ client = make_client_w_responses(responses)
+
+ with pytest.raises(ServerResponseError) as exc_info:
+ client.get("http://error";)
+
+ err = exc_info.value
+ error_str = str(err)
+ assert "Simple error" in error_str
+ # detail is None when the detail string is used as the message itself
+ assert "Detail:" not in error_str
+
+ def test_server_response_error_str_missing_detail_attr(self):
+ err = ServerResponseError.__new__(ServerResponseError)
+ err.args = ("Server returned error",)
+
Review Comment:
This test relies on constructing `ServerResponseError` via `__new__` without
initializing the underlying `httpx.HTTPStatusError` state. That makes the test
brittle against future `httpx` changes (e.g., if `HTTPStatusError.__str__()`
starts requiring `request`/`response`). A more robust approach is to create a
real `ServerResponseError` instance with minimal `request`/`response`, then
remove/clear the `detail` attribute (e.g., delete it from `__dict__`) and
assert `str(err)` does not include the `Detail:` section.
```suggestion
responses = [
httpx.Response(
409,
json={"detail": {"reason": "invalid_state", "message": "TI
was not in the running state"}},
)
]
client = make_client_w_responses(responses)
with pytest.raises(ServerResponseError) as exc_info:
client.get("http://error";)
err = exc_info.value
if "detail" in err.__dict__:
del err.__dict__["detail"]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
