Skip to content

Commit

Permalink
fix: add check to avoid NPE when binding value is null (#38104)
Browse files Browse the repository at this point in the history
## Description
Slack thread:
https://theappsmith.slack.com/archives/C04GRCN4CS3/p1733907474929399

Fixes #`Issue Number`  
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _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
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/12277298947>
> Commit: 71ba98b
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12277298947&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Wed, 11 Dec 2024 15:39:23 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Enhanced error handling for dynamic bindings by validating empty or
null values.
  
- **Bug Fixes**
- Improved test coverage for handling null binding values in dynamic
bindings.

- **Tests**
  - Added a new test for verifying behavior with null binding values.
- Updated existing tests to ensure they accurately reflect the expected
outcomes.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
subrata71 authored Dec 11, 2024
1 parent 996a30b commit 493dba0
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpStatus;
import org.springframework.util.StringUtils;
import org.springframework.web.reactive.function.client.WebClient;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
Expand Down Expand Up @@ -144,6 +145,14 @@ public Mono<Map<MustacheBindingToken, String>> refactorNameInDynamicBindings(

return Flux.fromIterable(bindingValues)
.flatMap(bindingValue -> {
if (!StringUtils.hasText(bindingValue.getValue())) {
// If the binding value is null or empty, it indicates an incorrect entry in the
// dynamicBindingPathList.
// Such bindings are considered invalid and can be safely discarded during refactoring.
// Therefore, we return an empty response.

return Mono.empty();
}
if (!bindingValue.getValue().contains(oldName)) {
// This case is not handled in RTS either, so skipping the RTS call here will not affect the
// behavior.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import reactor.test.StepVerifier;

import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -60,7 +61,8 @@ void refactorNameInDynamicBindings_validBindings_returnsUpdatedBindings() {
token1.setValue("abc['foo']");
MustacheBindingToken token2 = new MustacheBindingToken();
token2.setValue("xyz['bar']");
Set<MustacheBindingToken> bindings = Set.of(token1, token2);
MustacheBindingToken dummyToken = new MustacheBindingToken();
Set<MustacheBindingToken> bindings = Set.of(token1, token2, dummyToken);

String refactoredScript1 = "xyz['foo']";
String refactoredScript2 = "xyz['bar']";
Expand All @@ -71,7 +73,7 @@ void refactorNameInDynamicBindings_validBindings_returnsUpdatedBindings() {

doReturn(Mono.just(responseMap1))
.when(astService)
.refactorNameInDynamicBindings(Set.of(token1, token2), "abc", "xyz", 2, false);
.refactorNameInDynamicBindings(Set.of(token1, token2, dummyToken), "abc", "xyz", 2, false);

Mono<Map<MustacheBindingToken, String>> result =
astService.refactorNameInDynamicBindings(bindings, "abc", "xyz", 2, false);
Expand All @@ -85,6 +87,23 @@ void refactorNameInDynamicBindings_validBindings_returnsUpdatedBindings() {
.verifyComplete();
}

@Test
void refactorNameInDynamicBindings_nullBindingValue_returnsEmptyMap() {
MustacheBindingToken token1 = new MustacheBindingToken();

Set<MustacheBindingToken> bindings = new HashSet<>();
bindings.add(token1);

Mono<Map<MustacheBindingToken, String>> result =
astService.refactorNameInDynamicBindings(bindings, "abc", "xyz", 2, false);

StepVerifier.create(result)
.assertNext(map -> {
assertThat(map).hasSize(0);
})
.verifyComplete();
}

@Test
void refactorNameInDynamicBindings_whenValidJSObjectRequest_thenReturnUpdatedScript() {
MustacheBindingToken token = new MustacheBindingToken();
Expand Down

0 comments on commit 493dba0

Please sign in to comment.