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 49s ⏱️ +17s Results for commit 77399c2. ± 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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
..._cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py
Show resolved
Hide resolved
PyTest Results (Full)4 016 tests +38 4 004 ✅ +38 11m 20s ⏱️ ±0s Results for commit 77399c2. ± 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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
I had Devin do a mock test to see if the fix works, and it does. Also verified it locally. |
Alfredo Garcia (agarctfi)
requested review from
Copilot,
Daryna Ishchenko (darynaishchenko),
Patrick Nilan (pnilan) and
Anatolii Yatsuk (tolik0)
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.
| self.request_body_json = self.request_body.value.dict(exclude_none=True) | ||
| elif self.request_body.type in ("RequestBodyJsonObject", "RequestBodyPlainText"): | ||
| elif self.request_body.type == "RequestBodyPlainText": | ||
| self.request_body_data = self.request_body.value |
There was a problem hiding this comment.
RequestBodyPlainText is now routed to request_body_data, but interpolation for string bodies still runs through JinjaInterpolation, which applies ast.literal_eval to the rendered value. This means a plain-text body that looks like a Python/JSON literal (e.g. '{"a": 1}') will be converted into a dict/int/etc, and then sent as data (url-encoded) instead of remaining a raw string. To ensure plain-text bodies stay strings, consider disabling literal-eval for this code path (e.g., pass valid_types=(str,) into the interpolation step for RequestBodyPlainText, or use an interpolator variant that does not literal-eval) and add a regression test for a JSON-looking plain-text value.
| self.request_body_data = self.request_body.value | |
| # Plain-text request bodies must remain strings even if the rendered content | |
| # looks like a Python/JSON literal. Wrapping the template in `repr(...)` | |
| # ensures the downstream interpolation/literal-eval flow resolves back to a str. | |
| self.request_body_data = repr(self.request_body.value) |
There was a problem hiding this comment.
The repr() suggestion would change the actual body content (wrapping it in quotes), which is incorrect. The ast.literal_eval concern is pre-existing in the JinjaInterpolation layer and affects all string-valued request_body_data paths equally — it's not specific to this change. Addressing it would require modifying the interpolation layer and is out of scope for this routing fix.
..._cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py
Show resolved
Hide resolved
| value="interpolate_me=5&option={{ config['option'] }}", | ||
| ), | ||
| "interpolate_me=5&option=OPTION", | ||
| ), |
There was a problem hiding this comment.
The new plain-text request body tests cover generic strings, but they don’t cover the important edge case where the rendered plain-text value is parseable by ast.literal_eval (e.g. '{"nested": {"k": "v"}}' or '5'). Given JinjaInterpolation literal-evals string results, adding a regression test here would help ensure RequestBodyPlainText stays a string and doesn’t silently become a dict/int and get url-encoded as data.
| ), | |
| ), | |
| ( | |
| "test_plain_text_with_literal_eval_numeric_result", | |
| RequestBodyPlainText( | |
| type="RequestBodyPlainText", | |
| value="{{ 5 }}", | |
| ), | |
| "5", | |
| ), | |
| ( | |
| "test_plain_text_with_literal_eval_mapping_result", | |
| RequestBodyPlainText( | |
| type="RequestBodyPlainText", | |
| value="{{ '{\"nested\": {\"k\": \"v\"}}' }}", | |
| ), | |
| '{"nested": {"k": "v"}}', | |
| ), |
There was a problem hiding this comment.
The ast.literal_eval behavior is pre-existing in JinjaInterpolation and affects all string-valued request_body_data paths (including RequestBodyUrlEncodedForm), not something introduced by this PR. Adding tests that demonstrate this behavior would be testing the interpolation layer, which is out of scope for this fix.
If this is a concern worth addressing, it should be done in a separate PR that modifies the interpolation layer itself.
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 note 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
| if manifest[TYPE_TAG] != self.component_type: | ||
| return False | ||
| for key in self.original_keys: | ||
| if key in manifest: | ||
| if key == self.body_json_key and isinstance(manifest[key], str): | ||
| continue | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Could we block the mixed legacy-body case here, wdyt?
request_body_json is skipped when it is a string, but request_body_data still migrates. A requester that has both keys then ends up with legacy request_body_json plus new request_body, and once InterpolatedRequestOptionsProvider.__post_init__ resolves request_body, that becomes the same conflicting body combination it still rejects in airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py on Lines 71-75. Could we treat that mixed input as invalid, or leave it entirely untouched, instead of partially migrating it and returning True from validate()?
Also applies to: 45-57, 68-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@airbyte_cdk/manifest_migrations/migrations/http_requester_request_body_json_data_to_request_body.py`
around lines 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.
There was a problem hiding this comment.
Having both request_body_json and request_body_data on the same requester is already invalid — InterpolatedRequestOptionsProvider.__post_init__ raises a ValueError at lines 72-75 if both are set. This was true before this PR as well.
The original migration also didn't handle this case correctly — _migrate_value pops the old key and sets request_body, so if both existed, the second call would overwrite the first's request_body value.
Since a manifest with both keys was never a valid input (and was already broken in the original migration), adding guards for this edge case would add complexity for a state that shouldn't exist. I'd prefer to leave this for a human reviewer to decide if they feel it's worth addressing.
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. |
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_typevariableReview & Testing Checklist for Human
"Request body json cannot be a string"error. This is the primary user-facing scenario.request_body_json(e.g.,'{"nested": {"key": "{{ config.option }}"}}') continue to send JSON bodies after this change, not raw string data.request_body_jsonand arequest_body_datakey migrates correctly —request_body_datashould be migrated toRequestBodyUrlEncodedFormwhile the stringrequest_body_jsonis left in place.ast.literal_evalin interpolation layer:JinjaInterpolationappliesast.literal_evalto rendered string values. This means 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 that affects all string-valuedrequest_body_datapaths (includingRequestBodyUrlEncodedForm) and is not introduced by this PR, but it is worth being aware of for plain-text bodies. A follow-up PR could address this in the interpolation layer if needed.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