feat: enable handling of nested fields when injecting request_option in request body_json #201
Conversation
ChristoGrab
changed the title
to handle field_path in JSON body
ChristoGrab
changed the title
…other injection types
📝 WalkthroughWalkthroughThe pull request introduces comprehensive modifications to the Airbyte CDK's declarative source components, focusing on enhancing request option handling, authentication, and schema flexibility. The changes primarily involve updating type annotations, refactoring request option injection methods, and introducing new capabilities like nested field path support for request configurations. Changes
Possibly related PRs
Suggested Reviewers
Hey there! 👋 I noticed some interesting changes in how request options are being handled. The introduction of Would you be interested in expanding the README or creating a migration guide to help users understand the new nested field injection feature? It could really help smooth the transition for existing declarative source implementations. 🚀 ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
airbyte_cdk/sources/declarative/requesters/request_option.py (1)
43-49: Could we simplify the mutual exclusivity checks?Hi! I noticed that we're checking for mutual exclusivity between
field_nameandfield_pathwith separate conditions. Perhaps we could simplify this by ensuring exactly one is provided using a single condition:if (self.field_name is None) == (self.field_path is None): raise ValueError("RequestOption requires exactly one of field_name or field_path")This way, we handle both cases in one check. Wdyt?
unit_tests/utils/test_mapping_helpers.py (2)
19-44: Parameterizing string handling tests enhances clarity!Consolidating the string handling tests with parameterization makes the intent of each test case clearer and reduces code duplication. This approach is very effective. Wdyt?
97-106: Consider providing more detailed error messages in tests?In the
test_body_json_requests, the expected error message is "Duplicate keys found". Do you think adding more context to the error message could help in identifying issues faster during debugging? For example, specifying which keys are duplicated. Wdyt?airbyte_cdk/utils/mapping_helpers.py (2)
10-45: LGTM! Consider adding doctest examples?The implementation is robust and handles nested structures well. Would you consider adding some doctest examples to demonstrate the different behaviors between
allow_same_value_merge=TrueandFalse? This could help future maintainers understand the nuances quickly, wdyt?Example:
def _merge_mappings( target: Dict[str, Any], source: Mapping[str, Any], path: Optional[List[str]] = None, allow_same_value_merge: bool = False, ) -> None: """ Recursively merge two dictionaries, raising an error if there are any conflicts. >>> target = {"a": 1} >>> source = {"a": 1} >>> _merge_mappings(target, source, allow_same_value_merge=True) # No error >>> target {'a': 1} >>> target = {"a": 1} >>> source = {"a": 1} >>> _merge_mappings(target, source, allow_same_value_merge=False) # Raises ValueError Traceback (most recent call last): ... ValueError: Duplicate keys found: a """
47-104: LGTM! Consider enhancing type hints?The implementation is clean and well-documented. Would you consider making the type hints more specific for the return type? Instead of
Union[Mapping[str, Any], str], we could use a TypeVar to preserve the exact input type, wdyt?from typing import TypeVar T = TypeVar('T', bound=Union[Mapping[str, Any], str]) def combine_mappings( mappings: List[Optional[T]], allow_same_value_merge: bool = False, ) -> T:unit_tests/sources/declarative/requesters/request_options/test_request_options.py (1)
51-93: LGTM! Consider adding edge cases?Great test coverage! Would you consider adding a few edge cases to
test_inject_into_request_cases, wdyt?
- Empty path segments:
field_path=["data", "", "field"]- Special characters in paths:
field_path=["data", "field.with.dots"]- Integer keys:
field_path=["data", "0", "field"]unit_tests/sources/declarative/auth/test_token_auth.py (1)
253-285: LGTM! Consider adding error cases?Great test cases for the happy path! Would you consider adding some error scenarios to ensure proper validation, wdyt?
- Invalid field path (e.g.,
Nonein path)- Circular interpolation in path
- Non-string interpolation results
airbyte_cdk/sources/declarative/auth/token.py (1)
58-60: Consider adding a docstring for clarity, wdyt?The
_get_request_optionsmethod has been updated to useMutableMappingand the newinject_into_requestmethod. While the changes look good, adding a docstring would help explain the purpose of these changes, especially regarding nested field support.def _get_request_options(self, option_type: RequestOptionType) -> Mapping[str, Any]: + """ + Get request options for the specified option type. + + Args: + option_type: The type of request option (header, parameter, body_data, or body_json) + + Returns: + A mapping containing the request options, supporting nested fields for body_json + """ options: MutableMapping[str, Any] = {}airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2819-2829: Consider adding mutual exclusivity validation?The new
field_pathproperty looks great! Sincefield_nameandfield_pathserve similar purposes but in different contexts, would it make sense to add validation to ensure they're not used simultaneously? This could help prevent confusion and potential conflicts, wdyt?Example validation in the schema:
oneOf: - required: [field_name] not: required: [field_path] - required: [field_path] not: required: [field_name]airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (1)
131-133: Consider adding a comment explaining the special handling of body_json.The introduction of
is_body_jsonflag affects mapping behavior. Would you consider adding a brief comment explaining why body_json requests need different handling for merging mappings with same values? This would help future maintainers understand the intent better.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
airbyte_cdk/sources/declarative/auth/token.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/incremental/datetime_based_cursor.py(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/partition_routers/list_partition_router.py(2 hunks)airbyte_cdk/sources/declarative/partition_routers/substream_partition_router.py(3 hunks)airbyte_cdk/sources/declarative/requesters/http_requester.py(2 hunks)airbyte_cdk/sources/declarative/requesters/paginators/default_paginator.py(2 hunks)airbyte_cdk/sources/declarative/requesters/request_option.py(2 hunks)airbyte_cdk/sources/declarative/requesters/request_options/datetime_based_request_options_provider.py(1 hunks)airbyte_cdk/sources/declarative/retrievers/simple_retriever.py(2 hunks)airbyte_cdk/utils/mapping_helpers.py(1 hunks)unit_tests/sources/declarative/auth/test_token_auth.py(1 hunks)unit_tests/sources/declarative/incremental/test_datetime_based_cursor.py(5 hunks)unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py(1 hunks)unit_tests/sources/declarative/requesters/request_options/test_request_options.py(1 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(1 hunks)unit_tests/sources/declarative/retrievers/test_simple_retriever.py(6 hunks)unit_tests/utils/test_mapping_helpers.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py
- unit_tests/sources/declarative/retrievers/test_simple_retriever.py
⏰ 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 (20)
airbyte_cdk/sources/declarative/requesters/request_option.py (2)
58-65: Should we unify field handling by always usingfield_path?I see that we're handling
field_nameandfield_pathseparately. What do you think about convertingfield_nameinto a single-elementfield_pathlist during initialization? This might simplify the code by allowing us to work exclusively withfield_path. For example:if self.field_name is not None: self.field_path = [InterpolatedString.create(self.field_name, parameters=parameters)]This could reduce conditional logic later on. Wdyt?
85-116: Handlinginject_into_requestlogic elegantly!The use of
inject_into_requestmethod to handle bothfield_nameandfield_pathcases is clean and enhances readability. It effectively encapsulates the injection logic. Nice work!airbyte_cdk/sources/declarative/requesters/request_options/datetime_based_request_options_provider.py (2)
85-87: Should we handle missing time values before injection?Hi there! In the
_get_request_optionsmethod, ifstart_time_valueorend_time_valueis missing from thestream_slice, we might end up injectingNoneinto the request options. Do you think it would be good to check if these values are notNonebefore injecting them? This could prevent potential issues with requests. Wdyt?Also applies to: 89-91
83-91: Great refactoring to useinject_into_requestmethod!Switching to
inject_into_requestfor injecting options enhances modularity and keeps the codebase consistent. This improves maintainability and readability. Well done!unit_tests/utils/test_mapping_helpers.py (1)
6-17: Nice use of parameterization for basic functionality tests!Refactoring the basic mapping tests into a parameterized function improves the test suite's structure and makes it easier to add new cases in the future. Great job!
airbyte_cdk/sources/declarative/partition_routers/list_partition_router.py (1)
103-105: LGTM! Clean integration with the new request option handling.The changes properly utilize the new
inject_into_requestmethod while maintaining type safety withMutableMapping.airbyte_cdk/sources/declarative/auth/token.py (1)
8-8: LGTM! Enhanced type imports.The addition of
MutableMappingto imports supports better type safety for request options handling.airbyte_cdk/sources/declarative/requesters/paginators/default_paginator.py (1)
190-190: LGTM! Improved request option handling.The changes look good:
- Using
MutableMappingfor better type safety- Using
inject_into_requestfor both page token and page size options- Clear separation of concerns between token and page size injection
Also applies to: 199-199, 200-200, 206-208
airbyte_cdk/sources/declarative/requesters/http_requester.py (1)
202-204: Consider adding a test case for body_json merging, wdyt?The addition of
is_body_jsonflag andallow_same_value_mergeparameter looks good for supporting nested fields in GraphQL queries. However, it would be beneficial to add test cases specifically for this scenario.Run this script to check existing test coverage:
Also applies to: 214-215
airbyte_cdk/sources/declarative/partition_routers/substream_partition_router.py (1)
121-121: LGTM! Enhanced type safety and request option handling.The changes improve the code by:
- Using
MutableMappingfor better type safety- Using
inject_into_requestfor cleaner parameter injectionAlso applies to: 131-131
unit_tests/sources/declarative/requesters/test_http_requester.py (1)
280-281: Test cases updated to match new body_json behavior.The test expectations now correctly reflect that same values can be merged in body_json context. This aligns with the changes in the implementation.
unit_tests/sources/declarative/incremental/test_datetime_based_cursor.py (2)
785-787: Great addition of nested field injection test case!The new test case thoroughly validates the nested field injection capability for GraphQL queries. The test parameters are well structured and cover the key scenarios.
Also applies to: 827-845
872-874: Clean update to test helper code.The test helper code has been updated to support both field_name and field_path parameters, maintaining backward compatibility while adding support for the new functionality.
Also applies to: 879-884
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
722-722: LGTM! Nice improvement in clarity.Setting an explicit default value of
Nonefor the optionalconditionfield makes the intent clearer.
1173-1178: Deprecation notice looks good.The field description clearly indicates the plan to deprecate
field_namein favor offield_path. This aligns with the previous discussion in PR comments.
1179-1183: LGTM! Well-documented new field for nested paths.The new
field_pathfield is well-documented with clear examples, and it aligns perfectly with the PR objective of supporting nested fields for GraphQL queries.airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
2801-2801: LGTM! Clear description update.The updated description accurately reflects the addition of path-based field specification while maintaining backward compatibility.
2811-2812: LGTM! Well-documented deprecation notice.The deprecation notice clearly explains that
field_namewill eventually be replaced byfield_pathwhile maintaining backward compatibility for the Builder. This helps users prepare for future changes.airbyte_cdk/sources/declarative/incremental/datetime_based_cursor.py (1)
368-376: LGTM! Nice refactor of request option handling.The changes improve maintainability by separating value extraction from injection logic. The use of
inject_into_requestmethod provides a more robust way to handle request options.airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (1)
149-149: Verify combine_mappings behavior with allow_same_value_merge.Let's ensure the combine_mappings utility correctly handles the new parameter.
✅ Verification successful
combine_mappings implementation verified ✅
The utility correctly handles the
allow_same_value_mergeparameter with comprehensive test coverage for both body_json and non-body-json scenarios. The implementation in simple_retriever.py is using it correctly!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check combine_mappings implementation for allow_same_value_merge parameter # Search for combine_mappings implementation and its tests rg -A 10 "def combine_mappings"Length of output: 907
Script:
#!/bin/bash # Get the full implementation and tests rg -g "mapping_helpers*" -A 30 "def combine_mappings" # Look for test files fd "test.*mapping.*" --exec cat {}Length of output: 9606
…m/airbytehq/airbyte-python-cdk into christo/request-option-field-path
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2077-2079: Consider passing the parameters from the model for better consistency
Right now, you are passing an empty dictionary asparameters. Perhaps we could passmodel.parametersif needed for a consistent approach with the rest of the code. Wdyt?- return RequestOption( - field_name=field_name, field_path=field_path, inject_into=inject_into, parameters={} + return RequestOption( + field_name=field_name, + field_path=field_path, + inject_into=inject_into, + parameters=model.parameters or {}, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2075-2076: Add clarity on the type ignore directive?
It looks like you used# type: ignoreto handle a potential mismatch in the declared type or themodel.field_path. Maybe it's beneficial to refine the type annotation or handle the mismatch if there's a known reason behind it, wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/test_manifest_declarative_source.py (1)
1027-1028: LGTM! Consider adding more test cases for nested fields?The changes correctly test the new capability to inject request options into nested fields within the request body. This is particularly useful for GraphQL queries where variables need to be nested.
Would you consider adding more test cases to cover edge cases like:
- Multiple levels of nesting
- Invalid field paths
- Empty field paths
- Special characters in field paths
wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(8 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(6 hunks)unit_tests/sources/declarative/test_manifest_declarative_source.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (2)
596-597: LGTM! The test updates look good for ListPartitionRouter.The test now correctly validates the new field_path functionality for body_json injection. The assertions properly verify that the field_path contains InterpolatedString instances and has the expected length.
Also applies to: 613-616
719-720: LGTM! The test updates look good for DatetimeBasedCursor.The test now correctly validates the field_path functionality for body_json injection in the datetime-based cursor. The assertions properly verify the field path evaluation.
Also applies to: 748-750
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/test_manifest_declarative_source.py (1)
1033-1034: LGTM! The test case aligns with the PR objectives.The change from
field_nametofield_pathcorrectly tests the new nested field support for GraphQL queries. The test case uses a realistic example with["variables", "page_size"]as the field path.Hey, would you like to add a test case for deeply nested fields (e.g.,
["variables", "pagination", "page_size"]) to ensure the feature works with arbitrary nesting levels? wdyt? 🤔airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1203-1208: Consider adding a deprecation warning forfield_name.The description indicates plans to deprecate this field in favor of
field_path. Since this is a breaking change from required to optional, should we add a deprecation warning to help users transition smoothly? wdyt?field_name: Optional[str] = Field( None, description="[DEPRECATED] Configures which key should be used in the location that the descriptor is being injected into. We hope to eventually deprecate this field in favor of `field_path` for all request_options, but must currently maintain it for backwards compatibility in the Builder.", examples=["segment_id"], title="Field Name", deprecated=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(8 hunks)airbyte_cdk/sources/declarative/partition_routers/substream_partition_router.py(3 hunks)airbyte_cdk/sources/declarative/retrievers/simple_retriever.py(2 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(6 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(1 hunks)unit_tests/sources/declarative/retrievers/test_simple_retriever.py(6 hunks)unit_tests/sources/declarative/test_manifest_declarative_source.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- airbyte_cdk/sources/declarative/retrievers/simple_retriever.py
- unit_tests/sources/declarative/requesters/test_http_requester.py
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
- unit_tests/sources/declarative/parsers/test_model_to_component_factory.py
- airbyte_cdk/sources/declarative/partition_routers/substream_partition_router.py
- unit_tests/sources/declarative/retrievers/test_simple_retriever.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Ruff Lint Check
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: MyPy Check
- GitHub Check: preview_docs
- GitHub Check: Build and Inspect Python Package
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1209-1214: LGTM! The newfield_pathfield looks good.The field is well-documented with clear examples for nested structures in GraphQL queries. The implementation aligns with the PR objectives.
What
To support GraphQL queries natively in the CDK, we need to add handling of nested structures when injecting a
RequestOptioninto a request body (specifically withbody_jsoninjection).How
RequestOptionclass now includes an optionalfield_pathparam, and each instance must declare afield_pathORfield_name.inject_into_requestmethod determines how to handle the injection based on the existense offield_pathand the specificRequestOptionType. As stated above, onlybody_jsoninjections will support nested fields.combine_mappingsutility function now handles nested structures when checking for mapping conflicts.Suggested review order
request_option.py: Has the main updated logic for validation and the new methodinject_into_requestmapping_helpers.py: added a helper method and extended the main function for catching conflicts in nested structures.declarative_component_schema.yamlanddeclarative_component_schema.py: Updated schema + modeltoken.py,datetime_based_cursor.py,datetime_based_request_options_provider.py,default_paginator.py,list_partition_router.pyandsubstream_partition_router.pyall have minor updates to leverage the new method.Summary by CodeRabbit
Release Notes
New Features
field_pathfor specifying nested structures in JSON requests.Improvements
Bug Fixes
Testing