From 6d41e77b1ebf974e7f4e55ab5adb1fa23064118e Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Thu, 5 Dec 2024 21:14:23 +0600 Subject: [PATCH] chore: Skip unnecessary RTS calls to optimise performance (#37949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Motivation came from this [PoC](https://www.notion.so/appsmith/Evaluating-Performance-Bottlenecks-in-Pull-Based-Upgrades-Challenges-with-New-Relic-Telemetry-14bfe271b0e28063b9fdc08515ab3014) Fixes https://github.com/appsmithorg/appsmith/issues/37948 ## Automation /ok-to-test tags="@tag.All" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: d1f53e20d460cdf90bed3eef5f0552b7d871c197 > Cypress dashboard. > Tags: `@tag.All` > Spec: >
Thu, 05 Dec 2024 15:10:54 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling in the binding refactoring process to ensure smoother operation. - **Refactor** - Enhanced efficiency of the binding refactoring method by adding a conditional check to reduce unnecessary server calls. - **Tests** - Introduced a new test suite for the binding refactoring method, validating various scenarios to ensure correct functionality. --- .../server/services/ce/AstServiceCEImpl.java | 9 + .../server/services/ce/ASTServiceCETest.java | 154 ++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java index cd5b32be74..950cc8578c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java @@ -144,6 +144,15 @@ public class AstServiceCEImpl implements AstServiceCE { return Flux.fromIterable(bindingValues) .flatMap(bindingValue -> { + if (!bindingValue.getValue().contains(oldName)) { + // This case is not handled in RTS either, so skipping the RTS call here will not affect the + // behavior. + // Example: + // - Old name: foo.bar + // - New name: foo.baz + // - Binding: "foo['bar']" + return Mono.just(Tuples.of(bindingValue, bindingValue.getValue())); + } EntityRefactorRequest entityRefactorRequest = new EntityRefactorRequest( bindingValue.getValue(), oldName, newName, evalVersion, isJSObject); return rtsCaller diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java new file mode 100644 index 0000000000..09f91cba61 --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java @@ -0,0 +1,154 @@ +package com.appsmith.server.services.ce; + +import com.appsmith.external.models.MustacheBindingToken; +import com.appsmith.server.services.AstService; +import lombok.extern.slf4j.Slf4j; +import org.junit.jupiter.api.Test; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.SpyBean; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.util.Collections; +import java.util.Map; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doReturn; + +@SpringBootTest +@Slf4j +public class ASTServiceCETest { + @SpyBean + AstService astService; + + @Test + void refactorNameInDynamicBindings_nullOrEmptyBindings_returnsEmptyMono() { + Mono> result = + astService.refactorNameInDynamicBindings(null, "abc", "xyz", 2, false); + + StepVerifier.create(result).verifyComplete(); + + result = astService.refactorNameInDynamicBindings(Collections.emptySet(), "abc", "xyz", 2, false); + + StepVerifier.create(result).verifyComplete(); + } + + @Test + void refactorNameInDynamicBindings_bindingWithoutOldName_returnsUnchangedMap() { + MustacheBindingToken token1 = new MustacheBindingToken(); + token1.setValue("foo.bar"); + MustacheBindingToken token2 = new MustacheBindingToken(); + token2.setValue("baz.qux"); + Set bindings = Set.of(token1, token2); + + Mono> result = + astService.refactorNameInDynamicBindings(bindings, "abc", "xyz", 2, false); + + StepVerifier.create(result) + .assertNext(map -> { + assertThat(map).hasSize(2); + assertThat(map.get(token1)).isEqualTo("foo.bar"); + assertThat(map.get(token2)).isEqualTo("baz.qux"); + }) + .verifyComplete(); + } + + @Test + void refactorNameInDynamicBindings_validBindings_returnsUpdatedBindings() { + MustacheBindingToken token1 = new MustacheBindingToken(); + token1.setValue("abc['foo']"); + MustacheBindingToken token2 = new MustacheBindingToken(); + token2.setValue("xyz['bar']"); + Set bindings = Set.of(token1, token2); + + String refactoredScript1 = "xyz['foo']"; + String refactoredScript2 = "xyz['bar']"; + + Map responseMap1 = Map.of( + token1, refactoredScript1, + token2, refactoredScript2); + + doReturn(Mono.just(responseMap1)) + .when(astService) + .refactorNameInDynamicBindings(Set.of(token1, token2), "abc", "xyz", 2, false); + + Mono> result = + astService.refactorNameInDynamicBindings(bindings, "abc", "xyz", 2, false); + + StepVerifier.create(result) + .assertNext(map -> { + assertThat(map).hasSize(2); // Only one binding refactored + assertThat(map.get(token1)).isEqualTo(refactoredScript1); + assertThat(map.get(token2)).isEqualTo(refactoredScript2); + }) + .verifyComplete(); + } + + @Test + void refactorNameInDynamicBindings_whenValidJSObjectRequest_thenReturnUpdatedScript() { + MustacheBindingToken token = new MustacheBindingToken(); + token.setValue("export default { myFun1() { Api1.run(); return Api1.data;}}"); + Set bindingValues = Set.of(token); + String oldName = "Api1"; + String newName = "GetUsers"; + int evalVersion = 2; + boolean isJSObject = true; + + String refactoredScript = "export default { myFun1() { GetUsers.run(); return GetUsers.data;}}"; + + Map responseMap = Map.of(token, refactoredScript); + + doReturn(Mono.just(responseMap)) + .when(astService) + .refactorNameInDynamicBindings(bindingValues, oldName, newName, evalVersion, isJSObject); + + Mono> resultMono = + astService.refactorNameInDynamicBindings(bindingValues, oldName, newName, evalVersion, isJSObject); + + StepVerifier.create(resultMono) + .assertNext(result -> { + assertThat(result).hasSize(1); + + MustacheBindingToken key = bindingValues.iterator().next(); + + assertThat(result.containsKey(key)).isTrue(); + assertThat(result.get(key)).isEqualTo(refactoredScript); + }) + .verifyComplete(); + } + + @Test + void refactorNameInDynamicBindings_whenNoMatchingOldNameInJSObject_thenReturnOriginalScript() { + MustacheBindingToken token = new MustacheBindingToken(); + token.setValue("export default { myFun1() { GetUsers.run(); return GetUsers.data;}}"); + Set bindingValues = Set.of(token); + + String oldName = "Api1"; // oldName is not present in the script + String newName = "GetUsers"; + int evalVersion = 2; + boolean isJSObject = true; + + String refactoredScript = "export default { myFun1() { GetUsers.run(); return GetUsers.data;}}"; + + Map responseMap = Map.of(token, refactoredScript); + + doReturn(Mono.just(responseMap)) + .when(astService) + .refactorNameInDynamicBindings(bindingValues, oldName, newName, evalVersion, isJSObject); + + Mono> resultMono = + astService.refactorNameInDynamicBindings(bindingValues, oldName, newName, evalVersion, isJSObject); + + StepVerifier.create(resultMono) + .assertNext(result -> { + assertThat(result).hasSize(1); + + MustacheBindingToken key = bindingValues.iterator().next(); + + assertThat(result.containsKey(key)).isTrue(); + assertThat(result.get(key)).isEqualTo(token.getValue()); // Script remains unchanged + }) + .verifyComplete(); + } +}