From b7b5770a089efb1d55c717b2c965fb74a0d61257 Mon Sep 17 00:00:00 2001 From: sneha122 Date: Thu, 24 Oct 2024 11:08:28 +0530 Subject: [PATCH 1/2] fix: google sheets query getting executed even after changing sheet (#37006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR fixes the issue with google sheets -> selected sheets option. Following are the steps to reproduce the issue on cloud/release platform: **Steps:** 1. Create a Google sheets datasource with selected sheets option 2. Select "spreadsheet1" from the file picker. 3. Create a query for this datasource and attach it to table widget 4. Now edit the datasource and change authorisation from "spreadsheet1" to "spreadsheet2" 5. Refresh the page and check the table data This issue has been fixed, where we check if the correct spreadsheetId is present in authorisedSheetIds, if it's not throwing an exception so we don't execute the query. Fixes #36747 _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.Sanity" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: e0f725a6246ff849c8666d5a34e6cf016111cf0f > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Wed, 23 Oct 2024 05:43:59 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Introduced a new error message for missing spreadsheet URLs to enhance user guidance. - Updated method for retrieving authorized sheet IDs to include validation, improving error handling. - **Bug Fixes** - Enhanced error handling for cases where the spreadsheet URL is invalid or missing. - **Tests** - Added new test cases to validate the updated functionality for authorized sheet IDs, ensuring robust error handling and correct returns. --------- Co-authored-by: β€œsneha122” <β€œsneha@appsmith.com”> --- .../com/external/constants/ErrorMessages.java | 3 + .../external/plugins/GoogleSheetsPlugin.java | 8 +- .../java/com/external/utils/SheetsUtil.java | 26 ++++- .../com/external/config/SheetsUtilTest.java | 106 ++++++++++++++++-- 4 files changed, 127 insertions(+), 16 deletions(-) diff --git a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java index 466b205d2e0..53937354c81 100644 --- a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java +++ b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java @@ -39,6 +39,9 @@ public class ErrorMessages extends BasePluginErrorMessages { public static final String MISSING_ROW_INDEX_ERROR_MSG = "Missing required field 'Row index'"; + public static final String MISSING_SPREADSHEET_URL_SELECTED_SHEETS_ERROR_MSG = + "Missing required field 'Spreadsheet Url'. Please check if your datasource is authorized to use this spreadsheet."; + public static final String UNABLE_TO_CREATE_URI_ERROR_MSG = "Unable to create URI"; public static final String MISSING_VALID_RESPONSE_ERROR_MSG = "Missing a valid response object."; diff --git a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java index e99f97a9fe5..f336d54b8fc 100644 --- a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java +++ b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java @@ -51,7 +51,7 @@ import static com.appsmith.external.helpers.PluginUtils.getDataValueSafelyFromFormData; import static com.appsmith.external.helpers.PluginUtils.setDataValueSafelyInFormData; import static com.appsmith.external.helpers.PluginUtils.validConfigurationPresentInFormData; -import static com.external.utils.SheetsUtil.getUserAuthorizedSheetIds; +import static com.external.utils.SheetsUtil.validateAndGetUserAuthorizedSheetIds; import static java.lang.Boolean.TRUE; @Slf4j @@ -175,7 +175,8 @@ public Mono executeCommon( // This will get list of authorised sheet ids from datasource config, and transform execution response to // contain only authorised files - final Set userAuthorizedSheetIds = getUserAuthorizedSheetIds(datasourceConfiguration); + final Set userAuthorizedSheetIds = + validateAndGetUserAuthorizedSheetIds(datasourceConfiguration, methodConfig); // Triggering the actual REST API call return executionMethod @@ -338,7 +339,8 @@ public Mono trigger( // This will get list of authorised sheet ids from datasource config, and transform trigger response to // contain only authorised files - Set userAuthorizedSheetIds = getUserAuthorizedSheetIds(datasourceConfiguration); + Set userAuthorizedSheetIds = + validateAndGetUserAuthorizedSheetIds(datasourceConfiguration, methodConfig); return triggerMethod .getTriggerClient(client, methodConfig) diff --git a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java index 90b1e17698d..ee8506e3dfe 100644 --- a/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java +++ b/app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java @@ -1,7 +1,11 @@ package com.external.utils; +import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError; +import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.OAuth2; +import com.external.config.MethodConfig; +import com.external.constants.ErrorMessages; import com.external.enums.GoogleSheetMethodEnum; import com.fasterxml.jackson.databind.JsonNode; @@ -17,8 +21,10 @@ public class SheetsUtil { private static final String FILE_SPECIFIC_DRIVE_SCOPE = "https://www.googleapis.com/auth/drive.file"; private static final int USER_AUTHORIZED_SHEET_IDS_INDEX = 1; - public static Set getUserAuthorizedSheetIds(DatasourceConfiguration datasourceConfiguration) { + public static Set validateAndGetUserAuthorizedSheetIds( + DatasourceConfiguration datasourceConfiguration, MethodConfig methodConfig) { OAuth2 oAuth2 = (OAuth2) datasourceConfiguration.getAuthentication(); + Set userAuthorisedSheetIds = null; if (!isEmpty(datasourceConfiguration.getProperties()) && datasourceConfiguration.getProperties().size() > 1 && datasourceConfiguration.getProperties().get(USER_AUTHORIZED_SHEET_IDS_INDEX) != null @@ -33,9 +39,23 @@ public static Set getUserAuthorizedSheetIds(DatasourceConfiguration data .getProperties() .get(USER_AUTHORIZED_SHEET_IDS_INDEX) .getValue(); - return new HashSet(temp); + userAuthorisedSheetIds = new HashSet(temp); + + // This is added specifically for selected gsheets, so that whenever authorisation changes from one sheet to + // another + // We throw an error, this is done because when we use drive.file scope which is for selected sheets through + // file picker + // The access token for this scope grants access to all selected sheets across datasources + // we want to constraint the access for datasource to the sheet which was selected during ds authorisation + if (methodConfig != null + && methodConfig.getSpreadsheetId() != null + && !userAuthorisedSheetIds.contains(methodConfig.getSpreadsheetId())) { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + ErrorMessages.MISSING_SPREADSHEET_URL_SELECTED_SHEETS_ERROR_MSG); + } } - return null; + return userAuthorisedSheetIds; } public static Map getSpreadsheetData( diff --git a/app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java b/app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java index 1b5d7856880..c99f3f2ecd6 100644 --- a/app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java +++ b/app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java @@ -1,22 +1,22 @@ package com.external.config; +import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.OAuth2; import com.appsmith.external.models.Property; +import com.external.constants.ErrorMessages; import com.external.utils.SheetsUtil; import com.fasterxml.jackson.core.JsonProcessingException; import org.junit.jupiter.api.Test; -import java.util.ArrayList; -import java.util.List; -import java.util.Set; +import java.util.*; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.*; public class SheetsUtilTest { @Test - public void testGetUserAuthorizedSheetIds_allsheets_returnsNull() throws JsonProcessingException { + public void testValidateAndGetUserAuthorizedSheetIds_allsheets_returnsNull() throws JsonProcessingException { DatasourceConfiguration dsConfig = new DatasourceConfiguration(); List propList = new ArrayList(); Property prop = new Property(); @@ -24,12 +24,13 @@ public void testGetUserAuthorizedSheetIds_allsheets_returnsNull() throws JsonPro prop.setValue("test_email"); propList.add(prop); dsConfig.setProperties(propList); - Set result = SheetsUtil.getUserAuthorizedSheetIds(dsConfig); - assertEquals(result, null); + Set result = SheetsUtil.validateAndGetUserAuthorizedSheetIds(dsConfig, null); + assertEquals(null, result); } @Test - public void testGetUserAuthorizedSheetIds_specificSheets_returnsSetOfFileIds() throws JsonProcessingException { + public void testValidateAndGetUserAuthorizedSheetIds_specificSheets_returnsSetOfFileIds() + throws JsonProcessingException { DatasourceConfiguration dsConfig = new DatasourceConfiguration(); List propList = new ArrayList(); OAuth2 oAuth2 = new OAuth2(); @@ -47,7 +48,92 @@ public void testGetUserAuthorizedSheetIds_specificSheets_returnsSetOfFileIds() t propList.add(prop2); dsConfig.setProperties(propList); - Set result = SheetsUtil.getUserAuthorizedSheetIds(dsConfig); - assertEquals(result.size(), 1); + Set result = SheetsUtil.validateAndGetUserAuthorizedSheetIds(dsConfig, null); + assertEquals(1, result.size()); + } + + @Test + public void testValidateAndGetUserAuthorizedSheetIds_invalidSpecificSheets_throwsException() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + List propList = new ArrayList<>(); + OAuth2 oAuth2 = new OAuth2(); + + oAuth2.setScopeString("https://www.googleapis.com/auth/drive.file"); + dsConfig.setAuthentication(oAuth2); + + Property prop1 = new Property("emailAddress", "test_email"); + propList.add(prop1); + + List ids = new ArrayList<>(); + ids.add("id1"); + + Property prop2 = new Property("userAuthorizedSheetIds", ids); + propList.add(prop2); + + dsConfig.setProperties(propList); + + // Create formData map with only the spreadsheetUrl + Map formData = new HashMap<>(); + Map spreadsheetUrlData = new HashMap<>(); + spreadsheetUrlData.put("data", "https://docs.google.com/spreadsheets/d/id2"); + formData.put("sheetUrl", spreadsheetUrlData); + + MethodConfig methodConfig = new MethodConfig(formData); + + AppsmithPluginException exception = assertThrows(AppsmithPluginException.class, () -> { + SheetsUtil.validateAndGetUserAuthorizedSheetIds(dsConfig, methodConfig); + }); + + String expectedErrorMessage = ErrorMessages.MISSING_SPREADSHEET_URL_SELECTED_SHEETS_ERROR_MSG; + assertEquals(expectedErrorMessage, exception.getMessage()); + } + + @Test + public void testValidateAndGetUserAuthorizedSheetIds_validSpecificSheets_returnsAuthorisedSheetIds() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + List propList = new ArrayList<>(); + OAuth2 oAuth2 = new OAuth2(); + + oAuth2.setScopeString("https://www.googleapis.com/auth/drive.file"); + dsConfig.setAuthentication(oAuth2); + + Property prop1 = new Property("emailAddress", "test_email"); + propList.add(prop1); + + List ids = new ArrayList<>(); + ids.add("id1"); + + Property prop2 = new Property("userAuthorizedSheetIds", ids); + propList.add(prop2); + + dsConfig.setProperties(propList); + + // Create formData map with the spreadsheetUrl + Map formData = new HashMap<>(); + Map spreadsheetUrlData = new HashMap<>(); + spreadsheetUrlData.put("data", "https://docs.google.com/spreadsheets/d/id1"); + formData.put("sheetUrl", spreadsheetUrlData); + + MethodConfig methodConfig = new MethodConfig(formData); + + Set result = SheetsUtil.validateAndGetUserAuthorizedSheetIds(dsConfig, methodConfig); + + assertEquals(1, result.size()); + assertTrue(result.contains("id1")); + } + + @Test + public void testValidateAndGetUserAuthorizedSheetIds_allSheets_returnsNull() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + OAuth2 oAuth2 = new OAuth2(); + + oAuth2.setScopeString("https://www.googleapis.com/auth/drive,https://www.googleapis.com/auth/spreadsheets"); + dsConfig.setAuthentication(oAuth2); + + // Not adding any properties to dsConfig + + Set result = SheetsUtil.validateAndGetUserAuthorizedSheetIds(dsConfig, null); + + assertNull(result); } } From 5d571b918596a17e3c4f54448de193fd3d87b2e7 Mon Sep 17 00:00:00 2001 From: Rishabh Rathod Date: Thu, 24 Oct 2024 11:27:41 +0530 Subject: [PATCH 2/2] fix: resolve redundant JSObject action saving (#36958) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description In this change, we avoid storing duplicate actions or variables in the `parsedBody` after AST parsing. This prevents duplicate actions from being sent to the server on save. This change also removes `parsedFunction` from the action object of parsedBody as it is not being used. Fixes https://github.com/appsmithorg/appsmith/issues/36879 ## Test Cases - [x] After parsing JS Object make sure parsedBody doesn't have duplicate actions or variables ## Automation /ok-to-test tags="@tag.JS" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: c12ee7007d8f860aa3bbf2cc1889bd4b762785a6 > Cypress dashboard. > Tags: `@tag.JS` > Spec: >
Wed, 23 Oct 2024 18:20:11 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Introduced enhanced methods for creating, updating, and moving action collections, improving validation and consistency. - Added a new interface for better handling of JavaScript actions and variables, enhancing data processing efficiency. - **Bug Fixes** - Improved error reporting from pages to collections, ensuring accurate tracking of action errors. - Added tests to ensure duplicate actions are correctly handled during updates. - **Documentation** - Updated comments and documentation for clarity on new functionalities and changes. --- .../src/workers/Evaluation/JSObject/index.ts | 26 ++-- .../src/workers/Evaluation/JSObject/test.ts | 127 +++++++++++++++++- 2 files changed, 137 insertions(+), 16 deletions(-) diff --git a/app/client/src/workers/Evaluation/JSObject/index.ts b/app/client/src/workers/Evaluation/JSObject/index.ts index 4c9c6b35f78..79dce10b39b 100644 --- a/app/client/src/workers/Evaluation/JSObject/index.ts +++ b/app/client/src/workers/Evaluation/JSObject/index.ts @@ -114,18 +114,17 @@ export function saveResolvedFunctionsAndJSUpdates( JSObjectName: entityName, JSObjectASTParseTime, }); - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const actions: any = []; - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const variables: any = []; + + const actionsMap: Record = {}; + const variablesMap: Record = {}; if (success) { if (!!parsedObject) { jsPropertiesState.update(entityName, parsedObject); parsedObject.forEach((parsedElement) => { if (isJSFunctionProperty(parsedElement)) { + if (actionsMap[parsedElement.key]) return; + try { ExecutionMetaData.setExecutionMetaData({ enableJSVarUpdateTracking: false, @@ -164,12 +163,11 @@ export function saveResolvedFunctionsAndJSUpdates( `${entityName}.${parsedElement.key}`, functionString, ); - actions.push({ + actionsMap[parsedElement.key] = { name: parsedElement.key, body: functionString, arguments: params, - parsedFunction: result, - }); + }; } } catch { // in case we need to handle error state @@ -184,10 +182,10 @@ export function saveResolvedFunctionsAndJSUpdates( ? parsedElement.key.slice(1, -1) : parsedElement.key; - variables.push({ + variablesMap[parsedKey] = { name: parsedKey, value: parsedElement.value, - }); + }; JSObjectCollection.updateUnEvalState( `${entityName}.${parsedElement.key}`, parsedElement.value, @@ -196,8 +194,8 @@ export function saveResolvedFunctionsAndJSUpdates( }); const parsedBody = { body: entity.body, - actions: actions, - variables, + actions: Object.values(actionsMap), + variables: Object.values(variablesMap), }; set(jsUpdates, `${entityName}`, { @@ -312,8 +310,6 @@ export function parseJSActions( parsedBody.actions = parsedBody.actions.map((action) => { return { ...action, - // parsedFunction - used only to determine if function is async - parsedFunction: undefined, } as ParsedJSSubAction; }); }); diff --git a/app/client/src/workers/Evaluation/JSObject/test.ts b/app/client/src/workers/Evaluation/JSObject/test.ts index 3a4cebe482d..ccfb1c6c969 100644 --- a/app/client/src/workers/Evaluation/JSObject/test.ts +++ b/app/client/src/workers/Evaluation/JSObject/test.ts @@ -1,5 +1,14 @@ import type { ConfigTree, UnEvalTree } from "entities/DataTree/dataTreeTypes"; -import { getUpdatedLocalUnEvalTreeAfterJSUpdates } from "."; +import { + getUpdatedLocalUnEvalTreeAfterJSUpdates, + saveResolvedFunctionsAndJSUpdates, +} from "."; +import type { JSUpdate } from "utils/JSPaneUtils"; +import type { + JSActionEntity, + JSActionEntityConfig, +} from "ee/entities/DataTree/types"; +import DataTreeEvaluator from "workers/common/DataTreeEvaluator"; describe("updateJSCollectionInUnEvalTree", function () { it("updates async value of jsAction", () => { @@ -137,3 +146,119 @@ describe("updateJSCollectionInUnEvalTree", function () { expect(expectedResult).toStrictEqual(actualResult); }); }); + +describe("saveResolvedFunctionsAndJSUpdates", function () { + it("parses JSObject with duplicate actions, variables and updates jsUpdates correctly", () => { + const dataTreeEvalRef = new DataTreeEvaluator({}); + const entity: JSActionEntity = { + actionId: "64013546b956c26882acc587", + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + ENTITY_TYPE: "JSACTION", + name: "JSObject1", + pluginType: "JS", + }; + const jsUpdates: Record = {}; + const unEvalDataTree: UnEvalTree = { + JSObject1: { + myVar1: "[]", + myVar2: "{}", + myFun1: new String("() => {}"), + myFun2: new String("async () => {\n yeso;\n}"), + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + ENTITY_TYPE: "JSACTION", + meta: { + myFun1: { + arguments: [], + confirmBeforeExecute: false, + }, + myFun2: { + arguments: [], + confirmBeforeExecute: false, + }, + }, + dependencyMap: { + body: ["myFun1", "myFun2"], + }, + dynamicBindingPathList: [ + { + key: "body", + }, + { + key: "myVar1", + }, + { + key: "myVar2", + }, + { + key: "myFun1", + }, + { + key: "myFun2", + }, + { + key: "myFun2", + }, + ], + bindingPaths: { + body: "SMART_SUBSTITUTE", + myVar1: "SMART_SUBSTITUTE", + myVar2: "SMART_SUBSTITUTE", + myFun1: "SMART_SUBSTITUTE", + myFun2: "SMART_SUBSTITUTE", + }, + reactivePaths: { + body: "SMART_SUBSTITUTE", + myVar1: "SMART_SUBSTITUTE", + myVar2: "SMART_SUBSTITUTE", + myFun1: "SMART_SUBSTITUTE", + myFun2: "SMART_SUBSTITUTE", + }, + pluginType: "JS", + name: "JSObject1", + actionId: "64013546b956c26882acc587", + } as JSActionEntityConfig, + }; + const entityName = "JSObject1"; + + const result = saveResolvedFunctionsAndJSUpdates( + dataTreeEvalRef, + entity, + jsUpdates, + unEvalDataTree, + entityName, + ); + + const expectedJSUpdates = { + JSObject1: { + parsedBody: { + body: "export default {\n\tmyVar1: [],\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1: () => {\n\t\t//write code here\n\t\t\n\t},\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}\n,\n\tmyFun2: () => {\n\t\t//use async-await or promises\n\t\tyeso\n\t}}", + actions: [ + { + name: "myFun1", + body: "() => {}", + arguments: [], + }, + { + name: "myFun2", + body: "() => {\n yeso;\n}", + arguments: [], + }, + ], + variables: [ + { + name: "myVar1", + value: "[]", + }, + { + name: "myVar2", + value: "{}", + }, + ], + }, + id: "64013546b956c26882acc587", + }, + }; + + expect(result).toStrictEqual(expectedJSUpdates); + }); +});