SameerMesiah97 commented on code in PR #63368:
URL: https://github.com/apache/airflow/pull/63368#discussion_r2937121555
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -174,6 +174,39 @@ 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):
Review Comment:
This test appears to covering scenarios where detail exists but is `None`. I
would add another test for cases where the detail attribute does not exist. I
would ensure that both of these tests (the current one for detail = `None` and
the new one for detail not present which you may add) communicate their intent
precisely in their naming, docstrings and comments.
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1027,6 +1027,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 self.detail:
Review Comment:
Right now `def __str__` assumes that the detail attribute is always present
in `ServerResponseError`. As evidenced by failing CI, this is not always true.
Try adding doing something like this instead:
```
def __str__(self) -> str:
base = super().__str__()
detail = getattr(self, "detail", None)
if detail is not None:
return f"{base}\nDetail: {detail}"
return base
```
Using `if detail is not None` instead of `if self.detail` or `if not
self.detail` has the added benefit of including falsy values like `{}` or `""`,
which might be considered meaningful API messages.
--
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]
