feat: [CHA-2958] error hierarchy + wait_for_task#261
Open
mogita wants to merge 3 commits into
Open
Conversation
Adds StreamException / StreamApiException / StreamRateLimitException / StreamTransportException / StreamTaskException per the Server-Side SDK Error Handling Spec §9.2. APIError envelope parsing now surfaces unrecoverable, exception_fields, more_info, details, and the raw body on the new StreamApiException class. 429s build StreamRateLimitException with retry_after parsed from Retry-After (RFC 7231 §7.1.3 — integer seconds or HTTP-date, past dates clamp to zero, missing/garbage → None). httpx.RequestError raised by the sync and async transport paths is wrapped into StreamTransportException with an error_type enum (connection_reset / timeout / dns_failure / tls_handshake_failed / unknown) and the original exception preserved via __cause__. The classifier walks __cause__/__context__ to detect ssl.SSLError and socket.gaierror that httpx hides one level down. Adds Stream.wait_for_task and AsyncStream.wait_for_task that poll get_task until terminal state. Failed tasks raise StreamTaskException populated from ErrorResult; timeouts raise StreamTransportException with error_type='timeout'. getstream.base.StreamAPIException (capital API) is now an alias for the new StreamApiException via a module-level __getattr__ that emits DeprecationWarning. The alias points at the same class object, so isinstance / except / pytest.raises keep working without changes; it will be removed one minor cycle after this release.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a new exception module with API/transport/task exception types and helpers, integrates wrapping into the base client, adds sync/async task polling utilities and client convenience methods, re-exports exceptions at package level, updates changelog, and includes comprehensive tests. ChangesStream SDK Exception Hierarchy and Task Polling
Sequence Diagram(s)sequenceDiagram
participant Client
participant BaseClient
participant Exceptions
Client->>BaseClient: send HTTP request
BaseClient-->>Client: httpx.RequestError (network) / non-2xx response
BaseClient->>Exceptions: wrap_transport_error(err) / build_api_exception(response)
Exceptions-->>BaseClient: StreamTransportException / StreamApiException (or StreamRateLimitException)
BaseClient-->>Client: raise Stream* exception (with __cause__ chained)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_exceptions.py (1)
44-550: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor tests to fixture-driven, class-grouped structure for guideline compliance.
This module is fully function-based and builds most test objects inline. Please convert shared setup to pytest fixtures and group related tests into test classes.
As per coding guidelines,
**/test_*.py: "Use fixtures to inject objects in tests; test client fixtures can use the Stream API client" and "Keep tests well organized and use test classes to group similar tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_exceptions.py` around lines 44 - 550, Refactor this module by extracting repeated setup into pytest fixtures and grouping related tests into classes: create fixtures for the HTTP helper (_make_response), request helper (_request), mock clients (_FakeSyncClient, _FakeAsyncClient) and common response bodies, then move related tests into test classes such as TestApiException (tests using build_api_exception), TestRetryAfter (parse_retry_after tests), TestTransportWrapping (classify_transport_error, wrap_transport_error, transport mock tests), TestWaitForTask (wait_for_task_sync/async using the fake clients), and TestStreamTaskException; update tests to accept fixtures via parameters and replace inline construction with fixture usage while keeping assertions and referencing symbols like build_api_exception, parse_retry_after, classify_transport_error, wrap_transport_error, wait_for_task_sync, wait_for_task_async, _make_response, _request, _FakeSyncClient, and _FakeAsyncClient so tests remain functionally identical but fixture-driven and class-organized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_exceptions.py`:
- Around line 329-383: Replace the MockTransport-based tests
(test_transport_wrapping_via_mock_transport_sync and
test_transport_wrapping_via_mock_transport_async) with fixture-backed servers:
stop using httpx.MockTransport and instead use a pytest http server fixture
(e.g., pytest-httpserver or httpserver) to simulate the failure modes (for sync
test simulate a connection reset by having the server close the connection
immediately or by pointing the client at a stopped server; for async test
simulate a read timeout by delaying the response beyond the client's timeout).
Update the Stream and AsyncStream instantiation in those tests to use the
fixture server's URL, assert the same StreamTransportException properties
(info.value.error_type and __cause__), and ensure proper cleanup via
client.close() / await client.aclose(); replace any monkeypatched fake
transports with the fixture usage and remove httpx.MockTransport references.
---
Outside diff comments:
In `@tests/test_exceptions.py`:
- Around line 44-550: Refactor this module by extracting repeated setup into
pytest fixtures and grouping related tests into classes: create fixtures for the
HTTP helper (_make_response), request helper (_request), mock clients
(_FakeSyncClient, _FakeAsyncClient) and common response bodies, then move
related tests into test classes such as TestApiException (tests using
build_api_exception), TestRetryAfter (parse_retry_after tests),
TestTransportWrapping (classify_transport_error, wrap_transport_error, transport
mock tests), TestWaitForTask (wait_for_task_sync/async using the fake clients),
and TestStreamTaskException; update tests to accept fixtures via parameters and
replace inline construction with fixture usage while keeping assertions and
referencing symbols like build_api_exception, parse_retry_after,
classify_transport_error, wrap_transport_error, wait_for_task_sync,
wait_for_task_async, _make_response, _request, _FakeSyncClient, and
_FakeAsyncClient so tests remain functionally identical but fixture-driven and
class-organized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8c72a26-f491-47d8-bf64-0634eb49fa22
📒 Files selected for processing (7)
CHANGELOG.mdgetstream/__init__.pygetstream/base.pygetstream/exceptions.pygetstream/stream.pygetstream/tasks.pytests/test_exceptions.py
mogita
changed the title
…kets Honors AGENTS.md "do not use mocks or mock things in general unless you are asked to do that directly" (also flagged by CodeRabbit on PR #261). Two transport-wrapping tests previously used httpx.MockTransport. They now drive real httpx errors: - Connection-refused goes via a freshly-bound-and-closed loopback port. - Read-timeout goes via a real pytest-httpserver instance that sleeps past the client's request_timeout. Six wait_for_task tests previously used _Fake* stand-in client + response classes. They now point a real Stream/AsyncStream client at a pytest- httpserver that returns get-task JSON in the real wire format (nanosecond-epoch created_at/updated_at, ErrorResult payload). Adds pytest-httpserver as a dev dependency.
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
1 participant
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.
Summary
getstreamorgetstream.exceptions):StreamException(abstract base)StreamApiException— HTTP 4xx/5xx, full APIError envelope includingunrecoverableandexception_fieldsStreamRateLimitException— HTTP 429, addsretry_after: timedelta | NoneStreamTransportException— network failures,error_typeenum,__cause__preservedStreamTaskException— forwait_for_taskfailuresStream.wait_for_task(...)(sync) andAsyncStream.wait_for_task(...)(async). 1s poll, 60s timeout by default.httpx.RequestErroris wrapped at the SDK boundary in both sync and async paths.Retry-Afterparsing covers integer seconds and HTTP-date; past dates clamp to zero; missing / garbage →None.Back-compat
getstream.base.StreamAPIException(capitalAPI) preserved as an alias for one minor cycle via module-level__getattr__that emitsDeprecationWarning. Same class object as the new spelling, sopytest.raises(StreamAPIException),except StreamAPIException, andisinstance(...)keep working.Test plan
uv run pytest tests/test_exceptions.pyuv run pytest(full suite)uv run ruff check .Summary by CodeRabbit
New Features
Changed
Deprecated