feat(connector-builder): add full_resolve command to connector builder #442
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis pull request adds a new manifest resolution command by introducing the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Main
participant Handler
participant ManifestSource
Client->>Main: Send command "full_resolve_manifest" with source config
Main->>Handler: Invoke full_resolve_manifest(source)
alt Successful manifest resolution
Handler->>ManifestSource: Resolve manifest & dynamic streams
ManifestSource-->>Handler: Return resolved manifest
Handler->>Handler: Update streams (set dynamic_stream_name, append dynamic streams)
Handler-->>Main: Return AirbyteMessage(RECORD, manifest)
else Exception encountered
Handler->>Handler: Capture exception & create AirbyteTracedException
Handler-->>Main: Return AirbyteMessage(error)
end
Main-->>Client: Send response
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-full-resolve-command-to-builder-handler
|
/autofix
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
airbyte_cdk/connector_builder/main.py (1)
85-87: Good implementation of new command handling.The implementation follows the existing pattern for command handling and correctly calls the newly imported function. This enhances the connector builder's capability to fully resolve both static and dynamic streams in a manifest.
Have you considered adding a brief comment explaining what this command does? It might help future developers understand the purpose without needing to look at the implementation details. wdyt?
elif command == "resolve_manifest": return resolve_manifest(source) elif command == "test_read": assert ( catalog is not None ), "`test_read` requires a valid `ConfiguredAirbyteCatalog`, got None." return read_stream(source, config, catalog, state, limits) + # Fully resolves the manifest including both static and dynamic streams elif command == "full_resolve_manifest": return full_resolve_manifest(source) else: raise ValueError(f"Unrecognized command {command}.")airbyte_cdk/connector_builder/connector_builder_handler.py (1)
120-141: New function for resolving both static and dynamic streams - could use a docstringThe
full_resolve_manifestfunction looks well-implemented and follows the same pattern as the existingresolve_manifestfunction. Good error handling and type consistency!Would it be helpful to add a docstring explaining the purpose of this function and how it differs from
resolve_manifest? Something like:def full_resolve_manifest(source: ManifestDeclarativeSource) -> AirbyteMessage: """ Resolves both static and dynamic streams from a manifest source. Unlike resolve_manifest which only returns static streams, this function: 1. Resolves the manifest to get static streams 2. Marks them with dynamic_stream_name=None 3. Adds all dynamic streams from the source Returns an AirbyteMessage containing the complete manifest with both static and dynamic streams. """What do you think?
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
401-403: Nice implementation of dynamic stream naming with fallback!This ensures each dynamic stream has a unique identifier by using the index as a fallback when a name isn't provided in the dynamic definition. The use of an f-string makes this readable.
I do have one small suggestion - would using a meaningful prefix like "dynamic_stream_" be better than just "dynamic_stream_"? Or is this intentionally matching something elsewhere? wdyt?
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2052-2057: Good addition of the dynamic_stream_name field to DeclarativeStream!This optional field with default value "None" aligns well with the changes in manifest_declarative_source.py. It properly documents the relationship between dynamic and static streams.
One small note: the default value is "None" as a string rather than Python's None value. Is this intentional? It might be more consistent to use None (without quotes) if that's the semantic meaning here, wdyt?
2498-2500: Name field addition to DynamicDeclarativeStream looks good!This optional field provides a way to explicitly name dynamic streams, which works well with the fallback mechanism in manifest_declarative_source.py.
One minor observation: DeclarativeStream's dynamic_stream_name has default "None" while this name field has default empty string. Is this inconsistency intentional based on how they're used? Just checking if alignment between these defaults might be helpful, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/connector_builder/connector_builder_handler.py(1 hunks)airbyte_cdk/connector_builder/main.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(4 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(4 hunks)unit_tests/sources/declarative/resolvers/test_http_components_resolver.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2) (2)
_dynamic_stream_configs(355-427)resolved_manifest(113-114)
unit_tests/connector_builder/test_connector_builder_handler.py (3)
airbyte_cdk/connector_builder/models.py (2) (2)
HttpRequest(17-21)HttpResponse(10-13)airbyte_cdk/sources/declarative/manifest_declarative_source.py (2) (2)
ManifestDeclarativeSource(60-430)resolved_manifest(113-114)airbyte_cdk/connector_builder/main.py (1) (1)
handle_connector_builder_request(70-88)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
airbyte_cdk/sources/types.py (1) (1)
get(137-138)airbyte_cdk/test/mock_http/mocker.py (1) (1)
get(94-95)
🪛 GitHub Actions: Pytest (Fast)
unit_tests/connector_builder/test_connector_builder_handler.py
[error] 1777-1777: AssertionError: Expected resolved manifest does not match actual resolved manifest.
[warning] 36-36: PytestCollectionWarning: cannot collect test class 'TestReadLimits' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 85-85: PytestCollectionWarning: cannot collect test class 'TestStreamReader' because it has a init constructor.
[warning] 102-102: PytestCollectionWarning: cannot collect test class 'TestSpec' because it has a init constructor.
[warning] 135-135: PytestCollectionWarning: cannot collect test class 'TestFileContent' because it has a init constructor.
[warning] 261-261: PytestCollectionWarning: cannot collect test class 'TestServer' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestingCustomSubstreamPartitionRouter' because it has a init constructor.
[warning] 18-18: PytestCollectionWarning: cannot collect test class 'TestingSomeComponent' because it has a init constructor.
[warning] 44-44: PytestUnknownMarkWarning: Unknown pytest.mark.asyncio - is this a typo?
[warning] 88-88: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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)
🔇 Additional comments (13)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (3)
60-60: Addition of explicit dynamic stream name looks good.The new name parameter "TestDynamicStream" for the dynamic stream definition aligns well with the updated functionality in the manifest declarative source. This helps to test whether dynamic stream names are properly propagated through the system.
491-493: Good addition of dynamic stream name verification.This assertion effectively verifies that the dynamic stream name defined in the test manifest (line 60) is correctly passed through the system. This ensures the
_dynamic_stream_configsmethod is properly handling the named dynamic streams.
579-581: Verification for default dynamic stream name looks good.This test case ensures that when a manifest doesn't explicitly name a dynamic stream (as in the
_MANIFEST_WITH_HTTP_COMPONENT_RESOLVER_WITH_RETRIEVER_WITH_PARENT_STREAM), the default naming scheme using indices is applied correctly. Great coverage of the fallback scenario!airbyte_cdk/connector_builder/main.py (1)
15-15: New import for full_resolve_manifest looks good.The import is properly added alongside other related functions from the connector_builder_handler module.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1430-1436: New property for DeclarativeStream looks good!This addition of
dynamic_stream_nameto the DeclarativeStream definition provides a clear way to identify dynamic vs static streams. The default value of "None" makes sense as a indicator for static streams.Just a small suggestion - would it make sense to add more specific example values that demonstrate real-world usage patterns? Perhaps something like "DynamicTable_{{config.customer_id}}" to show interpolation? wdyt?
3776-3782: Looks good! Consistent with other property definitions.The addition of the
nameproperty to DynamicDeclarativeStream follows the same structure as other property definitions in the schema. Good to see consistent patterns being maintained.airbyte_cdk/connector_builder/connector_builder_handler.py (1)
124-126: Logic for combining static and dynamic streams looks goodThe approach for marking static streams (setting
dynamic_stream_nameto None) and then extending with dynamic streams is clean and straightforward.Have you verified that
source.dynamic_streamsalways returns a list that can be safely extended? If it could be None in some edge cases, we might need a fallback like:streams.extend(source.dynamic_streams or [])Just a thought - what do you think?
airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
109-109: Adding config instance variable looks good!This is a sensible addition to store the configuration provided to the constructor for later use in dynamic streams.
120-123: Good addition of the dynamic_streams property!This provides a convenient way to access dynamic streams directly from the source, similar to the existing
streamsproperty. Nice use of type annotations for better code clarity.
362-362: Using enumerate for dynamic definition indexing is a good approach!Adding the index to the loop provides a way to generate unique identifiers for dynamic streams. This is a clean Python pattern.
unit_tests/connector_builder/test_connector_builder_handler.py (3)
63-63: Good addition of HttpMocker for testingAdding HttpMocker, HttpRequest, and HttpResponse imports is a great choice for testing HTTP interactions without making actual network calls. This will make the tests more reliable and faster.
158-282: LGTM! Well-structured dynamic stream manifest definitionThe dynamic stream manifest extension looks comprehensive. It properly defines all the necessary components for testing dynamic streams including the stream template and components resolver with proper field mappings.
🧰 Tools
🪛 GitHub Actions: Pytest (Fast)
[warning] 261-261: PytestCollectionWarning: cannot collect test class 'TestServer' because it has a init constructor.
337-340: Good configuration for testing full_resolve_manifestThe configuration properly references the new dynamic stream manifest and sets the appropriate command for testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
unit_tests/connector_builder/test_connector_builder_handler.py (1)
1317-1779:⚠️ Potential issueFix the failing assertion
The pipeline is failing at line 1777 due to a mismatch in the expected vs. actual resolved manifest. There's ause_cachefield in the actual code that isn't present in theexpected_resolved_manifest. Would you like to update the expected dictionary to include"use_cache": True,"so that it matches the actual dynamic stream configuration? wdyt?Here's an example of how you could do it:
"requester": { "type": "HttpRequester", "url_base": "https://api.test.com", "path": "parent/{{ stream_partition.parent_id }}/items", "http_method": "GET", "authenticator": { "type": "ApiKeyAuthenticator", "header": "apikey", "api_token": "{{ config['api_key'] }}", }, + "use_cache": True, },🧰 Tools
🪛 GitHub Actions: Pytest (Fast)
[error] 1777-1777: AssertionError: Expected resolved manifest does not match actual resolved manifest. Differences in stream configurations.
🧹 Nitpick comments (3)
airbyte_cdk/connector_builder/connector_builder_handler.py (1)
120-140: New function looks good, but should we add some defensive programming?The implementation of
full_resolve_manifestfollows the pattern ofresolve_manifest, which is great for consistency. However, I have a few questions about potential edge cases:
- Should we verify each stream is a dictionary before setting
dynamic_stream_name?- Could we add validation for
source.dynamic_streamsto ensure it's not None?- Have you considered making a copy of the manifest before modifying it to avoid side effects?
These are just suggestions to make the code more robust - wdyt?
def full_resolve_manifest(source: ManifestDeclarativeSource) -> AirbyteMessage: try: manifest = source.resolved_manifest - streams = manifest.get("streams", []) + # Create a copy to avoid modifying the original manifest + manifest_copy = manifest.copy() if manifest else {} + streams = manifest_copy.get("streams", []) for stream in streams: - stream["dynamic_stream_name"] = None + # Ensure stream is a dictionary before modifying + if isinstance(stream, dict): + stream["dynamic_stream_name"] = None - streams.extend(source.dynamic_streams) + # Safely extend with dynamic streams if available + if hasattr(source, "dynamic_streams") and source.dynamic_streams: + streams.extend(source.dynamic_streams) return AirbyteMessage( type=Type.RECORD, record=AirbyteRecordMessage( - data={"manifest": manifest}, + data={"manifest": manifest_copy}, emitted_at=_emitted_at(), stream="full_resolve_manifest", ), )airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2052-2057: Example value format doesn't match field type - should this be a string instead of a list?The
dynamic_stream_namefield is defined asOptional[str]but the example is a list["Tables"]. This might be confusing for users.Would it make more sense to change the example to a string like
"Tables"to match the field type, wdyt?dynamic_stream_name: Optional[str] = Field( "None", description="The dynamic stream name that create current stream, if None is static stream.", - example=["Tables"], + example="Tables", title="Dynamic Stream Name", )Also, there's a small grammar issue in the description - perhaps:
- description="The dynamic stream name that create current stream, if None is static stream.", + description="The dynamic stream name that created the current stream, if None then it's a static stream.",
2498-2500: Same example format issue in the DynamicDeclarativeStream.name fieldSimilar to the previous comment, the example is a list
["Tables"]but the field is of typeOptional[str].Consider changing to a string example to match the field type:
name: Optional[str] = Field( "", description="The dynamic stream name.", example=["Tables"], title="Name" + "", description="The dynamic stream name.", example="Tables", title="Name" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/connector_builder/connector_builder_handler.py(1 hunks)airbyte_cdk/connector_builder/main.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(4 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(4 hunks)unit_tests/sources/declarative/resolvers/test_http_components_resolver.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2) (2)
_dynamic_stream_configs(355-427)resolved_manifest(113-114)
🪛 GitHub Actions: Pytest (Fast)
unit_tests/connector_builder/test_connector_builder_handler.py
[error] 1777-1777: AssertionError: Expected resolved manifest does not match actual resolved manifest. Differences in stream configurations.
[warning] 36-36: PytestCollectionWarning: cannot collect test class 'TestReadLimits' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestReader' because it has a init constructor.
[warning] 257-257: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestStreamReader' because it has a init constructor.
[warning] 102-102: PytestCollectionWarning: cannot collect test class 'TestSpec' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- 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 (11)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (3)
60-60: LGTM: Dynamic stream name is now explicitly defined in the test manifestThe addition of the explicit name "TestDynamicStream" in the manifest aligns with the new dynamic stream name feature being tested. This provides a clear test case for validating that the name property is correctly passed through to the stream configuration.
491-493: Good addition of test validation for dynamic stream nameThis verification ensures that the dynamic stream name is correctly propagated from the manifest definition to the stream configuration. It validates that when an explicit name is provided in the dynamic stream definition, it's properly assigned to the
dynamic_stream_nameproperty. Nice way to test the feature!
579-581: Helpful test for default dynamic stream naming patternThis assertion nicely complements the previous test by verifying the default naming pattern when no explicit name is provided in the dynamic stream definition. This ensures the fallback naming format (
dynamic_stream_{index}) works as expected. Good coverage of both explicit and implicit naming scenarios, wdyt?airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1430-1436: New propertydynamic_stream_namelooks good.This addition to the
DeclarativeStreamschema provides a clear way to track if a stream is dynamic or static. The default value ofNoneappropriately indicates a static stream by default. The description and example are helpful for users.
3776-3782: Thenameproperty for dynamic streams is well-defined.Good addition that complements the
dynamic_stream_nameproperty. This creates a clear relationship between dynamic streams and their names, following the schema patterns established elsewhere in the file.airbyte_cdk/connector_builder/main.py (2)
15-15: Import addition looks good.The import for
full_resolve_manifestis properly placed with the other related imports from the same module.
85-86: New command handler follows existing patterns.The implementation for the
full_resolve_manifestcommand follows the same structure as the other command handlers, making it consistent and easy to understand.airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
109-109: Looks good! Configuration initialization pattern.The initialization of
self._configusing theor {}pattern ensures we have a valid dictionary even if no config is provided.
120-123: Good addition of the dynamic_streams property.This property exposes the dynamic stream configurations by calling the existing method with both manifest and config parameters. The return type annotation properly indicates a list of dictionaries.
362-403: Nice implementation of dynamic stream name tracking.You've enhanced the loop to include the index for each dynamic definition, using it to generate a unique fallback name when the "name" is not explicitly provided. This ensures each dynamic stream has a distinct identifier.
The format
dynamic_stream_{dynamic_definition_index}provides good clarity about the stream's source and position.unit_tests/connector_builder/test_connector_builder_handler.py (1)
63-63: No issues with the new import
Everything looks good here.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/connector_builder/connector_builder_handler.py (1)
120-140: Function implementation looks good, but could we add a docstring?The implementation of
full_resolve_manifestlooks solid - it creates a copy of the source's resolved manifest, setsdynamic_stream_nameto None for existing streams, and adds dynamic streams. Consider adding a docstring explaining the purpose and behavior of this function - especially how it differs from the existingresolve_manifestfunction. This would help future maintainers understand its role in supporting dynamic stream names. What do you think?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/connector_builder/connector_builder_handler.py(1 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/connector_builder/test_connector_builder_handler.py (4)
airbyte_cdk/connector_builder/models.py (2)
HttpRequest(17-21)HttpResponse(10-13)airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
ManifestDeclarativeSource(60-430)resolved_manifest(113-114)airbyte_cdk/connector_builder/connector_builder_handler.py (1)
TestReadLimits(37-40)airbyte_cdk/connector_builder/main.py (1)
handle_connector_builder_request(70-88)
🪛 GitHub Actions: Pytest (Fast)
unit_tests/connector_builder/test_connector_builder_handler.py
[error] 1824-1824: AssertionError: Expected resolved manifest does not match actual resolved manifest. Differences in stream URLs indicate potential misconfiguration.
[warning] 36-36: PytestCollectionWarning: cannot collect test class 'TestReadLimits' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestReader' because it has a init constructor.
[warning] 64-64: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 81-81: PytestCollectionWarning: cannot collect test class 'TestSpec' because it has a init constructor.
[warning] 135-135: PytestCollectionWarning: cannot collect test class 'TestFileContent' because it has a init constructor.
[warning] 261-261: PytestCollectionWarning: cannot collect test class 'TestServer' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestingCustomSubstreamPartitionRouter' because it has a init constructor.
[warning] 18-18: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
[warning] 44-44: PytestCollectionWarning: cannot collect test class 'TestScenario' because it has a init constructor.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
unit_tests/connector_builder/test_connector_builder_handler.py (3)
158-329: LGTM! Complex but well-structured test manifestThe dynamic stream manifest is well-structured and provides a good test case for the dynamic stream functionality.
🧰 Tools
🪛 GitHub Actions: Pytest (Fast)
[warning] 261-261: PytestCollectionWarning: cannot collect test class 'TestServer' because it has a init constructor.
384-387: LGTM! Clear configuration setupThe configuration correctly points to the dynamic stream manifest and the new
full_resolve_manifestcommand.
1366-1825:❓ Verification inconclusive
Test is failing in the CI pipeline - possible manifest mismatch
The test structure and HTTP mocking look good, but there's a failing assertion in the CI pipeline: "AssertionError: Expected resolved manifest does not match actual resolved manifest."
The test might be failing because of differences between the expected and actual manifests. This could be related to the presence of a "use_cache" attribute in line 1757 of the expected manifest, which might differ from the actual implementation.
Once you identify the differences, you can update either your implementation or the expected manifest to make them match. Would you like help adjusting the test to match the actual implementation?
🏁 Script executed:
#!/bin/bash # Identify differences between expected and actual manifest cd unit_tests/connector_builder python -c " import json import difflib import pprint from test_connector_builder_handler import DYNAMIC_STREAM_MANIFEST from airbyte_cdk.sources.declarative.manifest_declarative_source import ManifestDeclarativeSource from airbyte_cdk.connector_builder.connector_builder_handler import TestReadLimits, full_resolve_manifest from unit_tests.connector_builder.utils import create_configured_catalog from airbyte_cdk.models import AirbyteStateMessage, AirbyteStreamState, StreamDescriptor, AirbyteStateBlob # Setup state = [AirbyteStateMessage(type='STREAM', stream=AirbyteStreamState(stream_descriptor=StreamDescriptor(name='stream_with_custom_requester'), stream_state=AirbyteStateBlob({'key': 'value'})))] source = ManifestDeclarativeSource(source_config=DYNAMIC_STREAM_MANIFEST) # Get actual manifest actual_manifest = full_resolve_manifest(source).record.data['manifest'] # Get expected manifest (from test_full_resolve_manifest) with open('test_connector_builder_handler.py', 'r') as file: test_file = file.read() # Extract expected manifest from the test file by finding a large dict assignment # This is a simplistic approach - you might need to adjust this import re expected_manifest_str = re.search(r'expected_resolved_manifest = \{.*?\n\}', test_file, re.DOTALL) if expected_manifest_str: print('Found expected manifest in test file') # Print key differences to help debug print('\\nKey differences that could be causing the test to fail:') # Compare the keys at root level actual_keys = set(actual_manifest.keys()) expected_keys = {'version', 'definitions', 'streams', 'check', 'spec', 'dynamic_streams', 'type'} print('Missing keys in actual:', expected_keys - actual_keys) print('Extra keys in actual:', actual_keys - expected_keys) # Compare stream names actual_stream_names = sorted([s.get('name', 'unknown') for s in actual_manifest.get('streams', [])]) expected_stream_names = sorted(['stream_with_custom_requester', 'parent_1_item_1', 'parent_1_item_2', 'parent_2_item_1', 'parent_2_item_2']) print('\\nStream names match:', actual_stream_names == expected_stream_names) if actual_stream_names != expected_stream_names: print('Actual stream names:', actual_stream_names) print('Expected stream names:', expected_stream_names) # Check for use_cache attribute use_cache_in_actual = 'use_cache' in str(actual_manifest) use_cache_in_expected = True # Based on the test file print('\\nuse_cache in actual:', use_cache_in_actual) print('use_cache in expected:', use_cache_in_expected) # Check number of streams print('\\nNumber of streams in actual:', len(actual_manifest.get('streams', []))) print('Number of streams expected:', 5) # Based on the test file "Length of output: 3200
CI Issue: Manifest Mismatch & Missing Dependency "orjson"
It looks like the CI test run for
test_full_resolve_manifestisn’t completing as expected due to two potential issues:
- The test is failing because there may be a mismatch between the expected and actual manifests (for example, the
use_cacheattribute seems to be a point of contention).- The execution aborted with a
ModuleNotFoundError: No module named 'orjson', indicating that the dependency required by the test environment is missing.Could you please verify whether:
- The dependency on orjson should be added to the CI environment or test dependencies?
- The expected manifest (including attributes like
use_cache) needs to be updated to reflect the actual implementation?Would you like some help in adjusting the test or updating the dependency configuration? wdyt?
🧰 Tools
🪛 GitHub Actions: Pytest (Fast)
[error] 1824-1824: AssertionError: Expected resolved manifest does not match actual resolved manifest. Differences in stream URLs indicate potential misconfiguration.
lazebnyi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
unit_tests/connector_builder/test_connector_builder_handler.py (3)
1371-1388: Consider adding more descriptive comments about the mock data.The HTTP mocking is functional, but it would be helpful to have comments explaining the expected relationship between the parents and items, and how that relates to the expected output streams.
Would you consider adding a comment explaining the relationship between these parent/item responses and how they translate to the expected dynamic streams? Something like:
with HttpMocker() as http_mocker: + # Mock parent data - each parent will generate streams for its items http_mocker.get( HttpRequest(url="https://api.test.com/parents"), HttpResponse(body=json.dumps([{"id": 1}, {"id": 2}])), ) parent_ids = [1, 2] for parent_id in parent_ids: + # Mock item data - each item will become a stream with name pattern 'parent_{parent_id}_item_{item_id}' http_mocker.get( HttpRequest(url=f"https://api.test.com/parent/{parent_id}/items"), HttpResponse( body=json.dumps( [ {"id": 1, "name": "item_1"}, {"id": 2, "name": "item_2"}, ] ) ), )
1392-1825: Expected manifest structure is comprehensive but could be more maintainable.The expected manifest is properly structured and provides thorough validation, but its size makes it potentially brittle to maintain as the structure evolves.
Would it make sense to focus the validation on the most critical aspects instead of the full structure? Perhaps:
- assert resolved_manifest.record.data["manifest"] == expected_resolved_manifest + # Validate key aspects of the resolved manifest + actual_manifest = resolved_manifest.record.data["manifest"] + # Check manifest type and version + assert actual_manifest["type"] == "DeclarativeSource" + assert actual_manifest["version"] == "0.30.3" + # Check stream generation + actual_streams = actual_manifest["streams"] + expected_stream_names = ["stream_with_custom_requester", "parent_1_item_1", "parent_1_item_2", "parent_2_item_1", "parent_2_item_2"] + assert len(actual_streams) == len(expected_stream_names) + assert set(s.get("name") for s in actual_streams) == set(expected_stream_names) + # Check that the dynamic streams have proper dynamic_stream_name + dynamic_streams = [s for s in actual_streams if s.get("dynamic_stream_name") == "TestDynamicStream"] + assert len(dynamic_streams) == 4 # We expect 4 dynamically generated streamsThis would make the test more resilient to non-essential changes in the manifest structure. What do you think?
82-82: Noteuse_cacheparameter in the components resolver.The
use_cacheparameter is set toTruein the components resolver's requester. This is visible in the resolved manifest (line 1757), but its implications aren't documented.The purpose of the
use_cacheparameter might not be immediately obvious to readers. Would it help to add a small comment explaining its role in caching responses during the test? wdyt?unit_tests/connector_builder/test.txt (1)
1-374: Diff output shows expected changes to manifest structure.This file appears to be a diff output showing the complete resolved manifest structure, including the URL base changes and dynamic stream additions. It's useful as reference documentation but not necessarily needed as a test file.
Since this is a large diff output file, would it make more sense to either:
- Include this as a comment in the test file, or
- Document it in a more structured format like a README or Markdown file?
Having standalone diff files can sometimes be confusing for maintainers who come across them later. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
unit_tests/connector_builder/test.txt(1 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/connector_builder/test_connector_builder_handler.py (4)
airbyte_cdk/connector_builder/models.py (2)
HttpRequest(17-21)HttpResponse(10-13)airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
ManifestDeclarativeSource(60-430)resolved_manifest(113-114)airbyte_cdk/connector_builder/connector_builder_handler.py (1)
TestReadLimits(37-40)airbyte_cdk/connector_builder/main.py (1)
handle_connector_builder_request(70-88)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Analyze (python)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (8)
unit_tests/connector_builder/test_connector_builder_handler.py (5)
63-63: Good addition of mock HTTP utilities.The import of
HttpMocker,HttpRequest, andHttpResponseis appropriate for the new test function that will use them for HTTP request/response mocking.
158-329: New dynamic stream manifest looks good.The structure follows the existing pattern from the regular
MANIFEST, while adding the essentialdynamic_streamsarray with properly configured DynamicDeclarativeStream. This will provide a good test case for the new full_resolve functionality.
384-387: Configuration for the new manifest is properly configured.The configuration object properly points to the dynamic stream manifest and specifies the new
full_resolve_manifestcommand.
1366-1391: Test is well structured with proper HTTP mocking.The test function properly sets up the environment and uses HttpMocker to simulate responses for both parent objects and their items. The approach of testing two parent IDs with two items each provides good coverage.
217-279:❓ Verification inconclusive
Check if using an internal IP in tests is appropriate.
The URL base has been changed from
https://api.sendgrid.comtohttps://10.0.27.27/api/v1/throughout the resolved manifest. While this is consistent, using what appears to be an internal IP address in tests could raise questions.Is this internal IP address intended for testing purposes only? If it's meant to represent an internal service, consider using a standard test domain like
example.comor a clear placeholder likehttps://test-api.internal/api/v1/to make the intent clearer.
🏁 Script executed:
#!/bin/bash # Check how this IP is used across the codebase echo "Checking for uses of 10.0.27.27 in the codebase" rg "10\.0\.27\.27" --type pyLength of output: 3511
Action Required: Confirm Test URL Intent
I noticed that inunit_tests/connector_builder/test_connector_builder_handler.pythe internal IP10.0.27.27is used consistently (e.g.,"https://10.0.27.27/api/v1/"). Could you please confirm if this is intentionally meant for testing, or would you consider replacing it with a more conventional test domain (such ashttps://test-api.internal/api/v1/orhttps://example.com/api/v1/) for clarity? wdyt?unit_tests/connector_builder/test.txt (3)
117-120: URL base change is consistently applied.The URL base change from
https://api.sendgrid.comtohttps://10.0.27.27/api/v1/is correctly applied throughout the configuration. This change aligns with what's in the test file.
32-108: Dynamic streams configuration matches test expectations.The dynamic streams configuration in this diff aligns well with the implementation in the test file, confirming the expected structure is correctly represented.
280-371: Generated dynamic streams look correctly structured.The four dynamic streams generated from the two parents with two items each show the correct naming pattern (
parent_{parent_id}_item_{item_id}) and have all the expected properties and configurations.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
120-123: New property looks good. Consider adding a docstring for clarity?The new
dynamic_streamsproperty provides a convenient way to access dynamic stream configurations with their names. It would be helpful to add a docstring explaining the purpose and return type of this property.@property def dynamic_streams(self) -> List[Dict[str, Any]]: + """ + Returns a list of dynamic stream configurations with unique identifiers. + Each dynamic stream includes a 'dynamic_stream_name' field for tracking and identification. + """ return self._dynamic_stream_configs(manifest=self._source_config, config=self._config, with_dynamic_stream_name=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(4 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)unit_tests/sources/declarative/resolvers/test_http_components_resolver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- unit_tests/sources/declarative/resolvers/test_http_components_resolver.py
- airbyte_cdk/sources/declarative/models/declarative_component_schema.py
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/manifest_declarative_source.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Please run 'ruff format' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- 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/manifest_declarative_source.py (5)
109-109: Good addition of default configuration initialization, wdyt about consistency with other class members?Initializing
self._configwith an empty dictionary when no configuration is provided is a good defensive programming practice. This prevents potentialNonereferences later in the code.
356-356: Parameter addition maintains backward compatibility, nice work!The addition of the optional
with_dynamic_stream_nameparameter with a default value ofNoneensures backward compatibility with existing code that calls this method.
362-362: Good enhancement to the loop structure for index trackingAdding the index to the loop iteration is a necessary improvement to support generating default names for dynamic streams.
401-405: Enhances uniqueness of dynamic stream names, nice implementation!This conditional block adds a unique identifier to each dynamic stream, using either the provided name from the dynamic definition or a default value based on the index. This enhances traceability and identification of dynamic streams.
1-1:✅ Verification successful
Fix formatting issues detected by the pipeline
The pipeline has reported a Ruff formatting issue that needs to be addressed.
🏁 Script executed:
#!/bin/bash # Run Ruff formatting to fix the issues ruff format airbyte_cdk/sources/declarative/manifest_declarative_source.py # Check what changes would be made git diff airbyte_cdk/sources/declarative/manifest_declarative_source.pyLength of output: 1567
Formatting issues have been fixed successfully – changes verified!
I've confirmed that the Ruff formatting fix reformatted the file as expected. The diff output shows the following improvements:
- The
dynamic_streamsproperty now spreads the parameters over multiple lines for improved readability.- The
_dynamic_stream_configsmethod signature has been reformatted so that each parameter appears on its own line.These changes address the formatting issues flagged by the pipeline. Does this update look good to you, wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Please run 'ruff format' to fix code style issues in this file.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/connector_builder/connector_builder_handler.py (1)
120-143: New function looks good, but could use docstring and minor improvementsThe implementation of
full_resolve_manifestsuccessfully handles both standard and dynamic streams. Nice work on separating the exception handling forAirbyteTracedExceptionandExceptionas suggested in the previous review!A few suggestions to enhance this function:
Could we add a docstring explaining what this function does, its parameters, and return values? This would help future developers understand the purpose of this function, wdyt?
The error message "Error full resolving manifest" at line 140 sounds a bit off grammatically. Perhaps "Error fully resolving manifest" would read better?
The dictionary copy at line 122 (
{**source.resolved_manifest}) creates a shallow copy. If there are nested dictionaries that need to be modified independently, consider usingcopy.deepcopyinstead, wdyt?+import copy + def full_resolve_manifest(source: ManifestDeclarativeSource) -> AirbyteMessage: + """ + Fully resolve a manifest by processing both standard and dynamic streams. + + Args: + source: The ManifestDeclarativeSource containing the manifest to resolve + + Returns: + AirbyteMessage: A record message containing the fully resolved manifest or an error message + """ try: - manifest = {**source.resolved_manifest} + manifest = copy.deepcopy(source.resolved_manifest) streams = manifest.get("streams", []) for stream in streams: stream["dynamic_stream_name"] = None streams.extend(source.dynamic_streams) manifest["streams"] = streams return AirbyteMessage( type=Type.RECORD, record=AirbyteRecordMessage( data={"manifest": manifest}, emitted_at=_emitted_at(), stream="full_resolve_manifest", ), ) except AirbyteTracedException as exc: return exc.as_airbyte_message() except Exception as exc: error = AirbyteTracedException.from_exception( - exc, message=f"Error full resolving manifest: {str(exc)}" + exc, message=f"Error fully resolving manifest: {str(exc)}" ) return error.as_airbyte_message()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/connector_builder/connector_builder_handler.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
lazebnyi
deleted the
lazebnyi/add-full-resolve-command-to-builder-handler
branch
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/12172
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/12171
Summary by CodeRabbit
New Features
Tests