diff --git a/app/client/cypress/fixtures/testdata.json b/app/client/cypress/fixtures/testdata.json index 20efa7f84d..0d31926877 100644 --- a/app/client/cypress/fixtures/testdata.json +++ b/app/client/cypress/fixtures/testdata.json @@ -3,7 +3,7 @@ "methods": "users", "headerKey": "Content-Type", "headerValue": "application/json", - "headerValueBlank": "", + "headerValueBlank": " ", "queryKey": "page", "queryValue": "2", "queryAndValue": "users?page=2", @@ -34,5 +34,8 @@ "deleteAction": "//div[contains(@id,'react-select') and contains(text(),'DELETE')]", "moustacheMethod": "{{Api.text}}", "nextUrl": ".data.next}}", - "prevUrl": ".data.previous}}" + "prevUrl": ".data.previous}}", + "methodsWithParam": "users?page=2", + "invalidHeader": "invalid", + "invalidValue": "invalid" } diff --git a/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_All_Verb_spec.js b/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_All_Verb_spec.js index 31e34d6ddc..d755ed2d9e 100644 --- a/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_All_Verb_spec.js +++ b/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_All_Verb_spec.js @@ -106,15 +106,10 @@ describe("API Panel Test Functionality", function() { }); it("Test GET Action for mock API with header and pagination", function() { - const apiname = "FirstAPI"; + const apiname = "SecondAPI"; cy.CreateAPI(apiname); cy.log("Creation of API Action successful"); - cy.EnterSourceDetailsWithHeader( - testdata.baseUrl, - testdata.methods, - testdata.headerValueBlank, - testdata.headerValueBlank, - ); + cy.enterDatasourceAndPath(testdata.baseUrl, testdata.methods); cy.RunAPI(); cy.ResponseStatusCheck(testdata.successStatusCode); cy.log("Response code check successful"); @@ -137,16 +132,9 @@ describe("API Panel Test Functionality", function() { }); it("API check with query params test API fetaure", function() { - cy.CreateAPI("FirstAPI"); - cy.log("Creation of FirstAPI Action successful"); - cy.EnterSourceDetailsWithQueryParam( - testdata.baseUrl, - testdata.methods, - testdata.headerValueBlank, - testdata.headerValueBlank, - testdata.queryKey, - testdata.queryValue, - ); + cy.CreateAPI("ThirdAPI"); + cy.log("Creation of API Action successful"); + cy.enterDatasourceAndPath(testdata.baseUrl, testdata.queryAndValue); cy.RunAPI(); cy.ResponseStatusCheck("200 OK"); cy.log("Response code check successful"); @@ -155,13 +143,13 @@ describe("API Panel Test Functionality", function() { }); it("API check with Invalid Header", function() { - cy.CreateAPI("FirstAPI"); - cy.log("Creation of SecondAPI Action successful"); + cy.CreateAPI("FourthAPI"); + cy.log("Creation of API Action successful"); cy.EnterSourceDetailsWithQueryParam( testdata.baseUrl, testdata.methods, testdata.headerKey, - testdata.headerValueBlank, + testdata.invalidValue, testdata.queryKey, testdata.queryValue, ); diff --git a/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_Edit_spec.js b/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_Edit_spec.js index 4592be3e3f..6bec492f77 100644 --- a/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_Edit_spec.js +++ b/app/client/cypress/integration/Smoke_TestSuite/ApiPaneTests/API_Edit_spec.js @@ -8,12 +8,7 @@ describe("API Panel Test Functionality", function() { cy.log("Navigation to API Panel screen successful"); cy.CreateAPI("FirstAPI"); cy.log("Creation of FirstAPI Action successful"); - cy.EnterSourceDetailsWithHeader( - testdata.baseUrl, - testdata.methods, - testdata.headerKey, - testdata.headerValue, - ); + cy.enterDatasourceAndPath(testdata.baseUrl, testdata.methods); cy.RunAPI(); cy.ResponseStatusCheck(testdata.successStatusCode); cy.get(apiwidget.createApiOnSideBar) diff --git a/app/client/cypress/integration/Smoke_TestSuite/OrganisationTests/CreateOrgTests_spec.js b/app/client/cypress/integration/Smoke_TestSuite/OrganisationTests/CreateOrgTests_spec.js index 93d695f2e8..a53a8c0ea3 100644 --- a/app/client/cypress/integration/Smoke_TestSuite/OrganisationTests/CreateOrgTests_spec.js +++ b/app/client/cypress/integration/Smoke_TestSuite/OrganisationTests/CreateOrgTests_spec.js @@ -121,5 +121,17 @@ describe("Create new org and share with a user", function() { expect($lis.eq(1)).to.contain(Cypress.env("TESTUSERNAME1")); expect($lis.eq(2)).to.contain(Cypress.env("TESTUSERNAME2")); }); + cy.xpath(homePage.appHome) + .should("be.visible") + .click(); + cy.wait("@applications").should( + "have.nested.property", + "response.body.responseMeta.status", + 200, + ); + cy.SearchApp(appid); + cy.get("#loading").should("not.exist"); + cy.wait("@getPropertyPane"); + cy.get("@getPropertyPane").should("have.property", "status", 200); }); }); diff --git a/app/client/cypress/support/commands.js b/app/client/cypress/support/commands.js index ce61024e39..3d73fb60c9 100644 --- a/app/client/cypress/support/commands.js +++ b/app/client/cypress/support/commands.js @@ -362,18 +362,14 @@ Cypress.Commands.add("EditApiName", apiname => { Cypress.Commands.add("WaitAutoSave", () => { // wait for save query to trigger - cy.wait(200); + cy.wait(2000); cy.wait("@saveAction"); //cy.wait("@postExecute"); }); Cypress.Commands.add("RunAPI", () => { cy.get(ApiEditor.ApiRunBtn).click({ force: true }); - cy.wait("@postExecute").should( - "have.nested.property", - "response.body.responseMeta.status", - 200, - ); + cy.wait("@postExecute"); }); Cypress.Commands.add("SaveAndRunAPI", () => { diff --git a/app/client/cypress/support/index.js b/app/client/cypress/support/index.js index d234f015a8..bbdf39e556 100644 --- a/app/client/cypress/support/index.js +++ b/app/client/cypress/support/index.js @@ -25,6 +25,12 @@ Cypress.on("uncaught:exception", (err, runnable) => { return false; }); +Cypress.on("fail", (error, runnable) => { + debugger; + throw error; // throw error to have test still fail + return false; +}); + before(function() { cy.startServerAndRoutes(); const username = Cypress.env("USERNAME"); diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/AppsmithPluginException.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/AppsmithPluginException.java index 3be7397c6a..effb2c75ec 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/AppsmithPluginException.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/AppsmithPluginException.java @@ -29,7 +29,7 @@ public class AppsmithPluginException extends Exception { } public Integer getAppErrorCode() { - return this.error.getAppErrorCode(); + return this.error == null ? 0 : this.error.getAppErrorCode(); } } diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/StaleConnectionException.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/StaleConnectionException.java new file mode 100644 index 0000000000..2c17c9acac --- /dev/null +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/pluginExceptions/StaleConnectionException.java @@ -0,0 +1,4 @@ +package com.appsmith.external.pluginExceptions; + +public class StaleConnectionException extends RuntimeException { +} diff --git a/app/server/appsmith-plugins/mysqlPlugin/pom.xml b/app/server/appsmith-plugins/mysqlPlugin/pom.xml index a3c1ddb834..718264d343 100644 --- a/app/server/appsmith-plugins/mysqlPlugin/pom.xml +++ b/app/server/appsmith-plugins/mysqlPlugin/pom.xml @@ -115,6 +115,22 @@ + + maven-dependency-plugin + + + copy-dependencies + package + + copy-dependencies + + + runtime + ${project.build.directory}/lib + + + + diff --git a/app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java b/app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java index 8194b6cad8..8142d1e4ff 100644 --- a/app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java +++ b/app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java @@ -1,8 +1,14 @@ package com.external.plugins; -import com.appsmith.external.models.*; +import com.appsmith.external.models.ActionConfiguration; +import com.appsmith.external.models.ActionExecutionResult; +import com.appsmith.external.models.AuthenticationDTO; +import com.appsmith.external.models.DatasourceConfiguration; +import com.appsmith.external.models.DatasourceTestResult; +import com.appsmith.external.models.Endpoint; import com.appsmith.external.pluginExceptions.AppsmithPluginError; import com.appsmith.external.pluginExceptions.AppsmithPluginException; +import com.appsmith.external.pluginExceptions.StaleConnectionException; import com.appsmith.external.plugins.BasePlugin; import com.appsmith.external.plugins.PluginExecutor; import lombok.extern.slf4j.Slf4j; @@ -13,9 +19,19 @@ import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import reactor.core.publisher.Mono; -import java.sql.*; import java.sql.Connection; -import java.util.*; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; import static com.appsmith.external.models.Connection.Mode.READ_ONLY; @@ -25,6 +41,7 @@ public class MySqlPlugin extends BasePlugin { private static final String USER = "user"; private static final String PASSWORD = "password"; + private static final int VALIDITY_CHECK_TIMEOUT = 5; public MySqlPlugin(PluginWrapper wrapper) { super(wrapper); @@ -39,11 +56,23 @@ public class MySqlPlugin extends BasePlugin { } @Override - public Mono execute(Object connection, DatasourceConfiguration datasourceConfiguration, + public Mono execute(Object connection, + DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration) { Connection conn = (Connection) connection; + try { + if (conn == null || conn.isClosed() || !conn.isValid(VALIDITY_CHECK_TIMEOUT)) { + log.info("Encountered stale connection in MySQL plugin. Reporting back."); + throw new StaleConnectionException(); + } + } catch (SQLException error) { + // This exception is thrown only when the timeout to `isValid` is negative. Since, that's not the case, + // here, this should never happen. + log.error("Error checking validity of MySQL connection.", error); + } + String query = actionConfiguration.getBody(); if (query == null) { @@ -149,7 +178,7 @@ public class MySqlPlugin extends BasePlugin { configurationConnection != null && READ_ONLY.equals(configurationConnection.getMode())); return Mono.just(connection); } catch (SQLException e) { - return pluginErrorMono("Error connecting to MySql.", e); + return pluginErrorMono("Error connecting to MySQL: " + e.getMessage(), e); } } diff --git a/app/server/appsmith-plugins/postgresPlugin/pom.xml b/app/server/appsmith-plugins/postgresPlugin/pom.xml index 359193b386..60b03972c8 100644 --- a/app/server/appsmith-plugins/postgresPlugin/pom.xml +++ b/app/server/appsmith-plugins/postgresPlugin/pom.xml @@ -115,6 +115,22 @@ + + maven-dependency-plugin + + + copy-dependencies + package + + copy-dependencies + + + runtime + ${project.build.directory}/lib + + + + diff --git a/app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java b/app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java index 23b73285dd..e96423e968 100644 --- a/app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java +++ b/app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java @@ -9,6 +9,7 @@ import com.appsmith.external.models.Endpoint; import com.appsmith.external.models.SSLDetails; import com.appsmith.external.pluginExceptions.AppsmithPluginError; import com.appsmith.external.pluginExceptions.AppsmithPluginException; +import com.appsmith.external.pluginExceptions.StaleConnectionException; import com.appsmith.external.plugins.BasePlugin; import com.appsmith.external.plugins.PluginExecutor; import lombok.NonNull; @@ -47,6 +48,7 @@ public class PostgresPlugin extends BasePlugin { private static final String PASSWORD = "password"; private static final String SSL = "ssl"; private static final String DATE_COLUMN_TYPE_NAME = "date"; + private static final int VALIDITY_CHECK_TIMEOUT = 5; public PostgresPlugin(PluginWrapper wrapper) { super(wrapper); @@ -61,12 +63,23 @@ public class PostgresPlugin extends BasePlugin { public static class PostgresPluginExecutor implements PluginExecutor { @Override - public Mono execute(@NonNull Object connection, + public Mono execute(Object connection, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration) { Connection conn = (Connection) connection; + try { + if (conn == null || conn.isClosed() || !conn.isValid(VALIDITY_CHECK_TIMEOUT)) { + log.info("Encountered stale connection in Postgres plugin. Reporting back."); + throw new StaleConnectionException(); + } + } catch (SQLException error) { + // This exception is thrown only when the timeout to `isValid` is negative. Since, that's not the case, + // here, this should never happen. + log.error("Error checking validity of Postgres connection.", error); + } + String query = actionConfiguration.getBody(); if (query == null) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java index 5c3fbc7f8a..acb7008375 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java @@ -11,6 +11,7 @@ import com.appsmith.external.models.Property; import com.appsmith.external.models.Provider; import com.appsmith.external.pluginExceptions.AppsmithPluginError; import com.appsmith.external.pluginExceptions.AppsmithPluginException; +import com.appsmith.external.pluginExceptions.StaleConnectionException; import com.appsmith.external.plugins.PluginExecutor; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.acl.PolicyGenerator; @@ -412,16 +413,34 @@ public class ActionServiceImpl extends BaseService pluginExecutor.execute( - resourceContext.getConnection(), - datasourceConfiguration, - actionConfiguration)) + Mono executionMono = Mono.just(datasource) + .flatMap(datasourceContextService::getDatasourceContext) + // Now that we have the context (connection details), execute the action. + .flatMap( + resourceContext -> pluginExecutor.execute( + resourceContext.getConnection(), + datasourceConfiguration, + actionConfiguration + ) + ); + + return executionMono + .onErrorResume(StaleConnectionException.class, error -> { + log.info("Looks like the connection is stale. Retrying with a fresh context."); + return datasourceContextService + .deleteDatasourceContext(datasource.getId()) + .then(executionMono); + }) .timeout(Duration.ofMillis(timeoutDuration)) + .onErrorMap( + StaleConnectionException.class, + error -> new AppsmithPluginException( + AppsmithPluginError.PLUGIN_ERROR, + "Secondary stale connection error." + ) + ) .onErrorResume(e -> { - log.debug("In the action execution error mode. Cause: {}", e.getMessage()); + log.debug("In the action execution error mode.", e); ActionExecutionResult result = new ActionExecutionResult(); result.setBody(e.getMessage()); result.setIsExecutionSuccess(false); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java index f4a44d07e7..f4a95ee3c3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java @@ -57,7 +57,7 @@ public class DatasourceContextServiceImpl implements DatasourceContextService { return Mono.just(datasourceContextMap.get(datasourceId)); } - log.debug("Datasource context doesn't exist. Creating connection"); + log.debug("Datasource context doesn't exist. Creating connection."); Mono datasourceMono; @@ -124,26 +124,24 @@ public class DatasourceContextServiceImpl implements DatasourceContextService { @Override public Mono deleteDatasourceContext(String datasourceId) { - DatasourceContext datasourceContext = datasourceContextMap.get(datasourceId); if (datasourceContext == null) { - //No resource context exists for this resource. Return void; + // No resource context exists for this resource. Return void. return Mono.empty(); } - Mono datasourceMono = datasourceService.findById(datasourceId, EXECUTE_DATASOURCES); - - Mono pluginMono = datasourceMono - .flatMap(datasource -> pluginService.findById(datasource.getPluginId())); - - //Datasource Context has not been created for this resource on this machine. Create one now. - Mono pluginExecutorMono = pluginExecutorHelper.getPluginExecutor(pluginMono); - - return Mono.zip(datasourceMono, pluginExecutorMono, ((datasource, pluginExecutor) -> { - pluginExecutor.datasourceDestroy(datasourceContext.getConnection()); - datasourceContextMap.remove(datasourceId); - return datasourceContext; - })); + return datasourceService + .findById(datasourceId, EXECUTE_DATASOURCES) + .zipWhen(datasource1 -> + pluginExecutorHelper.getPluginExecutor(pluginService.findById(datasource1.getPluginId())) + ) + .map(tuple -> { + final Datasource datasource = tuple.getT1(); + final PluginExecutor pluginExecutor = tuple.getT2(); + log.info("Clearing datasource context for datasource ID {}.", datasource.getId()); + pluginExecutor.datasourceDestroy(datasourceContext.getConnection()); + return datasourceContextMap.remove(datasourceId); + }); } @Override diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionServiceTest.java index dcf65b0090..b36ef563ba 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionServiceTest.java @@ -6,6 +6,7 @@ import com.appsmith.external.models.Policy; import com.appsmith.external.models.Property; import com.appsmith.external.pluginExceptions.AppsmithPluginError; import com.appsmith.external.pluginExceptions.AppsmithPluginException; +import com.appsmith.external.pluginExceptions.StaleConnectionException; import com.appsmith.external.plugins.PluginExecutor; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.constants.FieldName; @@ -517,6 +518,37 @@ public class ActionServiceTest { .verifyComplete(); } + @Test + @WithUserDetails(value = "api_user") + public void checkRecoveryFromStaleConnections() { + ActionExecutionResult mockResult = new ActionExecutionResult(); + mockResult.setIsExecutionSuccess(true); + mockResult.setBody("response-body"); + + Action action = new Action(); + ActionConfiguration actionConfiguration = new ActionConfiguration(); + actionConfiguration.setBody("select * from users"); + action.setActionConfiguration(actionConfiguration); + + ExecuteActionDTO executeActionDTO = new ExecuteActionDTO(); + executeActionDTO.setAction(action); + + Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(pluginExecutor)); + Mockito.when(pluginExecutor.execute(Mockito.any(), Mockito.any(), Mockito.any())) + .thenThrow(new StaleConnectionException()) + .thenReturn(Mono.just(mockResult)); + Mockito.when(pluginExecutor.datasourceCreate(Mockito.any())).thenReturn(Mono.empty()); + + Mono actionExecutionResultMono = actionService.executeAction(executeActionDTO); + + StepVerifier.create(actionExecutionResultMono) + .assertNext(result -> { + assertThat(result).isNotNull(); + assertThat(result.getBody()).isEqualTo(mockResult.getBody()); + }) + .verifyComplete(); + } + private void executeAndAssertAction(ExecuteActionDTO executeActionDTO, ActionConfiguration actionConfiguration, ActionExecutionResult mockResult) { Mono actionExecutionResultMono = executeAction(executeActionDTO, actionConfiguration, mockResult);