From 9e6d28f6d1cefd45eb900d5a4704c4bec42ef25d Mon Sep 17 00:00:00 2001 From: Sumit Kumar Date: Tue, 13 Jun 2023 18:32:19 +0530 Subject: [PATCH] fix: fix execution of REST API query when dynamic binding is present in the URL (#24368) ## Description - fix execution of REST API query when dynamic binding is present in the URL Fixes #24218 (cherry picked from commit 9330829e4004d44c13ca916cb633959d544886b2) --- ...TestExecuteWithDynamicBindingInUrl_spec.ts | 42 +++++++++++++++++++ .../pluginExceptions/AppsmithPluginError.java | 10 +++++ .../AppsmithPluginErrorCode.java | 3 +- .../restApiUtils/helpers/DatasourceUtils.java | 17 +++++++- .../restApiUtils/helpers/InitUtils.java | 3 +- .../external/models/DatasourceStorage.java | 4 ++ .../plugins/BaseRestApiPluginExecutor.java | 19 +++++++-- .../external/plugins/PluginExecutor.java | 26 +++++++----- .../external/plugins/DatasourceUtilsTest.java | 27 ++++++++++++ .../ce/DatasourceStorageServiceCEImpl.java | 6 ++- 10 files changed, 137 insertions(+), 20 deletions(-) create mode 100644 app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_TestExecuteWithDynamicBindingInUrl_spec.ts create mode 100644 app/server/appsmith-interfaces/src/test/java/com/appsmith/external/plugins/DatasourceUtilsTest.java diff --git a/app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_TestExecuteWithDynamicBindingInUrl_spec.ts b/app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_TestExecuteWithDynamicBindingInUrl_spec.ts new file mode 100644 index 0000000000..831fbb3ef7 --- /dev/null +++ b/app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_TestExecuteWithDynamicBindingInUrl_spec.ts @@ -0,0 +1,42 @@ +const datasourceFormData = require("../../../../fixtures/datasources.json"); + +import { + apiPage, + agHelper, + jsEditor, +} from "../../../../support/Objects/ObjectsCore"; + +describe("Test API execution with dynamic binding in URL - Bug #24218", () => { + it("1. Test API execution with dynamic binding in URL", () => { + // Create JS Object to set Appsmith store variable to mockApiUrl + jsEditor.CreateJSObject( + `export default { + myVar1: [], + myVar2: {}, + myFun1 () { + storeValue("api_url", "${datasourceFormData["mockApiUrl"]}"); + }, + myFun2: async function() { + } + }`, + { + paste: true, + completeReplace: true, + toRun: false, + shouldCreateNewJSObj: true, + prettify: true, + }, + ); + + jsEditor.RunJSObj(); + + // Create API and set URL to {{appsmith.store.api_url}} + apiPage.CreateAndFillApi( + "{{appsmith.store.api_url}}", + "Api_with_dynamic_binding", + ); + apiPage.RunAPI(); + apiPage.ResponseStatusCheck("200 OK"); + agHelper.ActionContextMenuWithInPane("Delete"); + }); +}); diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginError.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginError.java index 2e96dac78a..f1ab6992da 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginError.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginError.java @@ -29,6 +29,16 @@ public enum AppsmithPluginError implements BasePluginError{ "{1}", "{2}" ), + PLUGIN_VALIDATE_DATASOURCE_ERROR( + 500, + AppsmithPluginErrorCode.PLUGIN_VALIDATE_DATASOURCE_ERROR.getCode(), + "{0}", + AppsmithErrorAction.DEFAULT, + AppsmithPluginErrorCode.PLUGIN_VALIDATE_DATASOURCE_ERROR.getDescription(), + ErrorType.INTERNAL_ERROR, + "{1}", + "{2}" + ), PLUGIN_QUERY_TIMEOUT_ERROR( 504, AppsmithPluginErrorCode.PLUGIN_QUERY_TIMEOUT_ERROR.getCode(), diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginErrorCode.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginErrorCode.java index a5bfeedf16..2d13db7e1a 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginErrorCode.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginErrorCode.java @@ -19,7 +19,8 @@ public enum AppsmithPluginErrorCode { PLUGIN_AUTHENTICATION_ERROR("PE-ATH-5000", "Datasource authentication error"), PLUGIN_UQI_WHERE_CONDITION_UNKNOWN("PE-UQI-5000", "Where condition could not be parsed"), GENERIC_STALE_CONNECTION("PE-STC-5000", "Secondary stale connection error"), - PLUGIN_EXECUTE_ARGUMENT_ERROR("PE-ARG-5000", "Wrong arguments provided") + PLUGIN_EXECUTE_ARGUMENT_ERROR("PE-ARG-5000", "Wrong arguments provided"), + PLUGIN_VALIDATE_DATASOURCE_ERROR("PE-DSE-5005", "Failed to validate datasource") ; diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DatasourceUtils.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DatasourceUtils.java index 8d7fffb0f1..cd10eb48c7 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DatasourceUtils.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DatasourceUtils.java @@ -15,7 +15,8 @@ public class DatasourceUtils { protected static HeaderUtils headerUtils = new HeaderUtils(); - public Set validateDatasource(DatasourceConfiguration datasourceConfiguration) { + public Set validateDatasource(DatasourceConfiguration datasourceConfiguration, + boolean isEmbeddedDatasource) { /** * We don't verify whether the URL is in valid format because it can contain mustache template keys, and so * look invalid at this point, but become valid after mustache rendering. So we just check if URL field has @@ -25,7 +26,19 @@ public class DatasourceUtils { Set invalids = new HashSet<>(); if (StringUtils.isEmpty(datasourceConfiguration.getUrl())) { - invalids.add("Missing URL."); + if (isEmbeddedDatasource) { + /** + * Deliberately skipping adding any invalidity message here because based on the current parsing logic the + * client can skip adding a URL to embedded datasource and instead add the entire URL to + * `actionConfiguration.path`. + * ref: https://theappsmith.slack.com/archives/C040LHZN03V/p1686478370473659?thread_ts=1686300736 + * .679729&cid=C040LHZN03V + */ + } + else { + invalids.add("Missing URL."); + } + } final String contentTypeError = headerUtils.verifyContentType(datasourceConfiguration.getHeaders()); diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/InitUtils.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/InitUtils.java index 491c6d8f08..d15fd1c68e 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/InitUtils.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/InitUtils.java @@ -4,7 +4,6 @@ import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionResult; import com.appsmith.external.models.DatasourceConfiguration; -import lombok.AccessLevel; import lombok.NoArgsConstructor; @NoArgsConstructor @@ -13,7 +12,7 @@ public class InitUtils { public String initializeRequestUrl(ActionConfiguration actionConfiguration, DatasourceConfiguration datasourceConfiguration ) { String path = (actionConfiguration.getPath() == null) ? "" : actionConfiguration.getPath(); - return datasourceConfiguration.getUrl().trim() + path; + return datasourceConfiguration.getUrl().trim() + path.trim(); } public void initializeResponseWithError(ActionExecutionResult result) { diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java index 05f9678bfe..55cb60a9ff 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java @@ -164,4 +164,8 @@ public class DatasourceStorage extends BaseDomain { this.setPluginId(pluginMap.get(this.getPluginId())); this.setIsRecentlyCreated(null); } + + public boolean isEmbedded() { + return this.getDatasourceId() == null; + } } diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BaseRestApiPluginExecutor.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BaseRestApiPluginExecutor.java index e81aad7e4f..8b1659692a 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BaseRestApiPluginExecutor.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BaseRestApiPluginExecutor.java @@ -9,8 +9,8 @@ import com.appsmith.external.helpers.restApiUtils.helpers.DatasourceUtils; import com.appsmith.external.helpers.restApiUtils.helpers.HeaderUtils; import com.appsmith.external.helpers.restApiUtils.helpers.HintMessageUtils; import com.appsmith.external.helpers.restApiUtils.helpers.InitUtils; -import com.appsmith.external.helpers.restApiUtils.helpers.SmartSubstitutionUtils; import com.appsmith.external.helpers.restApiUtils.helpers.RestAPIActivateUtils; +import com.appsmith.external.helpers.restApiUtils.helpers.SmartSubstitutionUtils; import com.appsmith.external.helpers.restApiUtils.helpers.URIUtils; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionResult; @@ -73,9 +73,22 @@ public class BaseRestApiPluginExecutor implements PluginExecutor, } @Override - public Set validateDatasource(DatasourceConfiguration datasourceConfiguration) { + public Set validateDatasource(DatasourceConfiguration datasourceConfiguration, + boolean isEmbeddedDatasource) { /* Use the default validation routine for REST API based plugins */ - return datasourceUtils.validateDatasource(datasourceConfiguration); + return datasourceUtils.validateDatasource(datasourceConfiguration, isEmbeddedDatasource); + } + + @Override + public Set validateDatasource(DatasourceConfiguration datasourceConfiguration) { + /** + * This method is not expected to be used as the REST API based plugins may have embedded datasource. Instead + * the following variant should be used: + * validateDatasource(DatasourceConfiguration datasourceConfiguration, boolean isEmbeddedDatasource) + */ + throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_VALIDATE_DATASOURCE_ERROR, "This method should " + + "not be used for this plugin as it may have embedded datasource. Please use validateDatasource" + + "(dsConfig, isEmbedded)."); } @Override diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java index 7a1fb1f448..b943ee15bd 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java @@ -10,27 +10,25 @@ import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.DatasourceStructure; import com.appsmith.external.models.DatasourceTestResult; import com.appsmith.external.models.Param; -import com.appsmith.external.models.Property; import com.appsmith.external.models.TriggerRequestDTO; import com.appsmith.external.models.TriggerResultDTO; import io.micrometer.observation.ObservationRegistry; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Properties; -import java.util.Set; import org.pf4j.ExtensionPoint; -import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import reactor.core.observability.micrometer.Micrometer; import reactor.core.publisher.Mono; import reactor.core.scheduler.Schedulers; import reactor.util.function.Tuple2; +import java.util.HashSet; +import java.util.Map; +import java.util.Properties; +import java.util.Set; import java.util.stream.Collectors; import static com.appsmith.external.constants.spans.ActionSpans.ACTION_EXECUTION_PLUGIN_EXECUTION; import static com.appsmith.external.helpers.PluginUtils.getHintMessageForLocalhostUrl; +import static org.springframework.util.CollectionUtils.isEmpty; public interface PluginExecutor extends ExtensionPoint, CrudTemplateService { @@ -96,8 +94,8 @@ public interface PluginExecutor extends ExtensionPoint, CrudTemplateService { * @param datasourceConfiguration * @return boolean */ - default boolean isDatasourceValid(DatasourceConfiguration datasourceConfiguration) { - return CollectionUtils.isEmpty(validateDatasource(datasourceConfiguration)); + default boolean isDatasourceValid(DatasourceConfiguration datasourceConfiguration, boolean isEmbeddedDatasource) { + return isEmpty(validateDatasource(datasourceConfiguration, isEmbeddedDatasource)); } /** @@ -112,6 +110,14 @@ public interface PluginExecutor extends ExtensionPoint, CrudTemplateService { * @return Set : The set of invalid strings informing the user of all the invalid fields */ Set validateDatasource(DatasourceConfiguration datasourceConfiguration); + default Set validateDatasource(DatasourceConfiguration datasourceConfiguration, + boolean isEmbeddedDatasource) { + if (!isEmbeddedDatasource) { + return this.validateDatasource(datasourceConfiguration); + } + + return Set.of(); + } /** * This function tests the datasource by executing a test query or hitting the endpoint to check the correctness @@ -227,7 +233,7 @@ public interface PluginExecutor extends ExtensionPoint, CrudTemplateService { ExecuteActionDTO executeActionDTO) { //Do variable substitution //Do this only if params have been provided in the execute command - if (executeActionDTO != null && !CollectionUtils.isEmpty(executeActionDTO.getParams())) { + if (executeActionDTO != null && !isEmpty(executeActionDTO.getParams())) { Map replaceParamsMap = executeActionDTO .getParams() .stream() diff --git a/app/server/appsmith-interfaces/src/test/java/com/appsmith/external/plugins/DatasourceUtilsTest.java b/app/server/appsmith-interfaces/src/test/java/com/appsmith/external/plugins/DatasourceUtilsTest.java new file mode 100644 index 0000000000..28da84c5d2 --- /dev/null +++ b/app/server/appsmith-interfaces/src/test/java/com/appsmith/external/plugins/DatasourceUtilsTest.java @@ -0,0 +1,27 @@ +package com.appsmith.external.plugins; + +import com.appsmith.external.helpers.restApiUtils.helpers.DatasourceUtils; +import com.appsmith.external.models.DatasourceConfiguration; +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class DatasourceUtilsTest { + @Test + public void testValidateDatasourceSkipsEmptyUrlCheckIfEmbedded() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + DatasourceUtils datasourceUtils = new DatasourceUtils(); + Set invalids = datasourceUtils.validateDatasource(dsConfig, true); + assertTrue(invalids.isEmpty()); + } + + @Test + public void testValidateDatasourceAddsEmptyUrlInvalidIfNotEmbedded() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + DatasourceUtils datasourceUtils = new DatasourceUtils(); + Set invalids = datasourceUtils.validateDatasource(dsConfig, false); + assertTrue(invalids.contains("Missing URL.")); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceStorageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceStorageServiceCEImpl.java index 54c6fc40b8..7f7a913148 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceStorageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceStorageServiceCEImpl.java @@ -204,8 +204,10 @@ public class DatasourceStorageServiceCEImpl implements DatasourceStorageServiceC return pluginExecutorMono .map(pluginExecutor -> { DatasourceConfiguration datasourceConfiguration = datasourceStorage.getDatasourceConfiguration(); - if (datasourceConfiguration != null && !pluginExecutor.isDatasourceValid(datasourceConfiguration)) { - invalids.addAll(pluginExecutor.validateDatasource(datasourceConfiguration)); + if (datasourceConfiguration != null && !pluginExecutor.isDatasourceValid(datasourceConfiguration, + datasourceStorage.isEmbedded())) { + invalids.addAll(pluginExecutor.validateDatasource(datasourceConfiguration, + datasourceStorage.isEmbedded())); } return datasourceStorage;