-
Notifications
You must be signed in to change notification settings - Fork 31
fix(low-code): handle all ScannerError exceptions in ConfigComponentsResolver #854
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(low-code): handle all ScannerError exceptions in ConfigComponentsResolver #854
Conversation
…Resolver Previously, the _parse_yaml_if_possible method only caught ScannerError exceptions containing '%' characters, but re-raised other ScannerError types. This caused failures when strings containing tab characters were passed through the YAML parser, as tabs cannot start tokens in YAML. This fix catches all ScannerError exceptions and returns the original value unchanged, which is the expected behavior for strings that are not valid YAML. Fixes: airbytehq/oncall#10280 Co-Authored-By: unknown <>
Original prompt from API User |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 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@devin/1764137058-fix-scanner-error-tab-chars#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 devin/1764137058-fix-scanner-error-tab-charsHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Co-Authored-By: unknown <>
Per reviewer feedback, update the fix to specifically handle the tab character error case rather than catching all ScannerError exceptions. This maintains the existing pattern of handling specific error cases. Co-Authored-By: unknown <>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the ConfigComponentsResolver where YAML ScannerError exceptions caused by tab characters in configuration values (such as custom GAQL queries) would cause discovery to fail. The fix extends the exception handling in _parse_yaml_if_possible to catch tab-related ScannerError exceptions in addition to the existing % character case.
Key changes:
- Extended
ScannerErrorexception handling to include tab character errors - Added test case for tab character scenario
- Updated inline comment to reflect broader coverage
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py |
Added handling for tab character ScannerError with additional condition check |
unit_tests/sources/declarative/resolvers/test_config_components_resolver.py |
Added test manifest and test case for tab character scenario |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except ScannerError as e: # "%Y-%m-%d" or strings with tabs - not valid YAML | ||
| if "expected alphabetic or numeric character, but found '%'" in str(e): | ||
| return value | ||
| if "found character '\\t' that cannot start any token" in str(e): | ||
| return value | ||
| raise e |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern of checking specific error message strings and returning value for each case suggests a fragile approach. According to the PR description's root cause analysis, the intent is to catch ALL ScannerError exceptions since they indicate values that cannot be parsed as YAML. The current implementation still only catches two specific cases. Consider simplifying to except ScannerError: return value which would be more maintainable and align with the PR's stated goal of 'catch all ScannerError exceptions'.
| except ScannerError as e: # "%Y-%m-%d" or strings with tabs - not valid YAML | |
| if "expected alphabetic or numeric character, but found '%'" in str(e): | |
| return value | |
| if "found character '\\t' that cannot start any token" in str(e): | |
| return value | |
| raise e | |
| except ScannerError: # any ScannerError means not valid YAML | |
| return value |
fix(low-code): handle all ScannerError exceptions in ConfigComponentsResolver
Summary
This PR fixes a bug where custom GAQL queries containing tab characters (
\t) would cause discovery to fail with a YAMLScannerError. The_parse_yaml_if_possiblemethod inConfigComponentsResolverwas only catchingScannerErrorexceptions containing the%character, but re-raising all otherScannerErrortypes.The fix simplifies the exception handling to catch all
ScannerErrorexceptions and return the original value unchanged, which is consistent with howParserErroris already handled.Root cause: When strings containing tab characters are passed through
yaml.safe_load(), YAML raises aScannerErrorbecause tabs cannot start tokens in YAML. The previous code only handled the%character case.Related issue: https://github.com/airbytehq/oncall/issues/10280
Review & Testing Checklist for Human
ScannerErrorexceptions now instead of just the%character case. Confirm this won't mask legitimate YAML errors that should be surfaced to users.test_config_components_resolver.pyto ensure no regressionsScannerErrorscenarios that should be tested beyond%and tab characters?Recommended test plan:
pytest unit_tests/sources/declarative/resolvers/test_config_components_resolver.py -vNotes
/ai-triagecommand