fix: (CDK) (Manifest Migration) - fix the request_body_json / request_body_data > request_body migration
#521
Conversation
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis set of changes refactors and expands the handling of HTTP request body types in the declarative component schema, migration logic, and related code. The migration for unifying Changes
Sequence Diagram(s)sequenceDiagram
participant Manifest as Manifest
participant Migration as HttpRequesterRequestBodyJsonDataToRequestBody
participant Helper as Helper Methods
Manifest->>Migration: Call migrate(manifest)
Migration->>Manifest: Check for request_body_json or request_body_data
alt request_body_json exists
Migration->>Helper: _migrate_body_json(manifest, key)
Helper->>Manifest: Assign request_body with type based on value (PlainText, GraphQL, JsonObject)
Helper->>Manifest: Remove request_body_json key
else request_body_data exists
Migration->>Helper: _migrate_body_data(manifest, key)
Helper->>Manifest: Assign request_body with type UrlEncodedForm
Helper->>Manifest: Remove request_body_data key
end
Migration->>Manifest: Validate migration
Possibly related PRs
Suggested labels
Suggested reviewers
Would it make sense to add a few more test cases for edge-case GraphQL payloads, just to be extra sure the new migration logic covers all scenarios—wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py (1)
44-61: New helper method for JSON body migrationThis new method determines the appropriate type for JSON bodies based on their structure:
- Strings are treated as
RequestBodyPlainText- Dictionaries with a
querykey are treated asRequestBodyGraphQL- Other dictionaries are treated as
RequestBodyJsonObjectOne consideration: when checking for GraphQL queries, it only looks for the existence of a
querykey. Would it be more robust to also check the structure or type of thequeryvalue to avoid false positives? wdyt?airbyte_cdk/sources/declarative/declarative_component_schema.yaml (4)
4079-4092: Add examples for plain-text payload and unify naming
TheRequestBodyPlainTextschema is clear, but it lacks anexamplessection. Could we add a snippet like:examples: - type: RequestBodyPlainText value: "some plain text"Also, since the old examples use
RequestBodyData, should we rename either the definition or the examples to keep them aligned? wdyt?
4093-4106: Clarify URL-encoded form payload shape with examples
ForRequestBodyUrlEncodedForm, it might help to include an example object, e.g.:examples: - type: RequestBodyUrlEncodedForm value: field1: "a" field2: "b"Would that make the intended structure clearer? wdyt?
4108-4120: Consider simplifyingRequestBodyJsonObjectname or adding examples
RequestBodyJsonObjectis explicit but slightly verbose compared to the previousRequestBodyJsonnaming. Would you consider renaming it toRequestBodyJson? If we keep the current name, could we at least add an example like:examples: - type: RequestBodyJsonObject value: key: "value" nested: subkey: 123wdyt?
4122-4134: Unify GraphQL naming and add usage example
The GraphQL schema usesRequestBodyGraphQLand referencesRequestBodyGraphQlQuery(mixed casing). Would renaming the query definition toRequestBodyGraphQLQueryimprove clarity? Also, could we include an example underRequestBodyGraphQLsuch as:examples: - type: RequestBodyGraphQL value: query: query: "{ users { id name } }"wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py(1 hunks)airbyte_cdk/manifest_migrations/migrations/registry.yaml(1 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py(3 hunks)unit_tests/manifest_migrations/conftest.py(20 hunks)unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
RequestBodyGraphQL(1926-1928)RequestBodyJsonObject(1512-1514)RequestBodyPlainText(1502-1504)RequestBodyUrlEncodedForm(1507-1509)airbyte_cdk/sources/streams/http/http.py (2)
request_body_data(212-227)request_body_json(229-240)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py (3)
airbyte_cdk/manifest_migrations/manifest_migration.py (1)
validate(68-73)airbyte_cdk/manifest_migrations/migrations/http_requester_path_to_url.py (1)
validate(49-57)airbyte_cdk/manifest_migrations/migrations/http_requester_url_base_to_url.py (1)
validate(33-41)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (41)
airbyte_cdk/manifest_migrations/migrations/registry.yaml (1)
6-6: Version bump for the new migration looks good!This change increments the version from 6.48.2 to 6.48.3, corresponding to the enhanced HTTP request body migration logic updates. The version bump is necessary to track the migration changes.
unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py (20)
7-13: Updated imports to support the new request body typesThe imports now include all the necessary request body type models to support the refactored approach to handling HTTP request bodies. These align with the model changes in the declarative component schema.
146-149: Test now uses the typed RequestBodyJsonObject instead of raw dictThis test case has been updated to use the new typed request body class. The test still validates the same functionality but now uses the proper model structure.
153-157: Test updated with typed model correctly preserving test contextThe test has been refactored to use the
RequestBodyJsonObjectwhile maintaining the original test's meaning and assertions.
161-165: Test now properly uses typed request bodyGood update to use the structured
RequestBodyJsonObjectwith appropriate type annotation.
169-171: Test updated correctly to use RequestBodyJsonObjectThis refactoring preserves the test's functionality while adapting it to the new type system.
176-179: Updated to use the structured RequestBodyJsonObjectThe test continues to validate the interpolation of keys but now uses the proper request body model structure.
182-213: Tests properly updated to use typed request body modelsAll these test cases have been correctly migrated to use the explicit
RequestBodyJsonObjectmodel instead of raw dictionaries, preserving the same test assertions and validation logic.
216-219: Test now uses RequestBodyPlainText for string contentGood choice to use
RequestBodyPlainTextfor the test case that involves a JSON string literal. This matches the expected behavior where string literals are interpreted as plain text.
224-226: Nested object case properly updatedThis test case for nested objects has been correctly migrated to use the typed model.
231-234: Test for interpolated keys in nested objects updated correctlyThe test has been refactored to use the typed request body model while preserving the test's assertions.
237-246: New test case for GraphQL queries - great addition!This new test case ensures that GraphQL queries are properly handled and transformed into the expected format with a
queryfield. It's a good addition to cover the new GraphQL support.
251-251: Added return type annotation for consistencyAdded type hint for the function return type, which improves code readability and type safety. This is a good practice!
254-254: Updated parameter to use the unified request_body fieldThe provider constructor now takes a
request_bodyparameter instead of separate JSON and data parameters, which aligns with the new unified approach.
307-310: Test updated to use RequestBodyUrlEncodedFormThe test for form data has been updated to use the appropriate
RequestBodyUrlEncodedFormmodel, preserving the test's validation logic.
315-318: Form test correctly updated to use typed modelThis test case for URL-encoded form data with stream slice dependencies has been properly updated.
323-326: Config-dependent form data test updated correctlyThe test case has been migrated to use the typed form data model appropriately.
329-336: Empty dict test case updated for form dataThe test for default empty dictionary behavior has been correctly migrated to use an empty value object with the proper type.
339-344: Form data interpolation test properly updatedThe test with interpolated keys has been refactored to use the typed model, preserving the original test's assertions.
351-351: Added return type annotation for consistencyType hint added to match the similar change made to the JSON test function. Consistent typing improves code quality.
354-354: Updated parameter to use the unified request_body fieldThe provider constructor now uses the unified
request_bodyparameter, matching the pattern used in the JSON tests.airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (3)
9-14: Updated imports to include all request body type modelsThe imports now include all the specific request body types that will be handled by the provider, improving type safety and code clarity.
47-54: Enhanced type annotation with explicit request body typesThe type annotation for
request_bodynow explicitly lists all supported request body types, which makes the code more maintainable and enables better IDE autocompletion and type checking.
96-107: Improved request body resolution with explicit type handlingThe
_resolve_request_bodymethod now handles each request body type explicitly and raises errors for unsupported types. The new implementation:
- Handles URL-encoded forms (mapping to
request_body_data)- Handles GraphQL queries with a special structure
- Handles both JSON objects and plain text
- Includes proper error handling for unsupported types
This is a good improvement that aligns with the changes to the declarative component schema.
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py (4)
36-36: Delegated JSON body migration to a helper methodThe migration logic now delegates to a specialized helper method for JSON bodies. This improves code organization and makes the migration more maintainable.
38-38: Delegated form data migration to a helper methodSimilar to the JSON case, form data migration is now handled by a specialized helper method, making the code more maintainable.
62-67: New helper method for form data migrationThis method handles the migration of form data to the new typed structure, using
RequestBodyUrlEncodedFormas the type. Simple and clear implementation.
68-76: Shared helper for value migration with good reuseThis shared helper method sets the new key with the appropriate type and value, and removes the old key. This reduces code duplication and makes the migration logic more maintainable.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (6)
1502-1505: New class RequestBodyPlainText looks good!This new model for plain text request bodies with string values is a nice clean implementation. It effectively replaces part of the functionality of the deprecated generic RequestBody.
1507-1510: RequestBodyUrlEncodedForm implementation looks solidThis model for URL-encoded form data properly uses a dictionary of string key-value pairs, which matches how form data is typically structured.
1512-1515: RequestBodyJsonObject implementation is well-definedUsing Dict[str, Any] for the value field is appropriate here as JSON objects can contain values of various types.
1517-1521: GraphQL query model with flexible configurationThe extra configuration with
extra = Extra.allowmakes sense for GraphQL queries which can have varying structures. This allows for flexibility in the query format.
1926-1929: RequestBodyGraphQL wrapper implementation looks goodThis wrapper model correctly encapsulates the GraphQL query as its value, providing a clean type-safe approach to handling GraphQL requests.
2328-2336: Good implementation of the unified request_body fieldThe Union type correctly includes all the new request body types, providing type safety while maintaining flexibility. The deprecation of the old fields with clear migration messages is a good practice.
Should we consider adding validation to ensure that when users migrate from the deprecated
request_body_jsonandrequest_body_datafields, they correctly choose the appropriate new type? Maybe through documentation or examples? Just a thought, wdyt?unit_tests/manifest_migrations/conftest.py (7)
200-200: Version number updates to 6.48.3Good consistency in updating all version references to match the new version.
Also applies to: 497-497, 503-503, 515-515, 835-835, 1198-1198, 1204-1204, 1210-1210
641-645: Dictionary-based request_body_data test caseNice test case for migrating a dictionary-based
request_body_datawith interpolation. This will ensure the migration correctly handles complex URL-encoded form data including configuration values.
668-668: String-based request_body_data test caseGood test case for migrating a string-based
request_body_data. This covers an important use case where form data is provided as a pre-formatted string.
740-745: GraphQL request_body_json test caseExcellent addition of a GraphQL query test case. This ensures the migration correctly identifies and converts GraphQL queries to the new typed format.
I'm curious about how the migration determines whether a JSON object is a GraphQL query - is it just checking for the presence of a "query" field? This could potentially match non-GraphQL payloads that happen to have a "query" field. Might be worth considering a more robust detection mechanism if possible, wdyt?
850-856: Expected migration results with appropriate typesThe expected migration results correctly use the new typed request body models:
- RequestBodyUrlEncodedForm for form data (both dict and string formats)
- RequestBodyJsonObject for JSON objects
- RequestBodyPlainText for string JSON content
- RequestBodyGraphQL for GraphQL queries
This comprehensive coverage ensures the migration logic handles all relevant cases.
Also applies to: 883-885, 912-918, 945-947, 974-981
999-999: Empty requesters after migrationGood to see that the migration properly handles the
requester_Areference by excluding request body fields at the requester definition level and only including them at the stream level where they're actually used.
1195-1215: Well-structured metadata for applied migrationsThe metadata section properly records all applied migrations with consistent versioning. This history will be valuable for debugging and understanding the evolution of manifests.
What
After this change merged, there is a need to adjust the actual schema and the routing logic expected by Form-Based UI.
How
HttpRequester.request_bodyfield:RequestBodyUrlEncodedForm--> to encode asbody_dataRequestBodyGraphQL+RequestBodyGraphQlQuery(as a value) --> to encode asbody_jsonRequestBodyJsonObject--> to encode asbody_jsonRequestBodyPlainText--> to encode asbody_jsonairbyte_cdk.manifest_migrations.migrations.http_requester_request_body_json_data_to_request_body.pyto correctly resolve the previous structure to use the new one instead.migration testto cover the cases.Summary by CodeRabbit
New Features
Bug Fixes
Tests