fix: enhance null handling in action parameter evaluation and rendering #40910
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…evaluation and rendering processes. Specifically, it ensures that null values are treated appropriately, preventing unnecessary stringification and Blob creation. This change enhances the robustness of the action execution flow and prevents potential errors related to null values.
WalkthroughThese changes refine the handling of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginActionSaga
participant Server
participant PluginExecutor
participant MustacheHelper
Client->>PluginActionSaga: Trigger action with parameters (may include nulls)
PluginActionSaga->>PluginActionSaga: Evaluate parameters (preserve nulls)
PluginActionSaga->>Server: Send evaluated parameters
Server->>PluginExecutor: Execute action (sanitise nulls)
PluginExecutor->>MustacheHelper: Render mustache tokens (preserve formatting if missing)
MustacheHelper-->>PluginExecutor: Return rendered result
PluginExecutor-->>Server: Return execution result
Server-->>Client: Respond with action result
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (1)
316-326: Clarify the purpose of converting nulls to "null" strings.This method converts null parameter values to the string "null", which seems counterintuitive given the PR's goal of enhancing null handling. Consider adding documentation explaining when and why this conversion is necessary for plugin compatibility.
+ /** + * Sanitizes null parameter values by converting them to string "null". + * This is required for plugin compatibility where null values cannot be processed. + */ default void sanitiseNullsInParams(ExecuteActionDTO executeActionDTO) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-lint / client-lint
🔇 Additional comments (7)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (2)
492-499: LGTM - Proper null preservation in JSON serializationThe explicit null check prevents
JSON.stringify(null)from convertingnullto the string"null", correctly preserving the null value in the evaluated parameters.
505-510: LGTM - Consistent null handling for Blob creationGood addition to prevent creating a Blob with
"null"content when the value is actually null. This maintains consistency with the null handling approach in the previous segment.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (2)
66-66: Required import for new FormFieldPart usageThe import is necessary for the type checking in the parseExecuteParameter method.
345-346: Clear documentation of the architectural changeGood practice to document the removal of the previous null handling approach.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java (1)
344-354: LGTM! Improved null handling in mustache token rendering.The logic correctly preserves original token formatting when keys are missing and only replaces tokens with non-null binding values. This prevents unnecessary conversion of null values to strings during template rendering.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (2)
308-308: Addition of null sanitization call looks appropriate.The call to
sanitiseNullsInParamsis placed at the right entry point to ensure consistent parameter handling before execution.
393-407: Improved null preservation in variable substitution.The refactored logic correctly preserves null values instead of converting them to strings, and the explicit loop with
putIfAbsentproperly handles duplicate keys. This aligns well with the PR's null handling improvements.
Comment on lines
+509
to
+518
|
|
||
| if (part instanceof FormFieldPart) { | ||
| FormFieldPart formFieldPart = (FormFieldPart) part; | ||
| if ("null".equals(formFieldPart.value())) { | ||
| param.setValue(null); | ||
| } else if (formFieldPart.value().isEmpty()) { | ||
| param.setValue(formFieldPart.value()); | ||
| } | ||
| return Mono.just(param); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify string literal comparison and consider edge cases
The logic correctly handles null and empty string cases for FormFieldPart, but consider these potential improvements:
- The string comparison
"null".equals(formFieldPart.value())could be more robust - Missing
elseclause means the value remains unset for non-empty, non-null cases
Consider this more explicit implementation:
if (part instanceof FormFieldPart) {
FormFieldPart formFieldPart = (FormFieldPart) part;
- if ("null".equals(formFieldPart.value())) {
+ String value = formFieldPart.value();
+ if ("null".equals(value)) {
param.setValue(null);
- } else if (formFieldPart.value().isEmpty()) {
- param.setValue(formFieldPart.value());
+ } else if (value.isEmpty()) {
+ param.setValue("");
+ } else {
+ param.setValue(value);
}
return Mono.just(param);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (part instanceof FormFieldPart) { | |
| FormFieldPart formFieldPart = (FormFieldPart) part; | |
| if ("null".equals(formFieldPart.value())) { | |
| param.setValue(null); | |
| } else if (formFieldPart.value().isEmpty()) { | |
| param.setValue(formFieldPart.value()); | |
| } | |
| return Mono.just(param); | |
| } | |
| if (part instanceof FormFieldPart) { | |
| FormFieldPart formFieldPart = (FormFieldPart) part; | |
| String value = formFieldPart.value(); | |
| if ("null".equals(value)) { | |
| param.setValue(null); | |
| } else if (value.isEmpty()) { | |
| param.setValue(""); | |
| } else { | |
| param.setValue(value); | |
| } | |
| return Mono.just(param); | |
| } |
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
around lines 509 to 518, the current code compares the string literal "null" to
formFieldPart.value() which may not be robust and lacks handling for non-empty,
non-"null" values. Update the comparison to safely handle null values and add an
else clause to set param's value for all other cases, ensuring param.setValue is
always called appropriately.
Failed server tests
|
…ming the surrounding braces. This change enhances the clarity of the token processing logic, ensuring that the rendered output accurately reflects the intended values without leading or trailing whitespace.
mohanarpit
approved these changes
Jun 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This update improves the handling of null values in action parameter evaluation and rendering processes. Specifically, it ensures that null values are treated appropriately, preventing unnecessary stringification and Blob creation. This change enhances the robustness of the action execution flow and prevents potential errors related to null values.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15555494788
Commit: b869dfa
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 10 Jun 2025 10:21:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit