fix(low-code): handle ScannerError in ConfigComponentsResolver
#614
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe changes introduce additional error handling in the YAML parsing logic within Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant Resolver as ConfigComponentsResolver
participant YAML as yaml.safe_load
Test->>Resolver: Call _parse_yaml_if_possible(value)
Resolver->>YAML: Try to parse value as YAML
YAML-->>Resolver: Raise ScannerError (if '%' found)
alt Error message contains "expected alphabetic or numeric character, but found '%'"
Resolver-->>Test: Return original string value
else Other ScannerError
Resolver-->>Test: Raise ScannerError
end
Note over Resolver: If no error, return parsed value
Would you like to see a comparison diagram of the old vs. new error handling flow as well, wdyt? ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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 (2)
airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py (1)
189-192: Solid targeted error handling approach!The implementation correctly catches the specific ScannerError for datetime format strings while preserving the original exception for other scanner errors. The string matching approach is appropriate for this targeted fix, wdyt?
One small consideration: should we log this specific case for debugging purposes, or is silent handling preferred here?
unit_tests/sources/declarative/resolvers/test_config_components_resolver.py (1)
148-160: Excellent test coverage for the new error handling!The test manifest correctly includes a component mapping with a Jinja expression containing datetime format strings that would trigger the ScannerError. This provides good coverage for the new exception handling logic, wdyt?
One formatting note from static analysis - could we add an extra blank line before line 160 to follow PEP 8 conventions?
} ) +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py(2 hunks)unit_tests/sources/declarative/resolvers/test_config_components_resolver.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#13
File: airbyte_cdk/connector.py:99-99
Timestamp: 2024-11-10T04:50:11.914Z
Learning: When a PR's goal is to run the autoformat task from `ruff`, avoid suggesting code changes beyond formatting to prevent potential negative side effects.
unit_tests/sources/declarative/resolvers/test_config_components_resolver.py (4)
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1093-1102
Timestamp: 2025-01-14T00:20:32.310Z
Learning: In the `airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py` file, the strict module name checks in `_get_class_from_fully_qualified_class_name` (requiring `module_name` to be "components" and `module_name_full` to be "source_declarative_manifest.components") are intentionally designed to provide early, clear feedback when class declarations won't be found later in execution. These restrictions may be loosened in the future if the requirements for class definition locations change.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py (1)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
🪛 Flake8 (7.2.0)
unit_tests/sources/declarative/resolvers/test_config_components_resolver.py
[error] 160-160: expected 2 blank lines, found 1
(E302)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py (1)
14-14: LGTM on the import addition!The ScannerError import is correctly added to support the new exception handling below.
unit_tests/sources/declarative/resolvers/test_config_components_resolver.py (1)
174-174: Perfect test case addition!Adding the scanner error manifest to the parameterized test ensures the new error handling path gets exercised and validates that no exceptions are raised while still producing the expected stream names.
darynaishchenko
deleted the
daryna/low-code/config-components-mapping-parsing-yaml
branch
For bing-ads custom reports we need to define cursor format based on report aggregation provided in the config by users.
Manifest will look like this:
When format string is being parsed in the
_parse_yaml_if_possibleScanner error is raised.Added check for the error message for datetime format case to pass this value to stream template.
Summary by CodeRabbit
Bug Fixes
Tests