poc: ErrorHandler.interpret_response can return None #790
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@maxi297/poc_error_handler_change#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch maxi297/poc_error_handler_changeHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughThe error-handling API now allows interpret_response to return Optional[ErrorResolution] across interfaces and implementations. Fallback error resolution creation was removed (now returns None). HTTP client defaults None to a SUCCESS resolution. A default HttpResponseFilter is no longer auto-added in the declarative factory. A variable name typo was corrected. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as HttpClient
participant H as ErrorHandler
participant S as Server
C->>S: Send HTTP request
S-->>C: Response / Exception
C->>H: interpret_response(response_or_exception)
H-->>C: Optional ErrorResolution (may be None)
alt Resolution is None
Note over C: Default to SUCCESS resolution
else Resolution provided
Note over C: Use provided resolution
end
C->>C: _handle_error_resolution(resolution)
sequenceDiagram
autonumber
participant CEH as CompositeErrorHandler
participant EH1 as ErrorHandler A
participant EH2 as ErrorHandler B
CEH->>EH1: interpret_response(ctx)
EH1-->>CEH: Optional ErrorResolution
alt EH1 returned ErrorResolution
CEH-->>CEH: Return that resolution
else EH1 returned None
CEH->>EH2: interpret_response(ctx)
EH2-->>CEH: Optional ErrorResolution
alt EH2 returned ErrorResolution
CEH-->>CEH: Return that resolution
else No handlers matched
Note over CEH: No fallback resolution (returns None)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
111-132: Adjust interpret_response contract to OptionalNow that
create_fallback_error_resolutionreturnsNone, this override can leakNoneeven though its signature still promises anErrorResolution, which breaks the new abstract signature and trips our type checks. Could we update the return annotation toOptional[ErrorResolution](and let callers keep handling theNonecase) to stay aligned, wdyt?- def interpret_response( - self, response_or_exception: Optional[Union[requests.Response, Exception]] - ) -> ErrorResolution: + def interpret_response( + self, response_or_exception: Optional[Union[requests.Response, Exception]] + ) -> Optional[ErrorResolution]:airbyte_cdk/sources/streams/http/http_client.py (1)
345-385: Let ruff format this new expressionThe formatter is currently failing on this file, and the new ternary line is the culprit. Could we run
poetry run ruff format .(or wrap this assignment manually) so the pipeline goes green, wdyt?
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
59-68: Acknowledge expected pipeline failures.The pipeline failures at line 68 are expected as part of this PR's design:
[error] 68-68: "create_fallback_error_resolution" does not return a value (it only ever returns None)According to the AI summary,
create_fallback_error_resolutionnow returnsNoneinstead of creating a fallback resolution, and the HTTP client is responsible for substituting a default SUCCESS resolution when it receivesNone. The logic change at line 65 (checkingisinstance) correctly ensures only concreteErrorResolutioninstances are returned early, allowing the fallback toNoneat line 68 when no handler matches.Based on the PR design, wdyt about adding a comment at line 68 explaining that None is intentional and handled by the HTTP client? This would help future maintainers understand the contract.
+ # Return None when no handler matches; HTTP client will substitute a default SUCCESS resolution return create_fallback_error_resolution(response_or_exception)airbyte_cdk/sources/streams/http/http.py (1)
629-633: Guard against None from super().interpret_response?Since
super().interpret_response()now returnsOptional[ErrorResolution], it may returnNonehere—should we fallback to a defaultErrorResolutionfor consistency, or rely on callers to handleNone? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(0 hunks)airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py(1 hunks)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py(1 hunks)airbyte_cdk/sources/streams/http/error_handlers/error_handler.py(1 hunks)airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py(1 hunks)airbyte_cdk/sources/streams/http/error_handlers/response_models.py(1 hunks)airbyte_cdk/sources/streams/http/http.py(1 hunks)airbyte_cdk/sources/streams/http/http_client.py(2 hunks)unit_tests/sources/streams/http/error_handlers/test_response_models.py(2 hunks)
💤 Files with no reviewable changes (1)
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🧬 Code graph analysis (7)
airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/default_http_response_filter.py (2)
DefaultHttpResponseFilter(21-40)matches(22-40)airbyte_cdk/sources/declarative/requesters/error_handlers/http_response_filter.py (1)
matches(72-118)
airbyte_cdk/sources/streams/http/http_client.py (6)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (2)
ErrorResolution(24-27)ResponseAction(14-20)airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
interpret_response(59-68)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
interpret_response(111-132)airbyte_cdk/sources/streams/http/error_handlers/error_handler.py (1)
interpret_response(33-42)airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py (1)
interpret_response(48-110)airbyte_cdk/sources/streams/http/http.py (1)
interpret_response(629-673)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (5)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ErrorResolution(24-27)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
interpret_response(111-132)airbyte_cdk/sources/streams/http/error_handlers/error_handler.py (1)
interpret_response(33-42)airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py (1)
interpret_response(48-110)airbyte_cdk/sources/streams/http/http.py (1)
interpret_response(629-673)
airbyte_cdk/sources/streams/http/http.py (1)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ErrorResolution(24-27)
airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py (1)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ErrorResolution(24-27)
airbyte_cdk/sources/streams/http/error_handlers/error_handler.py (1)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ErrorResolution(24-27)
unit_tests/sources/streams/http/error_handlers/test_response_models.py (1)
airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
create_fallback_error_resolution(46-49)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py
[error] 131-131: "create_fallback_error_resolution" does not return a value (it only ever returns None) [func-returns-value]
airbyte_cdk/sources/streams/http/http_client.py
[error] 380-380: Ruff formatting check failed. 1 file would be reformatted by 'poetry run ruff format --diff .'
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py
[error] 68-68: Returning Any from function declared to return "ErrorResolution | None" [no-any-return]
[error] 68-68: "create_fallback_error_resolution" does not return a value (it only ever returns None) [func-returns-value]
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
🔇 Additional comments (1)
airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py (1)
48-50: LGTM! Signature aligns with interface.The return type change to
Optional[ErrorResolution]maintains consistency with the updatedErrorHandlerinterface. While this implementation always returns a concreteErrorResolution, the optional return type provides flexibility for other implementations and maintains a uniform contract.
PyTest Results (Fast)1 473 tests - 2 324 1 461 ✅ - 2 324 5m 22s ⏱️ - 1m 10s For more details on these failures, see this check. Results for commit b6dd0fa. ± Comparison against base commit 035264c. This pull request removes 2324 tests. |
PyTest Results (Full)3 797 tests - 3 3 780 ✅ - 8 11m 7s ⏱️ +8s For more details on these failures, see this check. Results for commit b6dd0fa. ± Comparison against base commit 035264c. This pull request removes 3 tests. |
| ) | ||
| response_filters.append( | ||
| HttpResponseFilter(config=config, parameters=model.parameters or {}) | ||
| ) |
There was a problem hiding this comment.
I assume because the fallback is create_fallback_error_resolution which was returning a system_error with RETRY
|
pending fixing tests obv |
Summary by CodeRabbit
Bug Fixes
Refactor
Tests