feat: Add CustomErrorHandler to CompositeErrorHandler list #786
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables CustomErrorHandler to be used within CompositeErrorHandler lists, allowing developers to combine custom error handling logic with existing error handlers in declarative manifests. This avoids the need to copy error handling code from manifests to Python when custom behavior is required.
- Updated type definitions to include
CustomErrorHandlerinCompositeErrorHandlererror handler lists - Added comprehensive unit tests for the new functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Updated Python type hints to include CustomErrorHandler in CompositeErrorHandler union type |
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Updated YAML schema to include CustomErrorHandler reference in error handler list |
| unit_tests/sources/declarative/parsers/testing_components.py | Added TestingCustomErrorHandler class for testing custom error handlers |
| unit_tests/sources/declarative/parsers/test_model_to_component_factory.py | Added comprehensive test for CompositeErrorHandler with CustomErrorHandler integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
👋 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@agarctfi/add-custom-error-handler-to-composite#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 agarctfi/add-custom-error-handler-to-compositeHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
|
/autofix
|
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
📝 WalkthroughWalkthroughThe composite error handler schema and model are updated to accept CustomErrorHandler in addition to CompositeErrorHandler and DefaultErrorHandler. Corresponding unit tests add a testing custom handler class and a test validating CompositeErrorHandler construction with a custom sub-handler. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Would you like to also add a negative-path test ensuring manifests with invalid handler types in CompositeErrorHandler are rejected, wdyt? 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
🧹 Nitpick comments (1)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
2030-2066: Nice test coverage for the new CustomErrorHandler in CompositeErrorHandler!The test structure looks solid and validates the core functionality described in the PR objectives. A couple of suggestions to consider:
Import for clarity: I noticed
TestingCustomErrorHandleris referenced via theclass_namestring (line 2036) but doesn't appear in the imports at the top of the file. While the test works with the string reference, would it make sense to add it to the imports on lines 180-183? This would allow you to useisinstance(error_handler_0, TestingCustomErrorHandler)on line 2058 instead of checking__class__.__name__, which would be more robust and explicit. What do you think?Optional docstring: Consider adding a brief docstring to explain what scenario this test validates, especially since it's testing a new feature. Something like:
"""Test that CompositeErrorHandler can include a CustomErrorHandler alongside a DefaultErrorHandler."""Though I see most other tests in this file don't have docstrings, so this is just a nice-to-have for consistency with best practices.
The test logic itself is clear and the assertions properly validate both error handlers and their configurations. Great work! 🎉
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(1 hunks)unit_tests/sources/declarative/parsers/testing_components.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
CompositeErrorHandler(20-101)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (9)
airbyte_cdk/sources/declarative/yaml_declarative_source.py (2)
YamlDeclarativeSource(17-69)_parse(62-69)airbyte_cdk/sources/declarative/parsers/manifest_reference_resolver.py (1)
preprocess_manifest(102-107)airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (1)
propagate_types_and_parameters(87-188)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
create_component(799-832)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
CompositeErrorHandler(2150-2159)DefaultErrorHandler(1968-1996)HttpResponseFilter(552-600)airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
CompositeErrorHandler(20-101)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)airbyte_cdk/sources/declarative/requesters/error_handlers/http_response_filter.py (1)
HttpResponseFilter(25-179)airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ResponseAction(14-20)
unit_tests/sources/declarative/parsers/testing_components.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
DefaultErrorHandler(1968-1996)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)
⏰ 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-intercom
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
🔇 Additional comments (3)
unit_tests/sources/declarative/parsers/testing_components.py (1)
93-100: LGTM! Clean test helper implementation.The new
TestingCustomErrorHandlerfollows the established pattern in this file perfectly. It properly extendsDefaultErrorHandler, uses the__test__marker to avoid pytest confusion, and provides a minimal implementation suitable for testing manifest parsing with custom error handlers.airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
394-394: Schema change looks good! Quick verification question.Adding
CustomErrorHandlerto theCompositeErrorHandler.error_handlersoptions enables the use case you described—composing custom and default error handlers without duplicating manifest contents into Python. The schema change follows the existing pattern correctly.Since the PR description mentions a test (
test_create_composite_error_handler_with_custom_error_handler), I assume the Python model parsing code already handles this correctly, but wanted to confirm: have you verified that theModelToComponentFactoryproperly instantiates aCompositeErrorHandlerwith aCustomErrorHandleras one of its sub-handlers? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2152-2158: LGTM! Nice consistency with HttpRequester's error_handler pattern.Adding
CustomErrorHandlerto theCompositeErrorHandler.error_handlersUnion makes perfect sense architecturally and aligns well with howHttpRequester.error_handler(line 2683) already supports all three error handler types. This gives manifest authors the flexibility to compose custom error handlers alongside the default ones without duplicating logic in Python.Since this is an auto-generated file from the YAML schema, the change flows through cleanly. The tests you added validate the composition works correctly, which is great.
One thought: does the model-to-component factory perform any runtime validation to ensure that the
class_namespecified in a CustomErrorHandler actually implements the ErrorHandler interface? Just curious if there are safeguards against misconfiguration, wdyt?
📝 WalkthroughWalkthroughAdds CustomErrorHandler as an allowed child type within CompositeErrorHandler in both the declarative schema and the generated Pydantic model. Updates unit tests to cover mixed custom/default handlers and introduces a minimal test-only CustomErrorHandler class. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant YAML as Declarative YAML
participant Parser as ModelToComponentFactory
participant Comp as CompositeErrorHandler
participant Cust as CustomErrorHandler
participant Def as DefaultErrorHandler
Dev->>YAML: Define composite with [CustomErrorHandler, DefaultErrorHandler]
Parser->>YAML: Load and validate (schema allows CustomErrorHandler)
Parser->>Comp: Instantiate CompositeErrorHandler
Parser->>Cust: Instantiate first child (custom)
Parser->>Def: Instantiate second child (default, http_codes=[429], action=RETRY)
Comp->>Comp: Register children in order
note over Comp: Composite now holds custom + default handlers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (2)
2030-2030: Consider adding a docstring to explain the test's purpose.Since this test validates a significant new capability (mixing custom and default error handlers in a composite), a brief docstring could help future maintainers understand its importance, wdyt?
def test_create_composite_error_handler_with_custom_error_handler(): + """ + Test that CompositeErrorHandler can contain a mix of CustomErrorHandler and DefaultErrorHandler. + This enables preserving existing DefaultErrorHandler behavior while adding custom handlers without + converting everything to Python code. + """ content = """
2056-2058: Preferisinstance()over string comparison for type checking.The test currently checks
error_handler_0.__class__.__name__ == "TestingCustomErrorHandler", but other tests in this file useisinstance()for custom components (e.g., lines 2358, 2412). This approach would be more robust and consistent, wdyt?+from unit_tests.sources.declarative.parsers.testing_components import ( + TestingCustomErrorHandler, + TestingCustomSubstreamPartitionRouter, + TestingSomeComponent, +)Then update the assertion:
# First error handler should be a custom error handler error_handler_0 = error_handler.error_handlers[0] - assert error_handler_0.__class__.__name__ == "TestingCustomErrorHandler" + assert isinstance(error_handler_0, TestingCustomErrorHandler)This would:
- Match the pattern used for checking the second handler (line 2062)
- Provide better error messages if the wrong type is returned
- Be more maintainable if the class is refactored
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(1 hunks)unit_tests/sources/declarative/parsers/testing_components.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
unit_tests/sources/declarative/parsers/testing_components.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
DefaultErrorHandler(1968-1996)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
CompositeErrorHandler(20-101)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (8)
airbyte_cdk/sources/declarative/yaml_declarative_source.py (1)
_parse(62-69)airbyte_cdk/sources/declarative/parsers/manifest_reference_resolver.py (1)
preprocess_manifest(102-107)airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (1)
propagate_types_and_parameters(87-188)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
create_component(799-832)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
CompositeErrorHandler(2150-2159)DefaultErrorHandler(1968-1996)HttpResponseFilter(552-600)airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
CompositeErrorHandler(20-101)airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py (1)
DefaultErrorHandler(26-147)airbyte_cdk/sources/streams/http/error_handlers/response_models.py (1)
ResponseAction(14-20)
⏰ 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-intercom
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2152-2158: LGTM! Clean union expansion.The addition of
CustomErrorHandlerto the union type is straightforward and enables the desired composite error handling configuration described in the PR objectives. The multi-line formatting with parentheses improves readability as well.unit_tests/sources/declarative/parsers/testing_components.py (1)
93-100: LGTM! Consistent test fixture.The new
TestingCustomErrorHandlerfollows the established pattern for test components in this file perfectly. The__test__ = Falseannotation is essential given the "Testing" prefix, and the minimal implementation is appropriate for its intended use as a test fixture, wdyt?
2 participants
For GSheets Connector, we have this PR, which required a CustomErrorHandler to be used, but there already is one being used for DefaultErrorHandler.
The logical change would be to convert this DefaultErrorHandler to a CompositeErrorHandler, so we can keep the contents of it in the manifest; otherwise, they will need to be copied over as Python, which can add complexity later down the road, as mentioned here.
This PR should allow us to use a CustomErrorHandler in the CompositeErrorHandler, so we can simply do this instead of copying them over:
Summary by CodeRabbit
New Features
Tests