fix(task-sdk): include error details in ServerResponseError string representation#63368
fix(task-sdk): include error details in ServerResponseError string representation#63368YoannAbriel wants to merge 3 commits into
Conversation
af40481 to
92a5e30
Compare
SameerMesiah97
left a comment
There was a problem hiding this comment.
Needs some adjustments. Please see my comments.
|
|
||
| def __str__(self) -> str: | ||
| base = super().__str__() | ||
| if self.detail: |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed — using getattr(self, 'detail', None) now.
| assert "Detail:" in error_str | ||
| assert "invalid_state" in error_str | ||
|
|
||
| def test_server_response_error_str_without_detail(self): |
There was a problem hiding this 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.
There was a problem hiding this comment.
Added test_server_response_error_str_missing_detail_attr that creates a bare instance without the attribute.
abba1a1 to
6137ca4
Compare
f58495e to
f6f8cac
Compare
|
@YoannAbriel A few things need addressing before review — see our Pull Request quality criteria.
No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
bb5abb3 to
36028f3
Compare
fd85240 to
367b52c
Compare
6192734 to
ba32e12
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves debuggability of ServerResponseError by ensuring its string representation includes server-provided error details when available, so tracebacks/logs retain actionable context.
Changes:
- Override
ServerResponseError.__str__()to appenddetailto the base message. - Add unit tests covering
__str__()behavior with dict detail, string detail, and missingdetailattribute.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| task-sdk/src/airflow/sdk/api/client.py | Adds __str__() override to include detail in the exception string. |
| task-sdk/tests/task_sdk/api/test_client.py | Adds tests validating the new __str__() behavior across detail variants. |
|
|
||
| def __str__(self) -> str: | ||
| base = super().__str__() | ||
| if detail := getattr(self, "detail", None): |
There was a problem hiding this 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).
| if detail := getattr(self, "detail", None): | |
| detail = getattr(self, "detail", None) | |
| if detail is not None: |
| err = ServerResponseError.__new__(ServerResponseError) | ||
| err.args = ("Server returned error",) | ||
|
|
There was a problem hiding this 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.
| err = ServerResponseError.__new__(ServerResponseError) | |
| err.args = ("Server returned error",) | |
| 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"] |
3c58f24 to
23f0a55
Compare
…presentation When ServerResponseError propagated up the stack without being explicitly caught and handled, the error details were lost because __str__() only returned the base message without the detail attribute. This made debugging difficult as logs would show 'Server returned error' without any context about what actually went wrong. Added __str__() override to include the detail when present, so error details are always visible in logs and tracebacks regardless of how the exception is caught and re-raised. Closes: apache#57961
23f0a55 to
92e9202
Compare
|
@YoannAbriel can you address and resolve all open comments |
Problem
When
ServerResponseErroris raised and propagates up the call stack without being explicitly caught and handled, error details are lost. Logs show only:...without any information about what actually went wrong. This makes debugging significantly harder, especially for
TaskInstanceOperationscalls where the exception is not caught and re-logged withe.detail.Root Cause
ServerResponseErrorstores error details in itsdetailattribute, but the__str__()method (inherited fromhttpx.HTTPStatusError) only returns the base message. When the exception is logged via traceback orstr(err), the detail is silently dropped.Some call sites (e.g.,
VariableOperations) explicitly catchServerResponseErrorand loge.detail, but many others (e.g.,TaskInstanceOperations.start()) let it propagate, losing the detail entirely.Fix
Added a
__str__()override toServerResponseErrorthat appends thedetailattribute when present. This ensures error details are always visible in logs and tracebacks, regardless of how the exception is caught.Before:
After:
Verified with unit tests. No external service access needed.
Closes: #57961
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4, claude-opus-4-6) following the guidelines