Skip to content

fix(task-sdk): include error details in ServerResponseError string representation #63368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
YoannAbriel wants to merge 3 commits into
base: main
Choose a base branch
from
+45 −0
Open

fix(task-sdk): include error details in ServerResponseError string representation #63368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4939adf
fix(task-sdk): include error details in ServerResponseError string re…
YoannAbriel Mar 11, 2026
e565683
fix(task-sdk): handle missing detail attr in ServerResponseError
YoannAbriel Apr 8, 2026
92e9202
style: apply ruff formatting
YoannAbriel Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions task-sdk/src/airflow/sdk/api/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
if detail := getattr(self, "detail", None):
detail = getattr(self, "detail", None)
if detail is not None:

Copilot uses AI. Check for mistakes.
return f"{base}\nDetail: {detail}"
return base

def __reduce__(self) -> tuple[Any, ...]:
# Needed because https://github.com/encode/httpx/pull/3108 isn't merged yet.
return Exception.__new__, (type(self),) + self.args, self.__dict__
Expand Down
39 changes: 39 additions & 0 deletions task-sdk/tests/task_sdk/api/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Copy Markdown
Contributor

@SameerMesiah97 SameerMesiah97 Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@YoannAbriel YoannAbriel Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test_server_response_error_str_missing_detail_attr that creates a bare instance without the attribute.

"""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",)

Comment on lines +212 to +214
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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"]

Copilot uses AI. Check for mistakes.
assert "Detail:" not in str(err)

def test_retry_handling_unrecoverable_error(self):
with time_machine.travel("2023-01-01T00:00:00Z", tick=False):
responses: list[httpx.Response] = [
Expand Down
Loading