fix: support dynamic body for form-urlencoded content type in REST API - #42016
fix: support dynamic body for form-urlencoded content type in REST API#42016mmustafasenoglu wants to merge 2 commits into
Conversation
When using mustache bindings like `{{ this.params.body }}` with
application/x-www-form-urlencoded content type, the body was ignored
because the code always read from bodyFormData (the static key-value
editor). This fix falls back to parsing the body string when bodyFormData
is empty.
Supports both:
- URL-encoded strings: key=value&key2=value2
- JSON objects: {"key": "value"}
Fixes appsmithorg#42006
Walkthrough
ChangesForm body parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java (2)
554-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
Propertyinstantiation. Both sites manually instantiate aPropertyobject and use setters; they can be simplified by using the existingProperty(String key, Object value)convenience constructor.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java#L554-L558: Replace withproperties.add(new Property(entry.getKey(), entry.getValue() != null ? entry.getValue().toString() : ""));app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java#L584-L588: Replace withproperties.add(new Property(key, value));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java` around lines 554 - 558, Simplify Property creation in DataUtils at lines 554-558 and 584-588 by replacing manual instantiation and setter calls with the existing Property(String key, Object value) constructor; preserve the current key and value expressions at each site.
561-563: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCatch specific exceptions instead of a generic
Exception.Catching a generic
Exceptionis generally discouraged as it can silently swallow unexpected runtime errors like aNullPointerException. Consider catching the specific exceptions thrown byobjectFromJsonto ensure unexpected issues aren't masked as a fall-through.♻️ Proposed refactor
- } catch (Exception e) { + } catch (JsonSyntaxException | ParseException e) { // Fall through to URL-encoded parsing }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java` around lines 561 - 563, Update the catch block in the objectFromJson parsing flow to catch only the specific parsing/JSON exceptions that objectFromJson is expected to throw, rather than generic Exception. Preserve the existing fall-through to URL-encoded parsing for those expected failures while allowing unexpected runtime errors to propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java`:
- Around line 573-582: The form-data parsing logic around URLDecoder.decode must
gracefully handle malformed percent-encoded input and preserve encoded parameter
keys. Catch IllegalArgumentException in the parsing method and throw the
established AppsmithPluginException with a clear malformed-body message; update
parseFormData to URL-encode keys as well as values when encodeParamsToggle is
enabled, ensuring pre-encoded keys such as line_items%5B0%5D remain valid in the
outgoing payload.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java`:
- Around line 554-558: Simplify Property creation in DataUtils at lines 554-558
and 584-588 by replacing manual instantiation and setter calls with the existing
Property(String key, Object value) constructor; preserve the current key and
value expressions at each site.
- Around line 561-563: Update the catch block in the objectFromJson parsing flow
to catch only the specific parsing/JSON exceptions that objectFromJson is
expected to throw, rather than generic Exception. Preserve the existing
fall-through to URL-encoded parsing for those expected failures while allowing
unexpected runtime errors to propagate.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 390fea8c-28a8-448a-801a-6e473513f116
📒 Files selected for processing (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
| if (eqIndex > 0) { | ||
| key = URLDecoder.decode(pair.substring(0, eqIndex), StandardCharsets.UTF_8); | ||
| value = URLDecoder.decode(pair.substring(eqIndex + 1), StandardCharsets.UTF_8); | ||
| } else if (eqIndex == 0) { | ||
| // key is empty, skip | ||
| continue; | ||
| } else { | ||
| key = URLDecoder.decode(pair, StandardCharsets.UTF_8); | ||
| value = ""; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle IllegalArgumentException from URL decoding and address key-encoding mismatch.
URLDecoder.decode throws an unhandled IllegalArgumentException if it encounters invalid escape sequences (e.g., %ZZ), which will cause the API execution to crash with an unhandled 500 error instead of a graceful plugin error.
Additionally, decoding the key here creates a correctness issue across boundaries. The downstream parseFormData method (lines 149-173) encodes the value but intentionally or accidentally does not encode the key. If a user provides a pre-encoded string like line_items%5B0%5D=10, decoding it here to line_items[0] means it will eventually be sent unencoded (line_items[0]=10), generating a corrupted HTTP payload.
Consider catching IllegalArgumentException to return a clear error, and review whether parseFormData should be fixed to properly encode keys when encodeParamsToggle is true.
🛠️ Example fix for the unhandled exception
try {
if (eqIndex > 0) {
key = URLDecoder.decode(pair.substring(0, eqIndex), StandardCharsets.UTF_8);
value = URLDecoder.decode(pair.substring(eqIndex + 1), StandardCharsets.UTF_8);
} else if (eqIndex == 0) {
// key is empty, skip
continue;
} else {
key = URLDecoder.decode(pair, StandardCharsets.UTF_8);
value = "";
}
} catch (IllegalArgumentException e) {
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Malformed URL-encoded body: " + e.getMessage());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java`
around lines 573 - 582, The form-data parsing logic around URLDecoder.decode
must gracefully handle malformed percent-encoded input and preserve encoded
parameter keys. Catch IllegalArgumentException in the parsing method and throw
the established AppsmithPluginException with a clear malformed-body message;
update parseFormData to URL-encode keys as well as values when
encodeParamsToggle is enabled, ensuring pre-encoded keys such as
line_items%5B0%5D remain valid in the outgoing payload.
Addressing CodeRabbit ReviewThanks for the thorough review! Here's how I addressed the findings: ** on malformed URL-encoded input:**
Key encoding mismatch:
Changes are minimal and backwards-compatible — URL-encoded payloads are now handled as raw strings, which is the standard approach for form-urlencoded content types. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java`:
- Around line 573-579: The parseFormData flow currently double-encodes raw
URL-encoded values when encodeParamsToggle is enabled. Update the fallback
branch around key/value extraction to carry an “already encoded” state for
URL-encoded input, and make the later URLEncoder.encode logic skip values in
that state while preserving encoding for JSON-derived properties.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4847861b-61b7-400f-87bc-0a6f8fa75f54
📒 Files selected for processing (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
| key = pair.substring(0, eqIndex); | ||
| value = pair.substring(eqIndex + 1); | ||
| } else if (eqIndex == 0) { | ||
| // key is empty, skip | ||
| continue; | ||
| } else { | ||
| key = pair; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid double-encoding raw form values.
The fallback now preserves the raw value, but parseFormData() later applies URLEncoder.encode() when encodeParamsToggle is enabled. For example, a=hello%20world becomes a=hello%2520world, so pre-encoded payloads are corrupted. Carry an “already encoded” mode for URL-encoded input and skip value encoding in that path while retaining encoding for JSON-derived properties.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java`
around lines 573 - 579, The parseFormData flow currently double-encodes raw
URL-encoded values when encodeParamsToggle is enabled. Update the fallback
branch around key/value extraction to carry an “already encoded” state for
URL-encoded input, and make the later URLEncoder.encode logic skip values in
that state while preserving encoding for JSON-derived properties.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi! Just checking in — this PR fixes a critical bug where Happy to address any review feedback or make changes if needed! |
|
Addressing CodeRabbit Review Findings: Thanks for the thorough review! Here is my analysis: Finding 1 — The current code no longer uses Finding 2 — Double-encoding with Since The intent of this PR is to fix the critical bug (#42006) where |
|
Hi! Just checking in on this PR — it fixes a critical bug (#42006) where |
1 participant
Description
Fixes #42006
When using
application/x-www-form-urlencodedcontent type in the REST API plugin, dynamic body bindings (mustache expressions) in the body field were silently ignored. This happened becausegetRequestBodyObject()always read frombodyFormData(the static key-value editor data) and never checked thebodyfield for form-urlencoded content types.Root Cause
In
DataUtils.getRequestBodyObject(), line 516-518:The
bodyfield (where mustache bindings like{{ this.params.body }}are resolved) was completely ignored.Fix
Added a fallback: when
bodyFormDatais null/empty butbodyis not empty, parse the body string intoList<Property>using a newparseFormUrlEncodedBodyString()method.Supports both:
key=value&key2=value2→ parsed viaURLDecoder{"key": "value"}→ parsed via existingobjectFromJson()and converted to propertiesChanges
DataUtils.java: ModifiedgetRequestBodyObject()to fall back to body parsing whenbodyFormDatais emptyDataUtils.java: AddedparseFormUrlEncodedBodyString()helper methodTesting
Users can now dynamically generate form-urlencoded bodies from JSObjects:
Or pass a pre-encoded string via params:
Summary by CodeRabbit
application/x-www-form-urlencodedandmultipart/form-datarequest bodies.