From bf6cdbe30509dfc572f0e7dcdf8b81af1cf95894 Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Thu, 25 Mar 2021 16:00:45 +0530 Subject: [PATCH] [Bug fix] A lax search for presence of binding during save page to match client algorithm to reduce page save error (#3698) * Lax mustache binding check added to match the client side check when client recognizes a field to have a dynamic binding. This would reduce/remove bad bindings from throwing a 400 during save page. * Added a test to assert that update layout does not fail in case the binding is technically incorrect because part of the mustache's lie inside quotes. Since client has a lax way of finding a dynamic path, server also follows suite. --- .../external/helpers/MustacheHelper.java | 6 ++ .../services/LayoutActionServiceImpl.java | 7 +- .../server/services/LayoutServiceTest.java | 76 ++++++++++++++++++- 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java index c820f63811..e6ee0eeea6 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java @@ -41,6 +41,8 @@ public class MustacheHelper { private static Pattern quoteQuestionPattern = Pattern.compile(regexQuotesTrimming); // The final replacement string of ? for replacing '?' or "?" private static String postQuoteTrimmingQuestionMark = "\\?"; + private static String laxMustacheBindingRegex = "\\{\\{([\\s\\S]*?)\\}\\}"; + private static Pattern laxMustacheBindingPattern = Pattern.compile(laxMustacheBindingRegex); /** @@ -364,4 +366,8 @@ public class MustacheHelper { return body; } + + public static Boolean laxIsBindingPresentInString(String input) { + return laxMustacheBindingPattern.matcher(input).find(); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java index bff3d09ed5..991926a7ed 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java @@ -353,19 +353,20 @@ public class LayoutActionServiceImpl implements LayoutActionService { } // Only extract mustache keys from leaf nodes if (isLeafNode) { - Set mustacheKeysFromFields = MustacheHelper.extractMustacheKeysFromFields(parent); // We found the path. But if the path does not have any mustache bindings, throw the error - if (mustacheKeysFromFields.isEmpty()) { + if (!MustacheHelper.laxIsBindingPresentInString((String) parent)) { try { String bindingAsString = objectMapper.writeValueAsString(parent); throw new AppsmithException(AppsmithError.INVALID_DYNAMIC_BINDING_REFERENCE, widgetType, - widgetName, widgetId, fieldPath, pageId, layoutId, bindingAsString); + widgetName, widgetId, fieldPath, pageId, layoutId, bindingAsString); } catch (JsonProcessingException e) { throw new AppsmithException(AppsmithError.JSON_PROCESSING_ERROR, parent); } } + // Stricter extraction of dynamic bindings + Set mustacheKeysFromFields = MustacheHelper.extractMustacheKeysFromFields(parent); dynamicBindings.addAll(mustacheKeysFromFields); } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java index a6f90dd652..64ac1e9e77 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java @@ -452,7 +452,7 @@ public class LayoutServiceTest { @Test @WithUserDetails(value = "api_user") - public void testIncorrectDynamicBinding() { + public void testIncorrectDynamicBindingPathInDsl() { Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor())); PageDTO testPage = new PageDTO(); @@ -525,6 +525,80 @@ public class LayoutServiceTest { .verify(); } + @Test + @WithUserDetails(value = "api_user") + public void testIncorrectMustacheExpressionInBindingInDsl() { + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor())); + + PageDTO testPage = new PageDTO(); + testPage.setName("testIncorrectMustacheExpressionInBinding Test Page"); + + Application app = new Application(); + app.setName("newApplication-testIncorrectMustacheExpressionInBinding-Test"); + + PageDTO page = createPage(app, testPage).block(); + String pageId = page.getId(); + String layoutId = page.getLayouts().get(0).getId(); + + Mono testMono = Mono.just(page) + .flatMap(page1 -> { + List> monos = new ArrayList<>(); + + ActionDTO action = new ActionDTO(); + action.setName("aGetAction"); + action.setActionConfiguration(new ActionConfiguration()); + action.getActionConfiguration().setHttpMethod(HttpMethod.GET); + action.setPageId(page1.getId()); + action.setDatasource(datasource); + monos.add(newActionService.createAction(action)); + + return Mono.zip(monos, objects -> page1); + }) + .zipWhen(page1 -> { + Layout layout = new Layout(); + + JSONObject obj = new JSONObject(Map.of( + "key", "value" + )); + layout.setDsl(obj); + + return layoutService.createLayout(page1.getId(), layout); + }) + .flatMap(tuple2 -> { + final PageDTO page1 = tuple2.getT1(); + final Layout layout = tuple2.getT2(); + + Layout newLayout = new Layout(); + + JSONObject obj = new JSONObject(Map.of( + "widgetName", "testWidget", + "widgetId", "id", + "type", "test_type", + "key", "value-updated", + "dynamicGet", "a\"{{aGetAction\"/'\"'}}\"\"" + )); + JSONArray dynamicBindingsPathList = new JSONArray(); + dynamicBindingsPathList.addAll(List.of( + new JSONObject(Map.of("key", "dynamicGet")) + )); + + obj.put("dynamicBindingPathList", dynamicBindingsPathList); + newLayout.setDsl(obj); + + return layoutActionService.updateLayout(page1.getId(), layout.getId(), newLayout); + }); + + StepVerifier + .create(testMono) + .assertNext(layoutDTO -> { + // We have reached here means we didnt get a throwable. Thats good + assertThat(layoutDTO).isNotNull(); + // Since this is still a bad mustache binding, we couldn't have extracted the action name + assertThat(layoutDTO.getLayoutOnLoadActions().size()).isEqualTo(0); + }) + .verifyComplete(); + } + @After public void purgePages() { newPageService.deleteAll();