fix: request_body GraphQL implementation #629
Conversation
lmossman
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the GraphQL implementation of the request_body field by treating query as a string and preserving sibling properties like variables.
- Changed
RequestBodyGraphQlQuery.queryfrom an object to astrand removed confusing examples. - Updated YAML and Pydantic schema defaults for
queryto a string template. - Refined migration logic to detect GraphQL only when
queryis a string and updated tests accordingly.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| unit_tests/.../test_interpolated_request_options_provider.py | Updated GraphQL tests to expect a string query and include variables. |
| unit_tests/manifest_migrations/conftest.py | Adjusted fixtures for request_body_json migration to use string query and variables. |
| airbyte_cdk/.../interpolated_request_options_provider.py | Switched GraphQL branch to use value.dict() so siblings like variables are included. |
| airbyte_cdk/.../declarative_component_schema.py | Changed query field type to str and removed outdated examples. |
| airbyte_cdk/.../declarative_component_schema.yaml | Updated query property to type: string, set a string default, and removed examples. |
| airbyte_cdk/manifest_migrations/.../http_requester_request_body_json_data_to_request_body.py | Refined detection to treat GraphQL only when query is a string. |
Comments suppressed due to low confidence (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:4307
- [nitpick] The default GraphQL query string contains indentation and empty lines; consider using a more concise placeholder like
"query { }"to avoid confusing users.
default: "query {\n \n}"
unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py:249
- [nitpick] Add a test case for a GraphQL request body without
variablesto verify thatrequest_body.value.dict()correctly handles queries when no additional fields are present.
)
..._cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py
Outdated
Show resolved
Hide resolved
…erpolated_request_options_provider.py Co-authored-by: Copilot <[email protected]>
📝 WalkthroughWalkthroughThis update changes the handling of GraphQL request bodies in the Airbyte CDK. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ManifestMigration
participant Schema
participant RequestOptionsProvider
User->>ManifestMigration: Provide manifest with request_body_json
ManifestMigration->>ManifestMigration: Check if "query" is string
alt "query" is string
ManifestMigration->>ManifestMigration: Migrate to RequestBodyGraphQL (query as string)
else
ManifestMigration->>ManifestMigration: Migrate to RequestBodyJsonObject
end
ManifestMigration->>Schema: Update schema (query as string)
RequestOptionsProvider->>RequestOptionsProvider: Serialize GraphQL request body as dict
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to add a note in the migration guide about the new expectation for GraphQL queries to be strings, just in case users are upgrading from older manifests—wdyt? ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
♻️ Duplicate comments (1)
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (1)
103-103: Perfect implementation of the complete GraphQL request body serialization!This change correctly serializes the entire GraphQL request body object rather than just extracting the query field. Using
.dict(exclude_none=True)ensures that optional fields likevariablesare only included when they have values, which is exactly what was suggested in the previous review. This preserves all sibling properties as mentioned in the PR objectives - great job implementing this enhancement!
🧹 Nitpick comments (2)
unit_tests/manifest_migrations/conftest.py (1)
740-746: Use POST instead of GET for GraphQL payloads?These fixtures now include a
RequestBodyGraphQLpayload but still specify"http_method": "GET".
Bodies onGETare ignored by many servers and some CDK logic may short-circuit when the method isn’t POST/PUT/PATCH, so the test might not exercise the actual serialization path. Swapping the method to"POST"would better reflect real-world GraphQL usage and guard against regressions—wdyt?Also applies to: 974-980, 1135-1140
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
4305-4307: Default GraphQL query placeholder might be invalid – reconsider the value?
"query {\n \n}"is not a syntactically valid GraphQL document because an empty selection set is forbidden. Could we switch the default to something harmless but valid, e.g.default: | query { __typename }so that users (or the Builder UI) don’t hit parser errors when the default is used unmodified—wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py(1 hunks)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/requesters/request_options/interpolated_request_options_provider.py(1 hunks)unit_tests/manifest_migrations/conftest.py(3 hunks)unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py (3)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (1)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (4)
undefined
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource class in airbyte_cdk/sources/declarative/yaml_declarative_source.py, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py file is auto-generated from declarative_component_schema.yaml and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in airbyte_cdk/cli/source_declarative_manifest/, including _run.py, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in airbyte_cdk/cli/source_declarative_manifest/ is being imported from another repository, avoid suggesting modifications to it during the import process.
</retrieved_learning>
unit_tests/manifest_migrations/conftest.py (2)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
undefined
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py file is auto-generated from declarative_component_schema.yaml and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource class in airbyte_cdk/sources/declarative/yaml_declarative_source.py, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (1)
airbyte_cdk/sources/streams/http/http.py (1)
request_body_json(229-240)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (3)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py (1)
57-57: LGTM! Correct validation for string-based GraphQL queries.The change from checking
is not Nonetoisinstance(..., str)properly validates that GraphQL queries are strings, which aligns perfectly with the PR objectives. This ensures the migration only treats request bodies as GraphQL when the query is actually a string rather than just any truthy value. Nice work!unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py (1)
242-246: Excellent test update for the new GraphQL format!The test case now correctly reflects the string-based GraphQL query approach with:
queryas a string with template interpolationvariablesas a separate dictionary for GraphQL variables- Expected output properly structured with both interpolated query and variables
This perfectly validates the new implementation behavior. The test coverage looks solid - wdyt about adding an edge case where
variablesis None to ensure theexclude_none=Truebehavior is tested?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1557-1557: LGTM! GraphQL query type correction looks good.The change from
Dict[str, Any]tostrfor thequeryfield is exactly right - GraphQL queries are indeed string-based rather than JSON objects. This aligns perfectly with the PR objectives to fix the request_body GraphQL implementation. Since this is auto-generated from the YAML schema, the change properly reflects the corrected type definition, wdyt?
When working on the Builder UI, I noticed that the GraphQL option of the new
request_bodyfield wasn't correct - it marked thequeryproperty as an object, but it is supposed to be a string, as GraphQL queries look something like:which is not a valid json object.
I therefore went through the CDK implementation of
request_bodyand its associated migration logic and switchedqueryto a string field.I also ensured that if there are sibling properties to
query(likevariables, common in graphql), those are also carried through when converting torequest_body_json, and added checks for this to the unit tests.I also updated the declarative_component_schema to have a default value for
querywith the desired default for the Builder UI, and removed theexamplesfrom therequest_bodyfield since those were confusing in the Builder UI.Summary by CodeRabbit
Bug Fixes
Documentation
Refactor