Repository navigation
fix(low-code): parse manifest and component schema with libyaml CSafeLoader when available - #1223
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting 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/1791499901-yaml-csafeloader#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/1791499901-yaml-csafeloaderPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)5 854 tests 5 823 ✅ 5m 44s ⏱️ Results for commit 766f1d3. ♻️ This comment has been updated with latest results. |
PyTest Results (Full)5 857 tests 5 826 ✅ 11m 3s ⏱️ Results for commit 766f1d3. ♻️ This comment has been updated with latest results. |
The issue this PR resolves has one addition, under "Proposed change" and "Acceptance criteria" of airbytehq/airbyte-internal-issues#17932. Please update the PR for it. Keep today's message for a YAML syntax error When the C loader raises a Why: the C loader's message keeps line and column but drops the source-line snippet when the YAML is read from a string, which is how a connector image reads its manifest ( Example: a list item indented one space shortPure-Python loader, today: C loader: Please cover:
The "Behavior notes for reviewers" line in the description, which says the snippet is omitted, will need updating too. |
|
Patrick Nilan (@pnilan) re: Keep today's message for a YAML syntax error (#1223 (comment)). Done in 4864013. When the C loader raises a
Parsing valid YAML never touches the pure-Python loader. New tests in
I updated the "Behavior notes" section of the PR description to match. |
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI Agent.
Review at 4864013c: fix before merge (not breaking)
The parser change is safe for existing connectors. All 539 manifest.yaml files in airbytehq/airbyte, plus the component schema, parse to identical objects with SafeLoader and CSafeLoader. The comparison covered str, bytes and file input, and checked types, key order and anchor sharing, on PyYAML 6.0.2 and 6.0.3 with Python 3.11, 3.13 and 3.14.
The items below are the title, edge cases in the new helper, and claims in the docstring and the description that aren't accurate. Please address 1–6. Item 7 is lower priority.
1. Retitle the PR (P1, blocks merge)
perf isn't an allowed type in .github/workflows/semantic_pr_check.yml:27-41, and Validate PR title is a required check on main. It reports pass today only because both of its steps skip on drafts, so the PR will turn red when it's marked ready. fix gets the same patch bump from the release drafter.
fix(low-code): parse manifest and component schema with libyaml CSafeLoader when available
Also fix the description's "semantic-release sets the CDK version from the PR title". The release drafter proposes a version from the title type, and a maintainer publishes the release (docs/RELEASES.md).
2. Re-parse outside the except block (P2)
Where: airbyte_cdk/utils/yaml_loader.py:42-48
The SafeLoader re-parse runs inside except yaml.YAMLError:, so the error it raises carries libyaml's error as __context__. str(error) is unchanged. But traceback.format_exc() at cli/source_declarative_manifest/_run.py:121 and :235 now prints libyaml's error first, then "During handling of the above exception, another exception occurred". For the misindented example in the tests, the traceback grows from 46 to 70 lines. That puts libyaml's wording back into the trace's stack_trace, which the fallback exists to avoid.
Fix: exit the except block, then rewind and re-parse after it (see the module under item 3). Add assert exc_info.value.__context__ is None to the string and file message tests.
3. Send non-seekable streams, mock handles and str/bytes subclasses straight to SafeLoader (P2)
Where: airbyte_cdk/utils/yaml_loader.py:15-25 and :45-47
- Non-seekable streams. A FIFO or
/dev/stdinpassed to--manifest-pathgets libyaml's verdict, even on inputSafeLoaderaccepts.description: "\ud83d\ude00 emoji"uses the escapejson.dumpsemits for emoji. It parsed before; it now raisesScannerError. - Mock handles. A
MagicMockormock_openhandle passes the duck-typedseekable()check, and itsseekdoes nothing.SafeLoaderthen re-reads an exhausted buffer, so a YAML syntax error returnsNone, and_parse_manifest_from_filereports "Manifest file at … is empty". - Subclasses. A
strsubclass raisesTypeError, whereyaml.safe_loadparsed it. The C parser accepts only exactstrandbytes, andTypeErrorisn't aYAMLError, so the fallback never runs.
This supersedes the earlier request in #1223 (comment) to raise the C loader's error for streams that can't be rewound. Parsing those streams with SafeLoader from the start gives today's message without a second parse.
Suggested airbyte_cdk/utils/yaml_loader.py (covers items 2, 3, 4 and 7)
#
# Copyright (c) 2025 Airbyte, Inc., all rights reserved.
#
from __future__ import annotations
from typing import IO, Any, Optional, Union, cast
import yaml
def _get_safe_loader() -> Union[type[yaml.SafeLoader], type[yaml.CSafeLoader]]:
"""Return libyaml's CSafeLoader when PyYAML was built with it, else the pure-Python SafeLoader."""
# The annotations stay unevaluated: yaml.CSafeLoader does not exist without libyaml.
return cast(
"Union[type[yaml.SafeLoader], type[yaml.CSafeLoader]]",
getattr(yaml, "CSafeLoader", yaml.SafeLoader),
)
def _tell_if_seekable(stream: Union[IO[str], IO[bytes]]) -> Optional[int]:
"""Return the stream's current position if it can be rewound, else None."""
try:
if stream.seekable() is not True:
return None
position = stream.tell()
except (AttributeError, OSError, ValueError):
return None
return position if isinstance(position, int) else None
def safe_load_yaml(stream: Union[str, bytes, IO[str], IO[bytes]]) -> Any:
"""Parse trusted YAML like yaml.safe_load, using libyaml's CSafeLoader when available.
Results match yaml.safe_load for documents both loaders accept. libyaml accepts
some input SafeLoader rejects (for example a tab after `key:`), ignores constructors
and resolvers registered on yaml.SafeLoader, and can crash the process on very deeply
nested input, so do not use this for untrusted input. When libyaml raises a
YAMLError, the input is parsed again with SafeLoader so callers see SafeLoader's error.
"""
loader = _get_safe_loader()
exact_text = type(stream) in (str, bytes)
rewind_to = None
if not exact_text and not isinstance(stream, (str, bytes)):
rewind_to = _tell_if_seekable(stream)
# str/bytes subclasses, non-seekable streams and mocks keep today's behaviour.
if loader is yaml.SafeLoader or (not exact_text and rewind_to is None):
return yaml.load(stream, Loader=yaml.SafeLoader)
try:
return yaml.load(stream, Loader=loader)
except yaml.YAMLError:
pass # Re-parse outside the handler so libyaml's error is not chained.
if rewind_to is not None:
cast(Union[IO[str], IO[bytes]], stream).seek(rewind_to)
return yaml.load(stream, Loader=yaml.SafeLoader)Checked against the current helper:
- It gives the same results and error messages as
yaml.safe_loadforstr,bytes, file,BytesIO,str-subclass and non-seekable input. - With a
mock_openhandle it raisesParserError, asyaml.safe_loaddoes; the current helper returnsNone. - No error carries a
__context__. - A successful parse never creates a
SafeLoader. - It imports and parses with
yaml.CSafeLoaderremoved. - It passes mypy with the repo's
mypy.ini.
Tests: replace test_syntax_error_in_non_seekable_stream_keeps_c_loader_message (unit_tests/utils/test_yaml_loader.py:164). On the current head, the first four tests below fail; with the suggested module, all five pass. They reuse the file's MISINDENTED_YAML and requires_libyaml, and need from unittest import mock.
Suggested tests
ESCAPED_EMOJI_YAML = 'description: "\\ud83d\\ude00 emoji"\n'
class _NonSeekable(io.StringIO):
def seekable(self) -> bool:
return False
@requires_libyaml
def test_syntax_error_does_not_chain_c_loader_error() -> None:
with pytest.raises(yaml.YAMLError) as exc_info:
safe_load_yaml(MISINDENTED_YAML)
assert exc_info.value.__context__ is None
@requires_libyaml
def test_non_seekable_stream_parses_and_errors_like_safe_loader() -> None:
assert safe_load_yaml(_NonSeekable(ESCAPED_EMOJI_YAML)) == yaml.load(
_NonSeekable(ESCAPED_EMOJI_YAML), Loader=yaml.SafeLoader
)
with pytest.raises(yaml.YAMLError) as expected:
yaml.load(_NonSeekable(MISINDENTED_YAML), Loader=yaml.SafeLoader)
with pytest.raises(yaml.YAMLError) as actual:
safe_load_yaml(_NonSeekable(MISINDENTED_YAML))
assert str(actual.value) == str(expected.value)
@requires_libyaml
def test_mock_open_handle_with_invalid_yaml_raises() -> None:
with pytest.raises(yaml.YAMLError):
safe_load_yaml(mock.mock_open(read_data=MISINDENTED_YAML)())
def test_str_subclass_parses() -> None:
class _Str(str):
pass
assert safe_load_yaml(_Str("a: 1")) == {"a": 1}
@requires_libyaml
def test_c_loader_accepts_tab_separators_that_safe_loader_rejects() -> None:
with pytest.raises(yaml.scanner.ScannerError):
yaml.load("a:\tb", Loader=yaml.SafeLoader)
assert safe_load_yaml("a:\tb") == {"a": "b"}The last test belongs to item 4. It pins current behaviour, so it passes on both versions.
4. Correct the equivalence claims (P2)
Where: airbyte_cdk/utils/yaml_loader.py:29 and the PR description
"Equivalent to yaml.safe_load" isn't true, and the description repeats it: "parsed objects, exception classes and error messages are the same as before". The fallback runs only when libyaml fails, so wherever libyaml succeeds, the result can differ:
- Accepted now, rejected before:
- tab separators:
a:\tb→{'a': 'b'}, plus[a,\tb]anda: 1\t# c - the
|#block header and[a?b] - nesting from about 500 to about 25,000 levels
- tab separators:
- Different value:
a: !gives''instead ofNone.- A U+FEFF at the start of a later line is dropped.
- Constructors and resolvers registered on
yaml.SafeLoaderare ignored, becauseCSafeLoaderdoesn't inherit from it.
- Different exception:
- A lone surrogate in a
strraisesUnicodeEncodeErrorinstead ofReaderError. - Nesting of about 25,000 levels or more segfaults (exit 139, no trace message) instead of raising
RecursionError.
- A lone surrogate in a
None of these appear in any shipped manifest, and none of the five call sites passes such input, so this isn't breaking. It matters for the next caller. resolvers/config_components_resolver.py:210 returns tab-containing config values unparsed by matching SafeLoader's tab error. Moving that call to this helper would turn "key:\tvalue" into {'key': 'value'}.
Fix:
- Use the docstring in the suggested module above.
- Add the tab test from the suggested tests.
- Update "Behavior notes for reviewers" and "Breaking change evaluation" in the description. List these differences and the 539/539 parity result.
5. Make the test module collectable without libyaml (P2)
Where: unit_tests/utils/test_yaml_loader.py:74
yaml.CSafeLoader in the module-level parametrize is evaluated at import. Without libyaml, collection fails with AttributeError before any skipif applies. All 10 tests are lost, including test_falls_back_to_pure_python_loader_without_libyaml, the one test written for that environment. CI doesn't catch it, because every leg installs a libyaml wheel.
pytest.param(
getattr(yaml, "CSafeLoader", None),
marks=pytest.mark.skipif(not yaml.__with_libyaml__, reason="PyYAML was built without libyaml"),
),6. Tighten the parity and malformed-YAML tests (P2)
Where: unit_tests/utils/test_yaml_loader.py:65 and :82
assert pure_python_schema == c_loader_schematreatsTrue == 1 == 1.0as equal and ignores key order. Comparerepr()of both results, or walk them and check types.test_malformed_yaml_raises_parser_error_with_both_loaderscallsyaml.loaddirectly, so no change tosafe_load_yamlcan make it fail. Send the malformed input throughsafe_load_yaml.
7. Make get_safe_loader private and type it (P3)
Where: airbyte_cdk/utils/yaml_loader.py:10, :15 and :23
docs/generate.py publishes every non-underscore module in the pdoc reference, so get_safe_loader becomes public API, although it exists for tests.
- Rename it to
_get_safe_loaderand give it a real return type instead ofAny. - Type
_tell_if_seekable's parameter, so itscastgoes away. - Keep the new annotation lazy.
yaml.CSafeLoaderdoesn't exist without libyaml, sotype[yaml.CSafeLoader]in an evaluated annotation, or in acastwhose type isn't a string, raises at import. The suggested module usesfrom __future__ import annotationsand a stringcast. - Update the two tests that import
get_safe_loader.
Leave unchanged
airbyte_cdk/test/standard_tests/declarative_sources.py:76stays onyaml.safe_load. It's the strict check that keeps manifests that only parse under libyaml out of the monorepo.config_components_resolver.py:204andparametrized_components_resolver.py:122stay onyaml.safe_load, for the reason in item 4.
CI notes
Check: destination-motherduckfails because the MotherDuck token was rejected ("Your request is not authenticated"). It's also red on #1233 and #1229, so it isn't caused by this PR.Skipped Check: source-google-drivedidn't run.Validate PR titlehasn't evaluated the title yet, because the PR is a draft.- Of the six
Test Connectorsentries, onlysource-intercomandsource-pokeapireach the changed parse path. - Still open from the issue: the measured before/after row for the table in airbytehq/airbyte-internal-issues#17920.
|
Patrick Nilan (@pnilan), about your review at
I left the callers you listed under "Leave unchanged" alone. Checks run locally:
The benchmark row for https://github.com/airbytehq/airbyte-internal-issues/issues/17920 is still open. |
There was a problem hiding this comment.
🔵 Needs a closer look
Documented parsing differences and deep-nesting crashes require maintainer acceptance and confirmation that updated entry points receive trusted input.
No open issues 🎉
What changed in this PR
Speeds up declarative connector startup by using libyaml for manifest and component-schema parsing when available.
Changes:
- Adds a shared loader with pure-Python fallback and error-message preservation.
- Updates five parsing call sites.
- Adds tests for loader selection, compatibility, fallback behavior, and integrations.
| File | Description |
|---|---|
| unit_tests/utils/test_yaml_loader.py | Tests loader behavior and integrations. |
| airbyte_cdk/utils/yaml_loader.py | Implements the shared YAML loader. |
| airbyte_cdk/sources/declarative/yaml_declarative_source.py | Updates manifest parsing. |
| airbyte_cdk/sources/declarative/concurrent_declarative_source.py | Updates component-schema parsing. |
| airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py | Updates legacy component-schema parsing. |
| airbyte_cdk/cli/source_declarative_manifest/_run.py | Updates CLI manifest-file parsing. |
Review effort: Balanced
…eLoader when available Co-Authored-By: bot_apk <apk@cognition.ai>
…r fails Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
2023244 to
766f1d3
Compare
Summary
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17932:
Close on merge: yes — internally filed, no customer to confirm with.
Part of epic https://github.com/airbytehq/airbyte-internal-issues/issues/17920. Requested by Patrick Nilan (Patrick Nilan (@pnilan)).
At startup, every manifest-only connector parses its
manifest.yamland the CDK's ~5.9k-linedeclarative_component_schema.yamlwith PyYAML's pure-PythonSafeLoader. This PR parses both with libyaml'sCSafeLoaderwhen PyYAML was built with libyaml, and falls back toSafeLoaderotherwise (without libyaml, accessingyaml.CSafeLoaderraisesAttributeError).New helper in
airbyte_cdk/utils/yaml_loader.py:Call sites switched to
safe_load_yaml:YamlDeclarativeSource._read_and_parse_yaml_fileandYamlDeclarativeSource._parseconcurrent_declarative_source._get_declarative_component_schemacli/source_declarative_manifest/_run._parse_manifest_from_file(--manifest-path)legacy/.../manifest_declarative_source._get_declarative_component_schema. The issue doesn't name this one, but it is the same schema load, so I changed it too.Left unchanged, because the issue puts them out of scope: the small-input
yaml.safe_loadcalls inconfig_components_resolver,parametrized_components_resolverandmigrations_registry, plusconnector.pyand the test utilities.Measured locally (linux/amd64, CDK poetry env, PyYAML 6.0.2,
yaml.__with_libyaml__ == True)SafeLoaderCSafeLoaderdeclarative_component_schema.yamlBoth loaders produce equal objects. This PR does not add the before/after row to the benchmark table in the epic (https://github.com/airbytehq/airbyte-internal-issues/issues/17920). Patrick Nilan (@pnilan), that row still needs to be measured and added.
Behavior notes for reviewers
Parity on real manifests. In Patrick Nilan (@pnilan)'s review, all 539
manifest.yamlfiles inairbytehq/airbyte, plus the component schema, parsed to identical objects withSafeLoaderandCSafeLoader. The comparison checked types, key order and anchor sharing acrossstr,bytesand file input, on PyYAML 6.0.2/6.0.3 and Python 3.11/3.13/3.14.Syntax errors. If libyaml raises a
yaml.YAMLError, the input is parsed again withSafeLoaderand that error is raised, without libyaml's error chained as__context__. Error type, message (including the source-line snippet for string input) and traceback match what callers saw before this PR. Seekable streams are rewound first.Streams that can't seek, mock handles and
str/bytessubclasses go straight toSafeLoader. The C parser accepts only exactstr/bytes, and these streams couldn't be parsed a second time.Not fully equivalent to
yaml.safe_load. When libyaml succeeds, a few inputs give results that differ fromSafeLoader's. None of them occur in a shipped manifest:SafeLoaderrejected:a:\tb→{'a': 'b'},[a,\tb],a: 1\t# c)|#block header and[a?b]a: !gives''instead ofNoneyaml.SafeLoaderare ignored, becauseCSafeLoaderdoesn't inherit from itstrraisesUnicodeEncodeErrorinstead ofReaderErrorRecursionErrorThe docstring says to use the helper only for trusted input. For that reason these stay on
yaml.safe_load:config_components_resolverandparametrized_components_resolver: they rely onSafeLoader's tab error.test/standard_tests/declarative_sources.py: it's the strict check that keeps libyaml-only manifests out of the monorepo._get_safe_loaderis private, so pdoc doesn't publish it.Test Coverage
unit_tests/utils/test_yaml_loader.py:Tests confirmed to fail on the old code:
test_declarative_call_sites_use_c_loader: checks that all five call sites useyaml.CSafeLoader. It fails with the old call sites (assert 0 == 1).SafeLoaderparse (str(error), type,__context__ is None):test_syntax_error_in_string_matches_pure_python_message(also runs throughYamlDeclarativeSource._parse)test_syntax_error_in_file_matches_pure_python_messagetest_syntax_error_does_not_chain_c_loader_errortest_non_seekable_stream_parses_and_errors_like_safe_loadertest_mock_open_handle_with_invalid_yaml_raisestest_str_subclass_parsesOther tests:
test_successful_parse_never_uses_pure_python_loader: a spy onyaml.SafeLoaderis never created for valid str, bytes or file input.test_pure_python_result_returned_if_only_c_loader_failstest_c_loader_accepts_tab_separators_that_safe_loader_rejects: pins one of the known differences.test_component_schema_parses_identically_with_both_loaders: compares with==andrepr(types, key order).test_malformed_yaml_raises_parser_error_with_c_loaderand..._without_libyamltest_falls_back_to_pure_python_loader_without_libyamlandtest_get_safe_loader_uses_c_loader_when_availabledel yaml.CSafeLoader: 3 passed, 12 skipped.Run locally:
ruff check,ruff formatandmypy: clean.test_yaml_loader.py,test_yaml_declarative_source.py,test_concurrent_declarative_source.py,unit_tests/source_declarative_manifestandunit_tests/legacy: 266 passed, 1 skipped.unit_tests/sources/declarativeon the first revision: 2345 passed.Declarative-First Evaluation
Not applicable: this changes the CDK framework, not a declarative connector's manifest or custom components.
Breaking change evaluation
Not breaking:
The differences listed above only affect input that no shipped manifest uses. The release drafter proposes a patch version from the
fixtitle type, and a maintainer publishes the release (docs/RELEASES.md).Link to Devin session: https://app.devin.ai/sessions/aa6c3fbe780246f99b7658fa21e285ac
Open in Devin Desktop: https://app.devin.ai/desktop/session/aa6c3fbe780246f99b7658fa21e285ac?variant=devin
Note
Auto-merge may have been disabled. Please check the PR status to confirm.