feat(low-code): add use check availability flag to dynamic check #293
Conversation
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Hey there! 👋 I noticed you've added a neat feature for controlling stream availability checks. Quick question: have you considered adding some documentation about the new 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (2)
Line range hint
228-233: The implementation looks solid! Would you consider a few enhancements? 🤔The recursive resolution of complex types is well implemented. Here are some suggestions that might make it even better:
- Maybe we could add type hints to the dictionary values? Something like:
- field_type["items"] = ( + field_type["items"]: Mapping[str, Any] = (
- Would you consider adding a docstring to explain the recursive nature? For example:
def _resolve_complex_type(self, complex_type: ComplexFieldType) -> Mapping[str, Any]: """Resolves a complex field type into its corresponding Airbyte type definition. For array types, recursively resolves the items property which can be either: - A simple type string (e.g., "string", "integer") - Another ComplexFieldType for nested arrays Args: complex_type: The complex field type to resolve Returns: A mapping containing the resolved Airbyte type definition """What do you think? 🙂
Line range hint
228-233: Would you like help with test cases for the recursive type resolution? 🧪To ensure the robustness of the complex type resolution, we might want to add test cases for:
- Simple array types (e.g., array of strings)
- Nested array types (e.g., array of arrays)
- Edge cases (e.g., invalid item types)
I'd be happy to help write these tests if you'd like! What do you think? 😊
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py (3)
124-125: Consider using more descriptive test case IDs.The test case IDs could be more descriptive to better indicate what's being tested, wdyt?
- pytest.param(200, True, True, [], id="test_stream_available"), - pytest.param(200, True, False, [], id="test_stream_available"), + pytest.param(200, True, True, [], id="test_stream_available_with_availability_check"), + pytest.param(200, True, False, [], id="test_stream_available_bypass_availability_check"),
153-155: Consider adding test cases for edge cases.Would it be helpful to add test cases for edge scenarios? For example:
- When stream count is 0
- When use_check_availability is False but stream count is 0
@pytest.mark.parametrize( "stream_count, use_check_availability, expected_messages", [ pytest.param(0, True, ["No streams to connect to"], id="test_no_streams_with_availability_check"), pytest.param(0, False, ["No streams to connect to"], id="test_no_streams_bypass_availability_check"), ], ) def test_check_dynamic_stream_edge_cases(stream_count, use_check_availability, expected_messages): # Test implementation
108-134: How about making the test IDs more descriptive?The test cases look comprehensive! Would you consider making the test IDs more descriptive by including the
use_check_availabilityvalue? For example:id="test_stream_available_with_check_enabled" # for use_check_availability=True id="test_stream_available_with_check_disabled" # for use_check_availability=FalseThis could make it easier to understand test results at a glance. What do you think? 🤔
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
906-914: Consider adding a descriptive assertion message.The assertion could benefit from an error message to help developers understand what went wrong, wdyt?
- assert model.use_check_availability is not None # for mypy + assert model.use_check_availability is not None, "use_check_availability must be defined in CheckDynamicStream" # for mypyairbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
323-327: LGTM! Would you consider enhancing the description? 🤔The schema changes look good and align well with the PR objectives. The property is well-defined with all required fields. However, would you consider making the description more informative by explaining why and when this flag would be set to false? Something like:
"Enables stream check availability. When set to false, the connection check will be bypassed if the streams have already been successfully created. This field is automatically set by the CDK."
wdyt?
airbyte_cdk/sources/declarative/checks/check_dynamic_stream.py (1)
26-26: Consider enhancing the class documentation to include the new attribute?The new
use_check_availabilityflag looks good! Would you consider adding it to the class docstring under Attributes to help other developers understand its purpose? Something like:Attributes: stream_count (int): numbers of streams to check + use_check_availability (bool): when True, checks stream availability by fetching data. When False, skips availability check if streams are created successfully.wdyt? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/checks/check_dynamic_stream.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)airbyte_cdk/sources/declarative/requesters/error_handlers/http_response_filter.py(1 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(1 hunks)unit_tests/sources/declarative/checks/test_check_dynamic_stream.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/declarative/requesters/error_handlers/http_response_filter.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
62-66: LGTM! Well-documented schema changes.The schema definition for
use_check_availabilityis well-structured with:
- Clear description
- Appropriate default value
- Good title
airbyte_cdk/sources/declarative/checks/check_dynamic_stream.py (2)
38-40: LGTM! Early return looks good.The early return when
use_check_availabilityis False is well-placed and maintains the expected return type signature. Nice work! 👍
51-56: Error handling improvements look great!Including the stream name in the error message and using exc_info=True will make debugging much easier. Excellent attention to detail! 🎯
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py (2)
138-155: Test implementation looks solid!Great job on the test implementation! Using
deepcopyfor the manifest and conditionally adjusting the request count based onuse_check_availabilityis a clean approach. The HTTP mock setup is clear and well-structured. 👏
166-168: Assertions are thorough and well-placed!The request count assertions effectively validate the behavior of
use_check_availability. Nice touch checking both item and items requests! 💯
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py (2)
108-133: Consider adding edge cases to improve test coverage?The test cases look good! To make them even more robust, what do you think about adding these scenarios?
- Error responses (500, 502, etc.) with
use_check_availability=False- Empty response with status 200
- Malformed JSON response
wdyt?
147-150: Consider extracting the item ID as a constant?The URL "/items/1" uses a magic number. What do you think about making it more explicit? Something like:
+TEST_ITEM_ID = 1 -item_request = HttpRequest(url="https://api.test.com/items/1") +item_request = HttpRequest(url=f"https://api.test.com/items/{TEST_ITEM_ID}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py (4)
135-137: LGTM! Clear and well-structured function signature.The parameter names are descriptive and follow a consistent pattern.
141-145: LGTM! Clean HTTP mock setup.The request and response setup is clear and well-structured.
152-154: LGTM! Clear conditional logic.The feature flag handling is straightforward and the comment explains the behavior well.
165-166: LGTM! Good verification of request count.The assertion ensures the feature flag correctly controls whether the request is made.
|
/autofix
|
What
Check availability by attempting to fetch the first slice of data during the connection check. However, for some dynamic streams, this slice may be empty, leading to a failed connection check even if the stream has been created and we have sufficient permissions to access data (e.g.,
source-google-sheetswill create a stream if there is at least one row in the sheet, but it won’t create a slice if there are no additional rows).How
Add a
use_check_availabilityflag to bypass the connection check if streams have been successfully created.Summary by CodeRabbit
New Features
use_check_availabilityoption to control stream availability checks.Improvements
Bug Fixes