fix: route RequestBodyPlainText to request_body_data instead of request_body_json#971
Conversation
…st_body_json Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1775019092-fix-request-body-plain-text#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1775019092-fix-request-body-plain-textPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)4 013 tests +38 4 002 ✅ +38 7m 32s ⏱️ ±0s Results for commit 237667c. ± Comparison against base commit 69cd63d. This pull request removes 8 and adds 46 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)4 016 tests +38 4 004 ✅ +38 10m 56s ⏱️ -24s Results for commit 237667c. ± Comparison against base commit 69cd63d. This pull request removes 8 and adds 46 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…migration String-valued request_body_json fields are no longer migrated to RequestBodyPlainText (which now correctly routes to request_body_data). Instead, they are left as request_body_json to preserve their original JSON body semantics via InterpolatedNestedRequestInputProvider. Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
|
I had Devin do a mock test to see if the fix works, and it does. Also verified it locally. |
There was a problem hiding this comment.
Pull request overview
Fixes request body handling so RequestBodyPlainText is treated as non-JSON body data (avoiding "Request body json cannot be a string" errors), and adjusts the manifest migration to preserve string-valued request_body_json for backward compatibility.
Changes:
- Route
RequestBodyPlainTexttorequest_body_datawhile keepingRequestBodyJsonObjectrouted torequest_body_json. - Update
HttpRequesterRequestBodyJsonDataToRequestBodymigration to skip migrating string-valuedrequest_body_jsonand adjust validation accordingly. - Update unit tests/fixtures to reflect the corrected plain-text behavior and the updated migration expectations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py |
Corrects request-body routing for plain text vs JSON object. |
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py |
Skips migrating string request_body_json to preserve legacy JSON-template behavior. |
unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py |
Replaces the removed “plain text parsed as JSON” case with explicit plain-text body tests. |
unit_tests/manifest_migrations/conftest.py |
Updates expected migrated manifests to keep string request_body_json unchanged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefines migration and runtime resolution to distinguish string-valued plain-text request bodies from structured JSON objects, preventing automatic conversion of string Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Would you like me to add a brief inline comment in the migration script explaining why string 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py`:
- Around line 29-36: The validator currently skips migrating a string
request_body_json but still migrates request_body_data, causing mixed legacy/new
body pairs; update validate() (and the analogous checks around the other blocks)
to detect when both body_json_key (request_body_json) exists as a string AND
request_body_data_key (request_body_data) is also present and in that case treat
the manifest as invalid/untouched by returning False (i.e. do not return True
for migration). Specifically, in the loop that checks original_keys inside
validate(), add a guard: if key == self.body_json_key and
isinstance(manifest[key], str) and self.request_body_data_key in manifest:
return False; mirror this logic in the other two similar places mentioned so
mixed legacy+new bodies are not partially migrated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9cacbd2-20ec-44f5-a80f-c9b54926b1a4
📒 Files selected for processing (4)
airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.pyairbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.pyunit_tests/manifest_migrations/conftest.pyunit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py
Alfredo Garcia (@agarctfi) Do we have such connectors? If we don't I think we can just go with the pure fix in InterpolatedRequestOptionsProvider without the migration. |
|
Daryna Ishchenko (@darynaishchenko) On Cloud, there is 1 connection, not sure about OSS or if we would even count that. Here is where I checked. |
Alfredo Garcia (@agarctfi) could you please verify if this connection will actually fail after applying a cdk fix? I think because of this #971 (comment) it should work fine. Were you able to reproduce the issue that is handled in the migration change? |
|
Daryna Ishchenko (@darynaishchenko) I checked, and that connector is for Shopify, so I used our test creds. Without the migration, we get a With the migration, we don't have this issue, so I think we should leave it. WDYT? Here is where I tested. |
Alfredo Garcia (@agarctfi) Thanks for testing it!
|
…grated Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
|
Daryna Ishchenko (@darynaishchenko) Thanks for the thorough review! Addressing each point: 1. Risk of keeping 2. Migrating to 3. Docstring: Added in |
Yes, it looks like Devin added the docstring, which looks good to me. Let me know if we need anything else for this one. Daryna Ishchenko (@darynaishchenko) , thanks! |
Summary
RequestBodyPlainTextwas incorrectly grouped withRequestBodyJsonObjectinInterpolatedRequestOptionsProvider._resolve_request_body(), causing plain text body values to be assigned torequest_body_jsoninstead ofrequest_body_data. This resulted in aValueError("Request body json cannot be a string")whenever a user configured a plain-text request body in the Connector Builder — for example, when creating an asynchronous job with a non-JSON body.The fix splits the combined
elifbranch so thatRequestBodyPlainTextroutes toself.request_body_data(likeRequestBodyUrlEncodedFormalready does), whileRequestBodyJsonObjectcontinues to route toself.request_body_json.Migration backward compatibility
The manifest migration (
HttpRequesterRequestBodyJsonDataToRequestBody) previously converted string-valuedrequest_body_jsonfields toRequestBodyPlainText. Before this fix, that worked by accident becauseRequestBodyPlainTextwas (incorrectly) routed torequest_body_json. Now thatRequestBodyPlainTextcorrectly routes torequest_body_data, the migration would break existing connectors that rely on string JSON templates being sent as JSON bodies.The fix: string-valued
request_body_jsonfields are now skipped by the migration and left asrequest_body_json, whereInterpolatedNestedRequestInputProvideralready handles them natively (interpolating the template string and parsing it viaast.literal_evalinto a dict).Changes to the migration:
should_migratereturnsFalsewhen the only migratable key is a stringrequest_body_json_migrate_body_jsonreturns early (no-op) for string values instead of converting toRequestBodyPlainTextvalidateaccepts manifests where stringrequest_body_jsonremains unmigratedtext_typevariablerequest_body_jsonis intentionally preservedReview & Testing Checklist for Human
"Request body json cannot be a string"error. This is the primary user-facing scenario.request_body_json: Confirm that connectors currently using string-valuedrequest_body_json(e.g., Shopify GraphQL) continue to send JSON bodies with the correctContent-Typeafter this change. Alfredo Garcia (@agarctfi) verified that without the migration fix, Shopify gets a415 Unsupported Media Type; with it, the connector works. Worth a second verification.validate()logic: The three-state validation (has_replacement,has_unmigrated,has_string_json) is the most complex part of this change. Verify that manifests with only a stringrequest_body_json(no other body keys) pass validation without requiringrequest_bodyto be present.ast.literal_evalin interpolation layer:JinjaInterpolationappliesast.literal_evalto rendered string values. A plain-text body whose content looks like a Python literal (e.g.,{"a": 1}or5) could be parsed into a dict/int rather than staying a string. This is pre-existing behavior not introduced by this PR, but is now more relevant forRequestBodyPlainTextbodies routed throughrequest_body_data.Notes
"test_string"intest_interpolated_request_json_using_request_bodywas encoding the buggy behavior: it expectedRequestBodyPlainTextwith value'{"nested": { "key": "..." }}'to produce a parsed JSON dict. This test has been removed and replaced with two new cases intest_interpolated_request_data_using_request_body.conftest.pyhave been updated so the expected output for stream "D" (stringrequest_body_json) retains the originalrequest_body_jsonkey rather than converting toRequestBodyPlainText.airbytehq/oncall#10360Link to Devin session: https://app.devin.ai/sessions/8a5ed7c6c890468ca63a35469e5c58c1
Requested by: Airbyte Support (@Airbyte-Support)
Summary by CodeRabbit