feat: skip config validation during discovery for sources with DynamicSchemaLoader #467
Conversation
…cSchemaLoader Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
…ation control Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
…class Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
…ver property Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
Co-Authored-By: Aaron <AJ> Steers <aj@airbyte.io>
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py:163
- [nitpick] The final duplicate assertions for 'check_config_during_discover' and 'check_config_against_spec' are redundant. Consider removing the duplicate assertions to streamline the test.
assert source.check_config_during_discover is False
📝 WalkthroughWalkthroughThe changes introduce a new boolean attribute Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI"
participant EP as "Entrypoint"
participant SRC as "Source"
CLI->>EP: Execute `discover` command (with/without config)
EP->>EP: Parse arguments (config now optional)
alt Config not provided & check_config_during_discover is False
EP->>EP: Create empty configuration dictionary
end
EP->>SRC: Invoke `discover(empty_config)`
SRC->>EP: Return catalog/messages
EP->>CLI: Yield results
sequenceDiagram
participant MDS as "ManifestDeclarativeSource"
participant CFG as "Source Config"
MDS->>MDS: Call _uses_dynamic_schema_loader() and inspect streams in CFG
alt DynamicSchemaLoader found
MDS->>MDS: Set check_config_during_discover = True
else No DynamicSchemaLoader found
MDS->>MDS: Set check_config_during_discover = False
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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:
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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
447-477: Well-implemented detection logicThe implementation of
_uses_dynamic_schema_loader()thoroughly checks both regular streams and dynamic streams for the presence of aDynamicSchemaLoader. The documentation clearly explains the purpose and behavior of this method.One minor suggestion: would adding a simple caching mechanism be beneficial if this method gets called multiple times? wdyt?
unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (4)
76-128: Thorough testing of discovery with DynamicSchemaLoader.The test effectively mocks the
streamsmethod and validates that discovery works with an empty config when using DynamicSchemaLoader. Should we also add a validation test that explicitly verifies the config validation is being skipped? Perhaps by patchingcheck_config_against_spec_or_exitand asserting it's not called? wdyt?
130-135: Docstring doesn't fully match test implementation.The docstring states "Test that discovery validates config when DynamicSchemaLoader is not used," but the test doesn't actually verify any validation occurs. Consider updating the docstring or adding validation verification logic?
- """Test that discovery validates config when DynamicSchemaLoader is not used.""" + """Test that discovery works with a static schema loader with minimal config."""
13-13: Imported but unused utility.The
check_config_against_spec_or_exitis imported but never used in the tests. If the intent was to verify validation behavior, consider adding tests that mock this function and assert whether it's called or not based on the schema loader type.For example, you might add a test like:
@patch("airbyte_cdk.sources.utils.schema_helpers.check_config_against_spec_or_exit") def test_validation_skipped_with_dynamic_schema_loader(mock_check_config): # Setup source with DynamicSchemaLoader # Perform discovery # Assert mock_check_config.assert_not_called()
167-171: Additional assertions at the end of the test are redundant.Lines 173-174 repeat assertions already made at lines 163-164. Consider removing the redundant assertions to improve test clarity, or if they're meant to verify something different after the discovery call, add a comment explaining the purpose.
assert isinstance(catalog, AirbyteCatalog) assert len(catalog.streams) == 1 assert catalog.streams[0].name == "test_static_stream" - assert source.check_config_during_discover is False - assert source.check_config_against_spec is True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/connector.py(1 hunks)airbyte_cdk/entrypoint.py(3 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(3 hunks)unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py(1 hunks)unit_tests/test_entrypoint.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (4)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
ManifestDeclarativeSource(61-477)streams(147-172)airbyte_cdk/sources/utils/schema_helpers.py (1)
check_config_against_spec_or_exit(168-186)airbyte_cdk/sources/abstract_source.py (2)
discover(85-90)streams(74-79)airbyte_cdk/entrypoint.py (1)
discover(239-248)
airbyte_cdk/entrypoint.py (3)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
discover(171-177)airbyte_cdk/test/entrypoint_wrapper.py (1)
discover(182-199)airbyte_cdk/sources/source.py (1)
discover(48-52)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (13)
unit_tests/test_entrypoint.py (1)
181-181: Update test to match removed config requirementThe test has been updated to remove "discover" from the required_args dictionary, which correctly aligns with the changes made to the entrypoint making the --config parameter optional for the discover command.
airbyte_cdk/connector.py (1)
35-42: New attribute and documentation looks good!Adding docstrings for the existing
check_config_against_specattribute and introducing the newcheck_config_during_discoverattribute with clear documentation improves the code readability. The default value ofFalseensures backward compatibility.airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
113-113: Good initialization of the new attributeSetting
check_config_during_discoverbased on the detection of aDynamicSchemaLoaderduring initialization ensures the behavior is determined early and consistently.airbyte_cdk/entrypoint.py (5)
96-96: Parameter updated to match the new featureChanging the
--configparameter from required to optional is the key change that enables discovery without authentication when not needed.
144-148: Style improvement with generator expressionConverting the message yielding process from a list comprehension to a generator expression is a nice optimization that prevents creating an intermediate list.
149-163: Core implementation of the new featureThis conditional block elegantly handles the case when discovery is requested without a config file and the source doesn't require config validation. The empty config dictionary approach keeps the code clean and avoids unnecessary code duplication.
168-171: Consistent style improvementAnother instance of converting from list comprehension to generator expression for more efficient message yielding.
243-243: Updated condition for config validationThe condition has been updated to use the new
check_config_during_discoverattribute instead ofcheck_config_against_spec, which aligns perfectly with the new feature.unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (5)
5-5: Clear and concise docstring summary.The docstring effectively summarizes the purpose of these tests, which aligns with the PR goal of skipping config validation during discovery for sources using DynamicSchemaLoader.
9-14: Good imports organization.The imports are well-organized and include all necessary components for testing the feature.
16-49: Well-structured test for DynamicSchemaLoader detection.This test correctly verifies that when a source uses DynamicSchemaLoader, the
check_config_during_discoverflag is set to True, while maintainingcheck_config_against_specas True. The detailed configuration setup provides good context for understanding how DynamicSchemaLoader is detected.
51-74: Strong coverage of the negative case.This test properly verifies the inverse condition - when InlineSchemaLoader is used instead of DynamicSchemaLoader, the
check_config_during_discoverflag is correctly set to False. This ensures comprehensive logic coverage.
133-172: Comprehensive test with static schema loader.This test confirms that discovery works with a static schema loader and an empty config. It effectively verifies the catalog structure and stream properties.
| assert isinstance(catalog, AirbyteCatalog) | ||
| assert len(catalog.streams) == 1 | ||
| assert catalog.streams[0].name == "test_dynamic_stream" |
There was a problem hiding this comment.
This should be failing. Probably failing to fail because we're mocking.
Replaces:
Allow Airbyte sources to run discovery unprivileged with DynamicSchemaLoader
This PR allows Airbyte sources to run discovery unprivileged if the source API doesn't need auth in order to provide the catalog info.
Implementation
Added automatic detection of
DynamicSchemaLoaderto skip config validation during discovery. When a source uses the dynamic schema feature, we assume that the schema endpoint might not require authentication and automatically skip config validation during discovery.Modified the AirbyteEntrypoint to make the
--configparameter optional for the discover command. This allows running discovery without providing a config file when the source doesn't require config validation.This enables sources with dynamic schemas to provide catalog information without authentication when the schema endpoint doesn't require it.
Testing
Added unit tests to verify that:
DynamicSchemaLoaderskip config validation during discoveryDynamicSchemaLoaderstill require config validationLink to Devin run: https://app.devin.ai/sessions/e6aa19df336347919b6cabcff7143a1c
Requested by: Aaron ("AJ") Steers (aj@airbyte.io)
Summary by CodeRabbit
New Features
Enhancements