From 73f7ea379120f886d6b96492c00879a424ba90c1 Mon Sep 17 00:00:00 2001 From: Hetu Nandu Date: Fri, 19 Mar 2021 14:21:14 +0530 Subject: [PATCH 1/6] Trigger client integration only on approved PRs (#3632) Co-authored-by: Nidhi Co-authored-by: Nidhi --- .github/workflows/client-build.yml | 115 ++++++++++++++++++ .../workflows/{client.yml => client-test.yml} | 23 ++-- app/client/README.md | 1 - 3 files changed, 124 insertions(+), 15 deletions(-) create mode 100644 .github/workflows/client-build.yml rename .github/workflows/{client.yml => client-test.yml} (96%) diff --git a/.github/workflows/client-build.yml b/.github/workflows/client-build.yml new file mode 100644 index 0000000000..7536baa8eb --- /dev/null +++ b/.github/workflows/client-build.yml @@ -0,0 +1,115 @@ +name: Appsmith Client Build Workflow + +on: + # This line enables manual triggering of this workflow. + workflow_dispatch: + + push: + branches: [release, master] + # Only trigger if files have changed in this specific path + paths: + - 'app/client/**' + - '!app/client/cypress/manual_TestSuite/**' + + pull_request: + branches: [release, master] + paths: + - 'app/client/**' + - '!app/client/cypress/manual_TestSuite/**' + +# Change the working directory for all the jobs in this workflow +defaults: + run: + working-directory: app/client + +jobs: + build: + runs-on: ubuntu-latest + defaults: + run: + working-directory: app/client + shell: bash + + steps: + # Checkout the code + - name: Checkout the merged commit from PR and base branch + if: github.event_name == 'pull_request' + uses: actions/checkout@v2 + with: + fetch-depth: 0 + ref: refs/pull/${{ github.event.pull_request.number }}/merge + + - name: Checkout the head commit of the branch + if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' + uses: actions/checkout@v2 + with: + fetch-depth: 0 + + - name: Figure out the PR number + run: echo ${{ github.event.pull_request.number }} + + - name: Use Node.js 14.15.4 + uses: actions/setup-node@v1 + with: + node-version: "14.15.4" + + - name: Get yarn cache directory path + id: yarn-dep-cache-dir-path + run: echo "::set-output name=dir::$(yarn cache dir)" + + # Retrieve npm dependencies from cache. After a successful run, these dependencies are cached again + - name: Cache npm dependencies + id: yarn-dep-cache + uses: actions/cache@v2 + env: + cache-name: cache-yarn-dependencies + with: + path: | + ${{ steps.yarn-dep-cache-dir-path.outputs.dir }} + key: ${{ runner.os }}-yarn-dep-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn-dep- + + # Install all the dependencies + - name: Install dependencies + run: yarn install + + - name: Set the build environment based on the branch + id: vars + run: | + echo "::set-output name=REACT_APP_ENVIRONMENT::DEVELOPMENT" + if [[ "${{github.ref}}" == "refs/heads/master" ]]; then + echo "::set-output name=REACT_APP_ENVIRONMENT::PRODUCTION" + fi + if [[ "${{github.ref}}" == "refs/heads/release" ]]; then + echo "::set-output name=REACT_APP_ENVIRONMENT::STAGING" + fi + # Since this is an unreleased build, we set the version to incremented version number with + # a `-SNAPSHOT` suffix. + latest_released_version="$(git tag --list 'v*' --sort=-version:refname | head -1)" + echo "latest_released_version = $latest_released_version" + next_version="$(echo "$latest_released_version" | awk -F. -v OFS=. '{ $NF++; print }')" + echo "next_version = $next_version" + echo ::set-output name=version::$next_version-SNAPSHOT + + - name: Run the jest tests + run: REACT_APP_ENVIRONMENT=${{steps.vars.outputs.REACT_APP_ENVIRONMENT}} yarn run test:unit + + # We burn React environment & the Segment analytics key into the build itself. + # This is to ensure that we don't need to configure it in each installation + - name: Create the bundle + run: | + REACT_APP_ENVIRONMENT=${{steps.vars.outputs.REACT_APP_ENVIRONMENT}} \ + REACT_APP_FUSIONCHARTS_LICENSE_KEY=${{ secrets.APPSMITH_FUSIONCHARTS_LICENSE_KEY }} \ + REACT_APP_SEGMENT_CE_KEY=${{ secrets.APPSMITH_SEGMENT_CE_KEY }} \ + SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} \ + REACT_APP_VERSION_ID=${{ steps.vars.outputs.version }} \ + REACT_APP_VERSION_RELEASE_DATE=$(date -u '+%Y-%m-%dT%H:%M:%SZ') \ + yarn build + + # Upload the build artifact so that it can be used by the test & deploy job in the workflow + - name: Upload react build bundle + uses: actions/upload-artifact@v2 + with: + name: build + path: app/client/build/ diff --git a/.github/workflows/client.yml b/.github/workflows/client-test.yml similarity index 96% rename from .github/workflows/client.yml rename to .github/workflows/client-test.yml index 776436ea9c..a3d6282675 100644 --- a/.github/workflows/client.yml +++ b/.github/workflows/client-test.yml @@ -1,21 +1,15 @@ -name: Appsmith Client Workflow +name: Appsmith Client Test Workflow on: # This line enables manual triggering of this workflow. workflow_dispatch: - push: - branches: [release, master] - # Only trigger if files have changed in this specific path - paths: - - 'app/client/**' - - '!app/client/cypress/manual_TestSuite/**' - - pull_request: + pull_request_review: + types: [submitted] branches: [release, master] paths: - - 'app/client/**' - - '!app/client/cypress/manual_TestSuite/**' + - "app/client/**" + - "!app/client/cypress/manual_TestSuite/**" # Change the working directory for all the jobs in this workflow defaults: @@ -24,6 +18,7 @@ defaults: jobs: build: + if: github.event.review.state == 'approved' runs-on: ubuntu-latest defaults: run: @@ -33,7 +28,7 @@ jobs: steps: # Checkout the code - name: Checkout the merged commit from PR and base branch - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request_review' uses: actions/checkout@v2 with: fetch-depth: 0 @@ -144,7 +139,7 @@ jobs: steps: # Checkout the code - name: Checkout the merged commit from PR and base branch - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request_review' uses: actions/checkout@v2 with: ref: refs/pull/${{ github.event.pull_request.number }}/merge @@ -289,7 +284,7 @@ jobs: steps: # Checkout the code - name: Checkout the merged commit from PR and base branch - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request_review' uses: actions/checkout@v2 with: ref: refs/pull/${{ github.event.pull_request.number }}/merge diff --git a/app/client/README.md b/app/client/README.md index 0ae785bd51..2a275ea504 100755 --- a/app/client/README.md +++ b/app/client/README.md @@ -7,4 +7,3 @@ This project was bootstrapped with [Create React App](https://github.com/facebook/create-react-app). For details on setting up your development machine, please refer to the [Setup Guide](https://github.com/appsmithorg/appsmith/blob/release/contributions/ClientSetup.md) - From 044a0c40f6e288c160c1c78af125aae6d1dc64bf Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Thu, 25 Mar 2021 12:36:56 +0530 Subject: [PATCH 2/6] Stringifying the binding name before setting in the error to ensure client can parse it correctly (#3689) --- .../com/appsmith/server/exceptions/AppsmithError.java | 2 +- .../server/services/LayoutActionServiceImpl.java | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index 5e46dc1d5f..f382d260ac 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -38,7 +38,7 @@ public enum AppsmithError { " \"widgetId\" : \"{2}\"," + " \"pageId\" : \"{4}\"," + " \"layoutId\" : \"{5}\"," + - " \"dynamicBinding\" : \"{6}\"", + " \"dynamicBinding\" : {6}", AppsmithErrorAction.LOG_EXTERNALLY), USER_ALREADY_EXISTS_IN_ORGANIZATION(400, 4021, "The user {0} has already been added to the organization with role {1}. To change the role, please navigate to `Manage Users` page.", AppsmithErrorAction.DEFAULT), UNAUTHORIZED_DOMAIN(401, 4019, "Invalid email domain {0} used for sign in/sign up. Please contact the administrator to configure this domain if this is unexpected.", AppsmithErrorAction.DEFAULT), 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 f01231fc4a..bff3d09ed5 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 @@ -34,7 +34,6 @@ import reactor.core.publisher.Mono; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -358,8 +357,13 @@ public class LayoutActionServiceImpl implements LayoutActionService { // We found the path. But if the path does not have any mustache bindings, throw the error if (mustacheKeysFromFields.isEmpty()) { - throw new AppsmithException(AppsmithError.INVALID_DYNAMIC_BINDING_REFERENCE, widgetType, - widgetName, widgetId, fieldPath, pageId, layoutId, parent); + try { + String bindingAsString = objectMapper.writeValueAsString(parent); + throw new AppsmithException(AppsmithError.INVALID_DYNAMIC_BINDING_REFERENCE, widgetType, + widgetName, widgetId, fieldPath, pageId, layoutId, bindingAsString); + } catch (JsonProcessingException e) { + throw new AppsmithException(AppsmithError.JSON_PROCESSING_ERROR, parent); + } } dynamicBindings.addAll(mustacheKeysFromFields); From 25d7fe23699f17ead13c6c60673a82f1871372f7 Mon Sep 17 00:00:00 2001 From: Hetu Nandu Date: Thu, 25 Mar 2021 13:08:45 +0530 Subject: [PATCH 3/6] Run cypress tests on push to release and master (#3697) Co-authored-by: Arpit Mohan --- .github/workflows/client-test.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/client-test.yml b/.github/workflows/client-test.yml index a3d6282675..0c99a20d11 100644 --- a/.github/workflows/client-test.yml +++ b/.github/workflows/client-test.yml @@ -11,6 +11,10 @@ on: - "app/client/**" - "!app/client/cypress/manual_TestSuite/**" + # trigger for pushes to release and master + push: + branches: [ release, master ] + # Change the working directory for all the jobs in this workflow defaults: run: @@ -18,7 +22,12 @@ defaults: jobs: build: - if: github.event.review.state == 'approved' + # If the build has been triggered manually via workflow_dispatch or via a push to protected branches + # then we don't check for the PR approved state + if: | + github.event_name == 'workflow_dispatch' || + github.event_name == 'push' || + (github.event_name == 'pull_request_review' && github.event.review.state == 'approved') runs-on: ubuntu-latest defaults: run: From e9f80f33127a0da2a4c31314c40070d0dc562ed6 Mon Sep 17 00:00:00 2001 From: Arpit Mohan Date: Thu, 25 Mar 2021 13:10:47 +0530 Subject: [PATCH 4/6] Adding the path limitations to run the client test only when client code changes --- .github/workflows/client-test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/client-test.yml b/.github/workflows/client-test.yml index 0c99a20d11..0addaed4df 100644 --- a/.github/workflows/client-test.yml +++ b/.github/workflows/client-test.yml @@ -14,6 +14,9 @@ on: # trigger for pushes to release and master push: branches: [ release, master ] + paths: + - "app/client/**" + - "!app/client/cypress/manual_TestSuite/**" # Change the working directory for all the jobs in this workflow defaults: From bf6cdbe30509dfc572f0e7dcdf8b81af1cf95894 Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Thu, 25 Mar 2021 16:00:45 +0530 Subject: [PATCH 5/6] [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(); From da2fb11847ac5555792e1ef82cf9948df23bbb30 Mon Sep 17 00:00:00 2001 From: Hetu Nandu Date: Mon, 29 Mar 2021 12:08:28 +0530 Subject: [PATCH 6/6] Create coverage-summary.json --- coverage-summary.json | 1 + 1 file changed, 1 insertion(+) create mode 100644 coverage-summary.json diff --git a/coverage-summary.json b/coverage-summary.json new file mode 100644 index 0000000000..0967ef424b --- /dev/null +++ b/coverage-summary.json @@ -0,0 +1 @@ +{}