diff --git a/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java b/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java index cad47c8d18e8..571640d647c4 100644 --- a/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java +++ b/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java @@ -1,16 +1,29 @@ package com.external.plugins; +import com.appsmith.external.constants.DataType; +import com.appsmith.external.datatypes.AppsmithType; +import com.appsmith.external.dtos.ExecuteActionDTO; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; +import com.appsmith.external.helpers.DataTypeServiceUtils; +import com.appsmith.external.helpers.MustacheHelper; import com.appsmith.external.models.ActionConfiguration; +import com.appsmith.external.models.ActionExecutionRequest; import com.appsmith.external.models.ActionExecutionResult; import com.appsmith.external.models.BearerTokenAuth; import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.DatasourceStructure; +import com.appsmith.external.models.MustacheBindingToken; +import com.appsmith.external.models.Param; +import com.appsmith.external.models.Property; +import com.appsmith.external.models.PsParameterDTO; +import com.appsmith.external.models.RequestParamDTO; import com.appsmith.external.plugins.BasePlugin; import com.appsmith.external.plugins.PluginExecutor; +import com.appsmith.external.plugins.SmartSubstitutionInterface; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.ObjectUtils; import org.pf4j.Extension; import org.pf4j.PluginWrapper; @@ -18,14 +31,22 @@ import reactor.core.publisher.Mono; import reactor.core.scheduler.Schedulers; +import java.math.BigDecimal; import java.sql.Connection; +import java.sql.Date; import java.sql.DriverManager; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.Statement; +import java.sql.Time; +import java.sql.Timestamp; +import java.sql.Types; +import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -34,13 +55,21 @@ import java.util.Set; import java.util.TreeMap; import java.util.stream.Collectors; +import java.util.stream.IntStream; +import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_CLOSED_ERROR_MSG; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_INVALID_ERROR_MSG; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_NULL_ERROR_MSG; import static com.appsmith.external.helpers.PluginUtils.getColumnsListForJdbcPlugin; +import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel; +import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex; +import static com.external.plugins.exceptions.DatabricksErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG; import static com.external.plugins.exceptions.DatabricksErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG; +import static com.external.plugins.exceptions.DatabricksErrorMessages.QUERY_PREPARATION_FAILED_ERROR_MSG; import static com.external.plugins.exceptions.DatabricksPluginError.QUERY_EXECUTION_FAILED; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; public class DatabricksPlugin extends BasePlugin { @@ -71,20 +100,95 @@ public DatabricksPlugin(PluginWrapper wrapper) { @Slf4j @Extension - public static class DatabricksPluginExecutor implements PluginExecutor { + public static class DatabricksPluginExecutor implements PluginExecutor, SmartSubstitutionInterface { + + // Index of the "Use prepared statements" toggle within actionConfiguration.pluginSpecifiedTemplates. + private static final int PREPARED_STATEMENT_INDEX = 0; + + /** + * Overriding the default executeParameterized so that dynamic bindings ({{...}}) can be bound as + * PreparedStatement parameters instead of being substituted into the SQL text. When the user disables + * prepared statements (to bind identifiers/fragments that a PreparedStatement cannot parameterize), we + * fall back to the default mustache substitution path, matching the Postgres/MySQL/MSSQL plugins. + */ + @Override + public Mono executeParameterized( + Connection connection, + ExecuteActionDTO executeActionDTO, + DatasourceConfiguration datasourceConfiguration, + ActionConfiguration actionConfiguration) { + + log.debug(Thread.currentThread().getName() + ": executeParameterized() called for Databricks plugin."); + String query = actionConfiguration.getBody(); + + if (!StringUtils.hasLength(query)) { + return Mono.error(new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, "Missing required parameter: Query.")); + } + + Boolean isPreparedStatement; + final List properties = actionConfiguration.getPluginSpecifiedTemplates(); + if (properties == null + || properties.size() <= PREPARED_STATEMENT_INDEX + || properties.get(PREPARED_STATEMENT_INDEX) == null) { + // In case the prepared statement configuration is missing, default to true (safe by default). + isPreparedStatement = true; + } else { + Object psValue = properties.get(PREPARED_STATEMENT_INDEX).getValue(); + if (psValue instanceof Boolean) { + isPreparedStatement = (Boolean) psValue; + } else if (psValue instanceof String) { + // Only an explicit "false" disables prepared statements; any other value keeps it enabled. + isPreparedStatement = !"false".equalsIgnoreCase(((String) psValue).trim()); + } else { + isPreparedStatement = true; + } + } + + // In case of non-prepared statement, simply do bind replacement and execute the raw statement. + if (FALSE.equals(isPreparedStatement)) { + prepareConfigurationsForExecution(executeActionDTO, actionConfiguration, datasourceConfiguration); + return executeCommon(connection, actionConfiguration, FALSE, null, null); + } + + // Prepared statement: replace every binding with a `?` and bind the values on the PreparedStatement. + List mustacheKeysInOrder = MustacheHelper.extractMustacheKeysInOrder(query); + String updatedQuery = MustacheHelper.replaceMustacheWithQuestionMark(query, mustacheKeysInOrder); + actionConfiguration.setBody(updatedQuery); + return executeCommon(connection, actionConfiguration, TRUE, mustacheKeysInOrder, executeActionDTO); + } @Override public Mono execute( Connection connection, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration) { + // Raw (non-parameterized) execution path. Reached only when prepared statements are disabled, after + // the framework has already performed mustache substitution on the action configuration. + return executeCommon(connection, actionConfiguration, FALSE, null, null); + } + + private Mono executeCommon( + Connection connection, + ActionConfiguration actionConfiguration, + Boolean preparedStatement, + List mustacheValuesInOrder, + ExecuteActionDTO executeActionDTO) { - log.debug(Thread.currentThread().getName() + ": execute() called for Databricks plugin."); + log.debug(Thread.currentThread().getName() + ": executeCommon() called for Databricks plugin."); String query = actionConfiguration.getBody(); List> rowsList = new ArrayList<>(INITIAL_ROWLIST_CAPACITY); final List columnsList = new ArrayList<>(); + final Map requestData = new HashMap<>(); + requestData.put("preparedStatement", TRUE.equals(preparedStatement)); + Map psParams = TRUE.equals(preparedStatement) ? new LinkedHashMap<>() : null; + String transformedQuery = + TRUE.equals(preparedStatement) ? replaceQuestionMarkWithDollarIndex(query) : query; + List requestParams = + List.of(new RequestParamDTO(ACTION_CONFIGURATION_BODY, transformedQuery, null, null, psParams)); + return (Mono) Mono.fromCallable(() -> { log.debug(Thread.currentThread().getName() + ": creating action execution result from Databricks plugin."); @@ -113,22 +217,47 @@ public Mono execute( ActionExecutionResult result = new ActionExecutionResult(); result.setIsExecutionSuccess(true); + Statement statement = null; + PreparedStatement preparedQuery = null; + try { // We can proceed since the connection is valid. - Statement statement = connection.createStatement(); - boolean hasResultSet = statement.execute(query); + boolean hasResultSet; + ResultSet resultSet; + + if (FALSE.equals(preparedStatement)) { + statement = connection.createStatement(); + hasResultSet = statement.execute(query); + resultSet = statement.getResultSet(); + } else { + preparedQuery = connection.prepareStatement(query); + + List> parameters = new ArrayList<>(); + preparedQuery = (PreparedStatement) smartSubstitutionOfBindings( + preparedQuery, mustacheValuesInOrder, executeActionDTO.getParams(), parameters); + + requestData.put("ps-parameters", parameters); + IntStream.range(0, parameters.size()) + .forEachOrdered(i -> psParams.put( + getPSParamLabel(i + 1), + new PsParameterDTO( + parameters.get(i).getKey(), + parameters.get(i).getValue()))); + + hasResultSet = preparedQuery.execute(); + resultSet = preparedQuery.getResultSet(); + } if (!hasResultSet) { // This must be an update/delete/insert kind of query which did not return any results. // Lets set sample response and return back. Map successResponse = Map.of("success", true); result.setBody(objectMapper.valueToTree(successResponse)); + result.setRequest(buildRequest(transformedQuery, requestData, requestParams)); return Mono.just(result); } - ResultSet resultSet = statement.getResultSet(); - ResultSetMetaData metaData = resultSet.getMetaData(); int colCount = metaData.getColumnCount(); columnsList.addAll(getColumnsListForJdbcPlugin(metaData)); @@ -169,15 +298,147 @@ public Mono execute( QUERY_EXECUTION_FAILED_ERROR_MSG, e.getMessage(), "SQLSTATE: " + sqlState)); + } finally { + if (preparedQuery != null) { + try { + preparedQuery.close(); + } catch (SQLException e) { + log.error("Error closing Databricks PreparedStatement : " + e.getMessage()); + } + } + if (statement != null) { + try { + statement.close(); + } catch (SQLException e) { + log.error("Error closing Databricks Statement : " + e.getMessage()); + } + } } result.setBody(objectMapper.valueToTree(rowsList)); + result.setRequest(buildRequest(transformedQuery, requestData, requestParams)); return Mono.just(result); }) .flatMap(obj -> obj) .subscribeOn(Schedulers.boundedElastic()); } + private ActionExecutionRequest buildRequest( + String query, Map requestData, List requestParams) { + ActionExecutionRequest request = new ActionExecutionRequest(); + request.setQuery(query); + request.setProperties(requestData); + request.setRequestParams(requestParams); + return request; + } + + @Override + public Object substituteValueInInput( + int index, + String binding, + String value, + Object input, + List> insertedParams, + Object... args) + throws AppsmithPluginException { + + PreparedStatement preparedStatement = (PreparedStatement) input; + Param param = (Param) args[0]; + AppsmithType appsmithType = DataTypeServiceUtils.getAppsmithType(param.getClientDataType(), value); + DataType valueType = appsmithType.type(); + + Map.Entry parameter = new SimpleEntry<>(value, valueType.toString()); + insertedParams.add(parameter); + + try { + switch (valueType) { + case NULL: { + preparedStatement.setNull(index, Types.NULL); + break; + } + case BINARY: { + preparedStatement.setBinaryStream(index, IOUtils.toInputStream(value)); + break; + } + case BYTES: { + preparedStatement.setBytes(index, value.getBytes("UTF-8")); + break; + } + case INTEGER: { + preparedStatement.setInt(index, Integer.parseInt(value)); + break; + } + case LONG: { + preparedStatement.setLong(index, Long.parseLong(value)); + break; + } + case FLOAT: + case DOUBLE: { + preparedStatement.setBigDecimal(index, new BigDecimal(String.valueOf(value))); + break; + } + case BOOLEAN: { + preparedStatement.setBoolean(index, Boolean.parseBoolean(value)); + break; + } + case DATE: { + preparedStatement.setDate(index, Date.valueOf(value)); + break; + } + case TIME: { + preparedStatement.setTime(index, Time.valueOf(value)); + break; + } + case TIMESTAMP: { + preparedStatement.setTimestamp(index, Timestamp.valueOf(value)); + break; + } + case STRING: + case JSON_OBJECT: { + preparedStatement.setString(index, value); + break; + } + case ARRAY: + case NULL_ARRAY: + // Array-typed values cannot be bound as a scalar JDBC parameter. Fail fast with a clear + // message rather than leaving the placeholder unbound and hitting a confusing driver error. + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, binding)); + default: + // An unrecognized data type would otherwise leave the placeholder unbound while it is + // still tracked as an inserted parameter, surfacing as a confusing driver error later. + log.warn( + "Unrecognized data type {} for binding {}; skipping parameter binding", + valueType, + binding); + break; + } + } catch (SQLException | IllegalArgumentException | java.io.IOException e) { + if ((e instanceof SQLException) + && e.getMessage() != null + && e.getMessage().contains("The column index is out of range:")) { + // The parameter is likely being set inside a commented-out part of the query. Ignore it. + } else { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(QUERY_PREPARATION_FAILED_ERROR_MSG, value, binding), + e.getMessage()); + } + } + + return preparedStatement; + } + + /** + * Quote a value as a Databricks (Spark SQL) identifier using backticks, escaping any embedded backticks by + * doubling them. Used for catalog/schema names that are concatenated into `USE CATALOG` / `USE SCHEMA` + * statements, which cannot be parameterized via a PreparedStatement. + */ + private static String quoteIdentifier(String identifier) { + return "`" + identifier.replace("`", "``") + "`"; + } + @Override public Mono datasourceCreate(DatasourceConfiguration datasourceConfiguration) { @@ -261,7 +522,9 @@ public Mono datasourceCreate(DatasourceConfiguration datasourceConfi if (!StringUtils.hasText(catalog)) { catalog = "samples"; } - String useCatalogQuery = "USE CATALOG " + catalog; + // Catalog name comes from datasource configuration (requires MANAGE_DATASOURCES). + // Quote it as a Databricks identifier so it cannot inject additional SQL. + String useCatalogQuery = "USE CATALOG " + quoteIdentifier(catalog); statement.execute(useCatalogQuery); } catch (SQLException e) { return Mono.error(new AppsmithPluginException( @@ -278,7 +541,9 @@ public Mono datasourceCreate(DatasourceConfiguration datasourceConfi if (!StringUtils.hasText(schema)) { schema = "default"; } - String useSchemaQuery = "USE SCHEMA " + schema; + // Schema name comes from datasource configuration (requires MANAGE_DATASOURCES). + // Quote it as a Databricks identifier so it cannot inject additional SQL. + String useSchemaQuery = "USE SCHEMA " + quoteIdentifier(schema); statement.execute(useSchemaQuery); } catch (SQLException e) { return Mono.error(new AppsmithPluginException( diff --git a/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/exceptions/DatabricksErrorMessages.java b/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/exceptions/DatabricksErrorMessages.java index e4ce3e43bb77..386de1f698df 100644 --- a/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/exceptions/DatabricksErrorMessages.java +++ b/app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/exceptions/DatabricksErrorMessages.java @@ -7,4 +7,10 @@ public class DatabricksErrorMessages { public static final String QUERY_EXECUTION_FAILED_ERROR_MSG = "Your query failed to execute. "; + + public static final String QUERY_PREPARATION_FAILED_ERROR_MSG = + "Query preparation failed while inserting value: %s for binding: {{%s}}. Please check the query again."; + + public static final String ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG = + "ARRAY parameters are not supported by prepared statements for this plugin. Binding: {{%s}}."; } diff --git a/app/server/appsmith-plugins/databricksPlugin/src/main/resources/dependency.json b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/dependency.json new file mode 100644 index 000000000000..5953a9c44360 --- /dev/null +++ b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/dependency.json @@ -0,0 +1,7 @@ +{ + "dependencies": { + "actionConfiguration.body": [ + "actionConfiguration.pluginSpecifiedTemplates[0].value" + ] + } +} diff --git a/app/server/appsmith-plugins/databricksPlugin/src/main/resources/editor/root.json b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/editor/root.json index bab24a7605cd..fbd93237e19d 100644 --- a/app/server/appsmith-plugins/databricksPlugin/src/main/resources/editor/root.json +++ b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/editor/root.json @@ -10,9 +10,27 @@ "children": [ { "label": "", + "propertyName": "query_prepared", "configProperty": "actionConfiguration.body", "controlType": "QUERY_DYNAMIC_TEXT", - "evaluationSubstitutionType": "TEMPLATE" + "evaluationSubstitutionType": "PARAMETER", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": false + } + }, + { + "label": "", + "propertyName": "query_non_prepared", + "configProperty": "actionConfiguration.body", + "controlType": "QUERY_DYNAMIC_TEXT", + "evaluationSubstitutionType": "TEMPLATE", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": true + } } ] } diff --git a/app/server/appsmith-plugins/databricksPlugin/src/main/resources/setting.json b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/setting.json new file mode 100644 index 000000000000..74bebf3b4fa0 --- /dev/null +++ b/app/server/appsmith-plugins/databricksPlugin/src/main/resources/setting.json @@ -0,0 +1,54 @@ +{ + "setting": [ + { + "sectionName": "", + "id": 1, + "children": [ + { + "label": "Run behavior", + "configProperty": "runBehaviour", + "controlType": "DROP_DOWN", + "initialValue": "MANUAL", + "options": [ + { + "label": "Automatic", + "subText": "Query runs on page load or when a variable it depends on changes", + "value": "AUTOMATIC" + }, + { + "label": "On page load", + "subText": "Query runs when the page loads or when manually triggered", + "value": "ON_PAGE_LOAD" + }, + { + "label": "Manual", + "subText": "Query only runs when called in an event or JS with .run()", + "value": "MANUAL" + } + ] + }, + { + "label": "Request confirmation before running this query", + "configProperty": "confirmBeforeExecute", + "controlType": "SWITCH", + "tooltipText": "Ask confirmation from the user each time before refreshing data" + }, + { + "label": "Use prepared statements", + "tooltipText": "Prepared statements prevent SQL injections on your queries but do not support dynamic bindings outside values in your SQL", + "configProperty": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "controlType": "SWITCH_WITH_CONFIRMATION", + "initialValue": true, + "modalType": "PREPARED_STATEMENT" + }, + { + "label": "Query timeout (in milliseconds)", + "subtitle": "Maximum time after which the query will return", + "configProperty": "actionConfiguration.timeoutInMillisecond", + "controlType": "INPUT_TEXT", + "dataType": "NUMBER" + } + ] + } + ] +} diff --git a/app/server/appsmith-plugins/databricksPlugin/src/test/java/com/external/plugins/DatabricksPluginTest.java b/app/server/appsmith-plugins/databricksPlugin/src/test/java/com/external/plugins/DatabricksPluginTest.java index 58cc364526ff..4a664105889c 100644 --- a/app/server/appsmith-plugins/databricksPlugin/src/test/java/com/external/plugins/DatabricksPluginTest.java +++ b/app/server/appsmith-plugins/databricksPlugin/src/test/java/com/external/plugins/DatabricksPluginTest.java @@ -1,22 +1,47 @@ package com.external.plugins; +import com.appsmith.external.datatypes.ClientDataType; +import com.appsmith.external.dtos.ExecuteActionDTO; +import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionResult; import com.appsmith.external.models.DatasourceConfiguration; +import com.appsmith.external.models.Param; +import com.appsmith.external.models.Property; +import com.external.plugins.exceptions.DatabricksErrorMessages; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; +import org.springframework.core.io.ClassPathResource; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; import java.sql.Connection; +import java.sql.PreparedStatement; import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_CLOSED_ERROR_MSG; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_INVALID_ERROR_MSG; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.CONNECTION_NULL_ERROR_MSG; import static com.external.plugins.DatabricksPlugin.VALIDITY_CHECK_TIMEOUT; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class DatabricksPluginTest { @@ -65,4 +90,164 @@ public void testExecuteInvalidConnection() throws SQLException { && throwable.getMessage().equals(CONNECTION_INVALID_ERROR_MSG)) .verify(); } + + // ------------------------------------------------------------------ + // Review-fix regression tests + // ------------------------------------------------------------------ + + /** + * Runs executeParameterized against a connection where BOTH the prepared and raw paths return a no-result-set + * (write) query, so a test can assert which path was chosen for a given prepared-statement setting. + */ + private Connection runDatabricks(List templates) throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isClosed()).thenReturn(false); + when(connection.isValid(VALIDITY_CHECK_TIMEOUT)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + when(ps.execute()).thenReturn(false); + + Statement st = mock(Statement.class); + when(connection.createStatement()).thenReturn(st); + when(st.execute(anyString())).thenReturn(false); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + ac.setPluginSpecifiedTemplates(templates); + + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue("value"); + param.setClientDataType(ClientDataType.STRING); + dto.setParams(List.of(param)); + + StepVerifier.create(databricksPluginExecutor.executeParameterized( + connection, dto, new DatasourceConfiguration(), ac)) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + return connection; + } + + /** + * The prepared path binds a value as a `?` placeholder and preserves the no-result-set write behavior (a + * synthetic success body), without ever taking the raw Statement path. + */ + @Test + public void testExecuteParameterizedPreparedStatementBindsAndPreservesWriteBehavior() throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isClosed()).thenReturn(false); + when(connection.isValid(VALIDITY_CHECK_TIMEOUT)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + // No result set: an update/insert/delete-style statement. + when(ps.execute()).thenReturn(false); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + ac.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + final String paramValue = "O'Reilly -- value"; + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue(paramValue); + param.setClientDataType(ClientDataType.STRING); + dto.setParams(List.of(param)); + + StepVerifier.create(databricksPluginExecutor.executeParameterized( + connection, dto, new DatasourceConfiguration(), ac)) + .assertNext(result -> { + assertTrue(result.getIsExecutionSuccess()); + // No-result-set write returns the synthetic success body. + assertTrue(result.getBody().toString().contains("success")); + }) + .verifyComplete(); + + verify(connection).prepareStatement("SELECT * FROM users WHERE name = ?"); + verify(ps).setString(eq(1), eq(paramValue)); + verify(connection, never()).createStatement(); + verify(ps).close(); + } + + /** The prepared-statement setting defaults to ON when absent or malformed. */ + @Test + public void testPreparedStatementDefaultsOnWhenSettingAbsentOrMalformed() throws SQLException { + Property boolTrue = new Property(); + boolTrue.setKey("preparedStatement"); + boolTrue.setValue(Boolean.TRUE); + + for (List templates : Arrays.asList( + (List) null, + new ArrayList(), + Arrays.asList((Property) null), + List.of(new Property("preparedStatement", "true")), + List.of(new Property("preparedStatement", "banana")), + List.of(boolTrue))) { + Connection connection = runDatabricks(templates); + verify(connection).prepareStatement("SELECT * FROM users WHERE name = ?"); + verify(connection, never()).createStatement(); + } + } + + /** Raw execution is used only when the user explicitly disables prepared statements. */ + @Test + public void testRawStatementOnlyWhenExplicitlyDisabled() throws SQLException { + Property boolFalse = new Property(); + boolFalse.setKey("preparedStatement"); + boolFalse.setValue(Boolean.FALSE); + + for (List templates : + Arrays.asList(List.of(new Property("preparedStatement", "false")), List.of(boolFalse))) { + Connection connection = runDatabricks(templates); + verify(connection).createStatement(); + verify(connection, never()).prepareStatement(any()); + } + } + + /** Array-typed params cannot be bound as scalar JDBC parameters and must fail with a clear message. */ + @Test + public void testExecuteParameterizedRejectsArrayParams() throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isClosed()).thenReturn(false); + when(connection.isValid(VALIDITY_CHECK_TIMEOUT)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM t WHERE id = {{Input1.ids}}"); + ac.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.ids"); + param.setValue("[1, 2, 3]"); + param.setClientDataType(ClientDataType.ARRAY); + dto.setParams(List.of(param)); + + StepVerifier.create(databricksPluginExecutor.executeParameterized( + connection, dto, new DatasourceConfiguration(), ac)) + .expectErrorMatches(e -> e instanceof AppsmithPluginException + && e.getMessage() + .contains(String.format( + DatabricksErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, "Input1.ids"))) + .verify(); + + verify(connection, never()).createStatement(); + verify(ps).close(); + } + + /** The plugin ships a dependency.json so the client re-evaluates the query body when the toggle flips. */ + @Test + public void testDependencyConfigLinksBodyToPreparedStatementToggle() throws IOException { + InputStream input = new ClassPathResource("dependency.json").getInputStream(); + String content = new BufferedReader(new InputStreamReader(input)) + .lines() + .collect(Collectors.joining(System.lineSeparator())); + assertTrue(content.contains("actionConfiguration.body")); + assertTrue(content.contains("actionConfiguration.pluginSpecifiedTemplates[0].value")); + } } diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java b/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java index 7baff899b0b1..e708cf7ef9b9 100644 --- a/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java +++ b/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java @@ -1,8 +1,13 @@ package com.external.plugins; +import com.appsmith.external.constants.DataType; +import com.appsmith.external.datatypes.AppsmithType; +import com.appsmith.external.dtos.ExecuteActionDTO; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; +import com.appsmith.external.helpers.DataTypeServiceUtils; +import com.appsmith.external.helpers.MustacheHelper; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionRequest; import com.appsmith.external.models.ActionExecutionResult; @@ -10,9 +15,14 @@ import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.DatasourceStructure; import com.appsmith.external.models.Endpoint; +import com.appsmith.external.models.MustacheBindingToken; +import com.appsmith.external.models.Param; +import com.appsmith.external.models.Property; +import com.appsmith.external.models.PsParameterDTO; import com.appsmith.external.models.RequestParamDTO; import com.appsmith.external.plugins.BasePlugin; import com.appsmith.external.plugins.PluginExecutor; +import com.appsmith.external.plugins.SmartSubstitutionInterface; import com.external.plugins.exceptions.RedshiftErrorMessages; import com.external.plugins.exceptions.RedshiftPluginError; import com.external.utils.RedshiftDatasourceUtils; @@ -20,6 +30,7 @@ import com.zaxxer.hikari.HikariPoolMXBean; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.ObjectUtils; import org.pf4j.Extension; import org.pf4j.PluginWrapper; @@ -29,14 +40,21 @@ import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; +import java.math.BigDecimal; import java.sql.Connection; +import java.sql.Date; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.Statement; +import java.sql.Time; +import java.sql.Timestamp; +import java.sql.Types; import java.time.LocalDateTime; import java.time.OffsetDateTime; import java.time.format.DateTimeFormatter; +import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; import java.util.Comparator; import java.util.HashMap; @@ -46,13 +64,18 @@ import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.IntStream; import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY; import static com.appsmith.external.constants.PluginConstants.PluginName.REDSHIFT_PLUGIN_NAME; import static com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages.JDBC_DRIVER_LOADING_ERROR_MSG; import static com.appsmith.external.helpers.PluginUtils.getColumnsListForJdbcPlugin; import static com.appsmith.external.helpers.PluginUtils.getIdenticalColumns; +import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel; +import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex; import static com.external.utils.RedshiftDatasourceUtils.createConnectionPool; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; import static org.apache.commons.lang3.StringUtils.isBlank; @Slf4j @@ -67,10 +90,13 @@ public RedshiftPlugin(PluginWrapper wrapper) { } @Extension - public static class RedshiftPluginExecutor implements PluginExecutor { + public static class RedshiftPluginExecutor implements PluginExecutor, SmartSubstitutionInterface { private final Scheduler scheduler = Schedulers.boundedElastic(); + // Index of the "Use prepared statements" toggle within actionConfiguration.pluginSpecifiedTemplates. + private static final int PREPARED_STATEMENT_INDEX = 0; + private static final String TABLES_QUERY = "select a.attname as name,\n" + " t1.typname as column_type,\n" @@ -189,16 +215,78 @@ private Map getRow(ResultSet resultSet) throws SQLException, App return row; } + /** + * Overriding the default executeParameterized so that dynamic bindings ({{...}}) can be bound as + * PreparedStatement parameters instead of being substituted into the SQL text. When the user disables + * prepared statements (to bind identifiers/fragments that a PreparedStatement cannot parameterize), we + * fall back to the default mustache substitution path, matching the Postgres/MySQL/MSSQL plugins. + */ + @Override + public Mono executeParameterized( + HikariDataSource connectionPool, + ExecuteActionDTO executeActionDTO, + DatasourceConfiguration datasourceConfiguration, + ActionConfiguration actionConfiguration) { + + log.debug(Thread.currentThread().getName() + ": executeParameterized() called for Redshift plugin."); + String query = actionConfiguration.getBody(); + if (!StringUtils.hasLength(query)) { + return Mono.error(new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + RedshiftErrorMessages.QUERY_PARAMETER_MISSING_ERROR_MSG)); + } + + Boolean isPreparedStatement; + final List properties = actionConfiguration.getPluginSpecifiedTemplates(); + if (properties == null + || properties.size() <= PREPARED_STATEMENT_INDEX + || properties.get(PREPARED_STATEMENT_INDEX) == null) { + // In case the prepared statement configuration is missing, default to true (safe by default). + isPreparedStatement = true; + } else { + Object psValue = properties.get(PREPARED_STATEMENT_INDEX).getValue(); + if (psValue instanceof Boolean) { + isPreparedStatement = (Boolean) psValue; + } else if (psValue instanceof String) { + // Only an explicit "false" disables prepared statements; any other value keeps it enabled. + isPreparedStatement = !"false".equalsIgnoreCase(((String) psValue).trim()); + } else { + isPreparedStatement = true; + } + } + + // In case of non-prepared statement, simply do bind replacement and execute the raw statement. + if (FALSE.equals(isPreparedStatement)) { + prepareConfigurationsForExecution(executeActionDTO, actionConfiguration, datasourceConfiguration); + return executeCommon(connectionPool, actionConfiguration, FALSE, null, null); + } + + // Prepared statement: replace every binding with a `?` and bind the values on the PreparedStatement. + List mustacheKeysInOrder = MustacheHelper.extractMustacheKeysInOrder(query); + String updatedQuery = MustacheHelper.replaceMustacheWithQuestionMark(query, mustacheKeysInOrder); + actionConfiguration.setBody(updatedQuery); + return executeCommon(connectionPool, actionConfiguration, TRUE, mustacheKeysInOrder, executeActionDTO); + } + @Override public Mono execute( HikariDataSource connectionPool, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration) { + // Raw (non-parameterized) execution path. Reached only when prepared statements are disabled, after + // the framework has already performed mustache substitution on the action configuration. + return executeCommon(connectionPool, actionConfiguration, FALSE, null, null); + } + + private Mono executeCommon( + HikariDataSource connectionPool, + ActionConfiguration actionConfiguration, + Boolean preparedStatement, + List mustacheValuesInOrder, + ExecuteActionDTO executeActionDTO) { - log.debug(Thread.currentThread().getName() + ": execute() called for Redshift plugin."); + log.debug(Thread.currentThread().getName() + ": executeCommon() called for Redshift plugin."); String query = actionConfiguration.getBody(); - List requestParams = - List.of(new RequestParamDTO(ACTION_CONFIGURATION_BODY, query, null, null, null)); if (!StringUtils.hasLength(query)) { return Mono.error(new AppsmithPluginException( @@ -206,6 +294,14 @@ public Mono execute( RedshiftErrorMessages.QUERY_PARAMETER_MISSING_ERROR_MSG)); } + final Map requestData = new HashMap<>(); + requestData.put("preparedStatement", TRUE.equals(preparedStatement)); + Map psParams = TRUE.equals(preparedStatement) ? new LinkedHashMap<>() : null; + String transformedQuery = + TRUE.equals(preparedStatement) ? replaceQuestionMarkWithDollarIndex(query) : query; + List requestParams = + List.of(new RequestParamDTO(ACTION_CONFIGURATION_BODY, transformedQuery, null, null, psParams)); + return Mono.fromCallable(() -> { Connection connection = null; try { @@ -241,14 +337,40 @@ public Mono execute( List> rowsList = new ArrayList<>(50); final List columnsList = new ArrayList<>(); Statement statement = null; + PreparedStatement preparedQuery = null; ResultSet resultSet = null; try { - statement = connection.createStatement(); - boolean isResultSet = statement.execute(query); + boolean isResultSet; + int updateCount; + + if (FALSE.equals(preparedStatement)) { + statement = connection.createStatement(); + isResultSet = statement.execute(query); + updateCount = statement.getUpdateCount(); + } else { + preparedQuery = connection.prepareStatement(query); + + List> parameters = new ArrayList<>(); + preparedQuery = (PreparedStatement) smartSubstitutionOfBindings( + preparedQuery, mustacheValuesInOrder, executeActionDTO.getParams(), parameters); + + requestData.put("ps-parameters", parameters); + IntStream.range(0, parameters.size()) + .forEachOrdered(i -> psParams.put( + getPSParamLabel(i + 1), + new PsParameterDTO( + parameters.get(i).getKey(), + parameters.get(i).getValue()))); + + isResultSet = preparedQuery.execute(); + updateCount = preparedQuery.getUpdateCount(); + } if (isResultSet) { - resultSet = statement.getResultSet(); + resultSet = FALSE.equals(preparedStatement) + ? statement.getResultSet() + : preparedQuery.getResultSet(); ResultSetMetaData metaData = resultSet.getMetaData(); columnsList.addAll(getColumnsListForJdbcPlugin(metaData)); @@ -257,8 +379,7 @@ public Mono execute( rowsList.add(row); } } else { - rowsList.add(Map.of( - "affectedRows", ObjectUtils.defaultIfNull(statement.getUpdateCount(), 0))); + rowsList.add(Map.of("affectedRows", ObjectUtils.defaultIfNull(updateCount, 0))); } } catch (SQLException e) { e.printStackTrace(); @@ -268,6 +389,15 @@ public Mono execute( e.getMessage(), "SQLSTATE: " + e.getSQLState())); } finally { + if (preparedQuery != null) { + try { + preparedQuery.close(); + } catch (SQLException e) { + log.error("Error closing Redshift PreparedStatement"); + e.printStackTrace(); + } + } + if (resultSet != null) { try { resultSet.close(); @@ -323,6 +453,7 @@ public Mono execute( .map(actionExecutionResult -> { ActionExecutionRequest request = new ActionExecutionRequest(); request.setQuery(query); + request.setProperties(requestData); request.setRequestParams(requestParams); ActionExecutionResult result = actionExecutionResult; result.setRequest(request); @@ -367,6 +498,104 @@ private Set populateHintMessages(List columnNames) { return messages; } + @Override + public Object substituteValueInInput( + int index, + String binding, + String value, + Object input, + List> insertedParams, + Object... args) + throws AppsmithPluginException { + + PreparedStatement preparedStatement = (PreparedStatement) input; + Param param = (Param) args[0]; + AppsmithType appsmithType = DataTypeServiceUtils.getAppsmithType(param.getClientDataType(), value); + DataType valueType = appsmithType.type(); + + Map.Entry parameter = new SimpleEntry<>(value, valueType.toString()); + insertedParams.add(parameter); + + try { + switch (valueType) { + case NULL: { + preparedStatement.setNull(index, Types.NULL); + break; + } + case BINARY: { + preparedStatement.setBinaryStream(index, IOUtils.toInputStream(value)); + break; + } + case BYTES: { + preparedStatement.setBytes(index, value.getBytes("UTF-8")); + break; + } + case INTEGER: { + preparedStatement.setInt(index, Integer.parseInt(value)); + break; + } + case LONG: { + preparedStatement.setLong(index, Long.parseLong(value)); + break; + } + case FLOAT: + case DOUBLE: { + preparedStatement.setBigDecimal(index, new BigDecimal(String.valueOf(value))); + break; + } + case BOOLEAN: { + preparedStatement.setBoolean(index, Boolean.parseBoolean(value)); + break; + } + case DATE: { + preparedStatement.setDate(index, Date.valueOf(value)); + break; + } + case TIME: { + preparedStatement.setTime(index, Time.valueOf(value)); + break; + } + case TIMESTAMP: { + preparedStatement.setTimestamp(index, Timestamp.valueOf(value)); + break; + } + case STRING: + case JSON_OBJECT: { + preparedStatement.setString(index, value); + break; + } + case ARRAY: + case NULL_ARRAY: + // Array-typed values cannot be bound as a scalar JDBC parameter. Fail fast with a clear + // message rather than leaving the placeholder unbound and hitting a confusing driver error. + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(RedshiftErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, binding)); + default: + // An unrecognized data type would otherwise leave the placeholder unbound while it is + // still tracked as an inserted parameter, surfacing as a confusing driver error later. + log.warn( + "Unrecognized data type {} for binding {}; skipping parameter binding", + valueType, + binding); + break; + } + } catch (SQLException | IllegalArgumentException | java.io.IOException e) { + if ((e instanceof SQLException) + && e.getMessage() != null + && e.getMessage().contains("The column index is out of range:")) { + // The parameter is likely being set inside a commented-out part of the query. Ignore it. + } else { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(RedshiftErrorMessages.QUERY_PREPARATION_FAILED_ERROR_MSG, value, binding), + e.getMessage()); + } + } + + return preparedStatement; + } + @Override public Mono datasourceCreate(DatasourceConfiguration datasourceConfiguration) { log.debug(Thread.currentThread().getName() + ": datasourceCreate() called for Redshift plugin."); diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/exceptions/RedshiftErrorMessages.java b/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/exceptions/RedshiftErrorMessages.java index 9fc589ebfee1..cba17ad5aac6 100644 --- a/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/exceptions/RedshiftErrorMessages.java +++ b/app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/exceptions/RedshiftErrorMessages.java @@ -18,6 +18,12 @@ public class RedshiftErrorMessages extends BasePluginErrorMessages { public static final String QUERY_EXECUTION_FAILED_ERROR_MSG = "Error occurred while executing Redshift query. To know more please check the error details."; + public static final String QUERY_PREPARATION_FAILED_ERROR_MSG = + "Query preparation failed while inserting value: %s for binding: {{%s}}. Please check the query again."; + + public static final String ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG = + "ARRAY parameters are not supported by prepared statements for this plugin. Binding: {{%s}}."; + public static final String GET_STRUCTURE_ERROR_MSG = "Appsmith server has failed to fetch the structure of the database. " + "Please check if the database credentials are valid and/or you have the required permissions."; diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/dependency.json b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/dependency.json new file mode 100644 index 000000000000..5953a9c44360 --- /dev/null +++ b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/dependency.json @@ -0,0 +1,7 @@ +{ + "dependencies": { + "actionConfiguration.body": [ + "actionConfiguration.pluginSpecifiedTemplates[0].value" + ] + } +} diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/editor.json b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/editor.json index d960202031f2..6c2341675528 100644 --- a/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/editor.json +++ b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/editor.json @@ -12,8 +12,28 @@ { "label": "", "internalLabel": "Query", + "propertyName": "query_prepared", "configProperty": "actionConfiguration.body", - "controlType": "QUERY_DYNAMIC_TEXT" + "controlType": "QUERY_DYNAMIC_TEXT", + "evaluationSubstitutionType": "PARAMETER", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": false + } + }, + { + "label": "", + "internalLabel": "Query", + "propertyName": "query_non_prepared", + "configProperty": "actionConfiguration.body", + "controlType": "QUERY_DYNAMIC_TEXT", + "evaluationSubstitutionType": "TEMPLATE", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": true + } } ] } diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/setting.json b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/setting.json new file mode 100644 index 000000000000..74bebf3b4fa0 --- /dev/null +++ b/app/server/appsmith-plugins/redshiftPlugin/src/main/resources/setting.json @@ -0,0 +1,54 @@ +{ + "setting": [ + { + "sectionName": "", + "id": 1, + "children": [ + { + "label": "Run behavior", + "configProperty": "runBehaviour", + "controlType": "DROP_DOWN", + "initialValue": "MANUAL", + "options": [ + { + "label": "Automatic", + "subText": "Query runs on page load or when a variable it depends on changes", + "value": "AUTOMATIC" + }, + { + "label": "On page load", + "subText": "Query runs when the page loads or when manually triggered", + "value": "ON_PAGE_LOAD" + }, + { + "label": "Manual", + "subText": "Query only runs when called in an event or JS with .run()", + "value": "MANUAL" + } + ] + }, + { + "label": "Request confirmation before running this query", + "configProperty": "confirmBeforeExecute", + "controlType": "SWITCH", + "tooltipText": "Ask confirmation from the user each time before refreshing data" + }, + { + "label": "Use prepared statements", + "tooltipText": "Prepared statements prevent SQL injections on your queries but do not support dynamic bindings outside values in your SQL", + "configProperty": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "controlType": "SWITCH_WITH_CONFIRMATION", + "initialValue": true, + "modalType": "PREPARED_STATEMENT" + }, + { + "label": "Query timeout (in milliseconds)", + "subtitle": "Maximum time after which the query will return", + "configProperty": "actionConfiguration.timeoutInMillisecond", + "controlType": "INPUT_TEXT", + "dataType": "NUMBER" + } + ] + } + ] +} diff --git a/app/server/appsmith-plugins/redshiftPlugin/src/test/java/com/external/plugins/RedshiftPluginTest.java b/app/server/appsmith-plugins/redshiftPlugin/src/test/java/com/external/plugins/RedshiftPluginTest.java index 504f5402017b..44b92bf4457f 100644 --- a/app/server/appsmith-plugins/redshiftPlugin/src/test/java/com/external/plugins/RedshiftPluginTest.java +++ b/app/server/appsmith-plugins/redshiftPlugin/src/test/java/com/external/plugins/RedshiftPluginTest.java @@ -1,5 +1,7 @@ package com.external.plugins; +import com.appsmith.external.datatypes.ClientDataType; +import com.appsmith.external.dtos.ExecuteActionDTO; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; import com.appsmith.external.models.ActionConfiguration; @@ -8,7 +10,10 @@ import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.DatasourceStructure; import com.appsmith.external.models.Endpoint; +import com.appsmith.external.models.Param; +import com.appsmith.external.models.Property; import com.appsmith.external.models.RequestParamDTO; +import com.external.plugins.exceptions.RedshiftErrorMessages; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; @@ -17,11 +22,17 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.mockito.Mockito; +import org.springframework.core.io.ClassPathResource; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; import java.sql.Connection; import java.sql.Date; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; @@ -40,13 +51,16 @@ import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -698,4 +712,234 @@ public void testGetEndpointIdentifierForRateLimit_HostPresentPortAbsent_ReturnsC }) .verifyComplete(); } + + /** + * When prepared statements are enabled (the default), a dynamic binding must be replaced by a `?` + * placeholder and its value bound on the PreparedStatement, not substituted into the SQL text. A param + * value containing SQL metacharacters must reach the driver as a bound parameter, never as SQL text. + */ + @Test + public void testExecuteParameterizedBindsValuesInsteadOfInterpolating() throws SQLException { + HikariDataSource mockConnectionPool = mock(HikariDataSource.class); + when(mockConnectionPool.isClosed()).thenReturn(false); + when(mockConnectionPool.isRunning()).thenReturn(true); + + Connection mockConnection = mock(Connection.class); + when(mockConnection.isClosed()).thenReturn(false); + when(mockConnection.isValid(Mockito.anyInt())).thenReturn(true); + when(mockConnectionPool.getConnection()).thenReturn(mockConnection); + + // The prepared statement path must go through Connection.prepareStatement(...), never createStatement(). + PreparedStatement mockPreparedStatement = mock(PreparedStatement.class); + when(mockConnection.prepareStatement(any())).thenReturn(mockPreparedStatement); + // A write-style statement: no result set, just an affected-row count. Keeps the mock minimal. + when(mockPreparedStatement.execute()).thenReturn(false); + when(mockPreparedStatement.getUpdateCount()).thenReturn(1); + doNothing().when(mockPreparedStatement).close(); + + ActionConfiguration actionConfiguration = new ActionConfiguration(); + // Binding is unquoted so it becomes a `?` placeholder (the safe prepared-statement form). + actionConfiguration.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + // Prepared statements enabled (this is also the default when the property is absent). + actionConfiguration.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + final String paramValue = "O'Reilly -- value"; + ExecuteActionDTO executeActionDTO = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue(paramValue); + param.setClientDataType(ClientDataType.STRING); + executeActionDTO.setParams(List.of(param)); + + DatasourceConfiguration dsConfig = createDatasourceConfiguration(); + Mono dsConnectionMono = Mono.just(mockConnectionPool); + + RedshiftPlugin.RedshiftPluginExecutor spyPluginExecutor = spy(new RedshiftPlugin.RedshiftPluginExecutor()); + doNothing().when(spyPluginExecutor).printConnectionPoolStatus(mockConnectionPool, false); + + Mono executeMono = dsConnectionMono.flatMap(connPool -> + spyPluginExecutor.executeParameterized(connPool, executeActionDTO, dsConfig, actionConfiguration)); + + StepVerifier.create(executeMono) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + + // The binding must have been replaced by a `?` placeholder in the prepared query... + verify(mockConnection).prepareStatement("SELECT * FROM users WHERE name = ?"); + // ...and the value must have been bound as a parameter, not concatenated into the SQL text. + verify(mockPreparedStatement).setString(eq(1), eq(paramValue)); + // The raw Statement path must never be taken when prepared statements are enabled. + Mockito.verify(mockConnection, Mockito.never()).createStatement(); + } + + // ------------------------------------------------------------------ + // Review-fix regression tests + // ------------------------------------------------------------------ + + /** Holder for a wired-up executeParameterized invocation so tests can inspect the mocks and the result Mono. */ + private static class ExecHarness { + Mono mono; + Connection connection; + PreparedStatement preparedStatement; + Statement statement; + } + + /** + * Wires a Redshift executor with both the prepared and raw JDBC paths mocked for a no-result-set (write) query, + * so a test can assert which path executeParameterized selects and how a single binding is bound. + */ + private ExecHarness buildExec(List pluginSpecifiedTemplates, ClientDataType clientDataType, String value) + throws SQLException { + HikariDataSource mockConnectionPool = mock(HikariDataSource.class); + when(mockConnectionPool.isClosed()).thenReturn(false); + when(mockConnectionPool.isRunning()).thenReturn(true); + + Connection mockConnection = mock(Connection.class); + when(mockConnection.isClosed()).thenReturn(false); + when(mockConnection.isValid(Mockito.anyInt())).thenReturn(true); + when(mockConnectionPool.getConnection()).thenReturn(mockConnection); + + PreparedStatement mockPreparedStatement = mock(PreparedStatement.class); + when(mockConnection.prepareStatement(any())).thenReturn(mockPreparedStatement); + when(mockPreparedStatement.execute()).thenReturn(false); + when(mockPreparedStatement.getUpdateCount()).thenReturn(0); + + Statement mockStatement = mock(Statement.class); + when(mockConnection.createStatement()).thenReturn(mockStatement); + when(mockStatement.execute(any())).thenReturn(false); + when(mockStatement.getUpdateCount()).thenReturn(0); + + ActionConfiguration actionConfiguration = new ActionConfiguration(); + actionConfiguration.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + actionConfiguration.setPluginSpecifiedTemplates(pluginSpecifiedTemplates); + + ExecuteActionDTO executeActionDTO = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue(value); + param.setClientDataType(clientDataType); + executeActionDTO.setParams(List.of(param)); + + DatasourceConfiguration dsConfig = createDatasourceConfiguration(); + + RedshiftPlugin.RedshiftPluginExecutor spyPluginExecutor = spy(new RedshiftPlugin.RedshiftPluginExecutor()); + doNothing().when(spyPluginExecutor).printConnectionPoolStatus(mockConnectionPool, false); + + ExecHarness harness = new ExecHarness(); + harness.connection = mockConnection; + harness.preparedStatement = mockPreparedStatement; + harness.statement = mockStatement; + harness.mono = spyPluginExecutor.executeParameterized( + mockConnectionPool, executeActionDTO, dsConfig, actionConfiguration); + return harness; + } + + private void assertPreparedPathTaken(List templates) throws SQLException { + ExecHarness harness = buildExec(templates, ClientDataType.STRING, "value"); + StepVerifier.create(harness.mono) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + verify(harness.connection).prepareStatement("SELECT * FROM users WHERE name = ?"); + Mockito.verify(harness.connection, Mockito.never()).createStatement(); + } + + private void assertRawPathTaken(List templates) throws SQLException { + ExecHarness harness = buildExec(templates, ClientDataType.STRING, "value"); + StepVerifier.create(harness.mono) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + verify(harness.connection).createStatement(); + Mockito.verify(harness.connection, Mockito.never()).prepareStatement(any()); + } + + /** + * The prepared-statement setting must default to ON whenever it is absent or malformed, so that saved actions + * without a populated setting are protected. Covers: missing list, empty list, present-but-null element, + * string "true" and boolean true. + */ + @Test + public void testPreparedStatementDefaultsOnWhenSettingAbsentOrMalformed() throws SQLException { + Property boolTrue = new Property(); + boolTrue.setKey("preparedStatement"); + boolTrue.setValue(Boolean.TRUE); + + assertPreparedPathTaken(null); // pluginSpecifiedTemplates missing entirely + assertPreparedPathTaken(new ArrayList<>()); // non-null but empty list (would previously throw IOOBE) + assertPreparedPathTaken(Arrays.asList((Property) null)); // index 0 present but null + assertPreparedPathTaken(List.of(new Property("preparedStatement", "true"))); // string "true" + assertPreparedPathTaken(List.of(boolTrue)); // boolean true + assertPreparedPathTaken(List.of(new Property("preparedStatement", "banana"))); // malformed non-boolean string + } + + /** Raw (non-prepared) execution is used only when the user explicitly disables prepared statements. */ + @Test + public void testRawStatementOnlyWhenExplicitlyDisabled() throws SQLException { + Property boolFalse = new Property(); + boolFalse.setKey("preparedStatement"); + boolFalse.setValue(Boolean.FALSE); + + assertRawPathTaken(List.of(new Property("preparedStatement", "false"))); // string "false" + assertRawPathTaken(List.of(boolFalse)); // boolean false + } + + /** Array-typed params cannot be bound as scalar JDBC parameters and must fail with a clear message. */ + @Test + public void testExecuteParameterizedRejectsArrayParams() throws SQLException { + ExecHarness harness = + buildExec(List.of(new Property("preparedStatement", "true")), ClientDataType.ARRAY, "[1, 2, 3]"); + // Redshift converts execution errors into a failed result (isExecutionSuccess=false) with errorInfo set. + StepVerifier.create(harness.mono) + .assertNext(result -> { + assertFalse(result.getIsExecutionSuccess()); + assertTrue(String.valueOf(result.getBody()) + .contains(String.format( + RedshiftErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, "Input1.text"))); + }) + .verifyComplete(); + Mockito.verify(harness.connection, Mockito.never()).createStatement(); + } + + /** A null param is bound via setNull rather than being interpolated as the string "null". */ + @Test + public void testExecuteParameterizedBindsNull() throws SQLException { + ExecHarness harness = + buildExec(List.of(new Property("preparedStatement", "true")), ClientDataType.NULL, "null"); + StepVerifier.create(harness.mono) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + verify(harness.preparedStatement).setNull(eq(1), Mockito.anyInt()); + } + + /** + * When the driver rejects a bound value, a clear preparation error is surfaced and the prepared statement is + * still closed (no resource leak). + */ + @Test + public void testExecuteParameterizedBindFailureSurfacesErrorAndClosesStatement() throws SQLException { + ExecHarness harness = + buildExec(List.of(new Property("preparedStatement", "true")), ClientDataType.NUMBER, "123"); + Mockito.doThrow(new SQLException("driver rejected value")) + .when(harness.preparedStatement) + .setInt(Mockito.anyInt(), Mockito.anyInt()); + + StepVerifier.create(harness.mono) + .assertNext(result -> { + assertFalse(result.getIsExecutionSuccess()); + // The underlying driver error is surfaced to the user rather than being swallowed. + assertTrue(String.valueOf(result.getBody()).contains("driver rejected value")); + }) + .verifyComplete(); + verify(harness.preparedStatement).close(); + } + + /** The plugin ships a dependency.json so the client re-evaluates the query body when the toggle flips. */ + @Test + public void testDependencyConfigLinksBodyToPreparedStatementToggle() throws IOException { + InputStream input = new ClassPathResource("dependency.json").getInputStream(); + String content = new BufferedReader(new InputStreamReader(input)) + .lines() + .collect(Collectors.joining(System.lineSeparator())); + assertTrue(content.contains("actionConfiguration.body")); + assertTrue(content.contains("actionConfiguration.pluginSpecifiedTemplates[0].value")); + } } diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java index d205dc506a74..7d01c857287f 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java @@ -1,8 +1,13 @@ package com.external.plugins; +import com.appsmith.external.constants.DataType; +import com.appsmith.external.datatypes.AppsmithType; +import com.appsmith.external.dtos.ExecuteActionDTO; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; +import com.appsmith.external.helpers.DataTypeServiceUtils; +import com.appsmith.external.helpers.MustacheHelper; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionRequest; import com.appsmith.external.models.ActionExecutionResult; @@ -12,8 +17,14 @@ import com.appsmith.external.models.DatasourceStructure; import com.appsmith.external.models.DatasourceTestResult; import com.appsmith.external.models.KeyPairAuth; +import com.appsmith.external.models.MustacheBindingToken; +import com.appsmith.external.models.Param; +import com.appsmith.external.models.Property; +import com.appsmith.external.models.PsParameterDTO; +import com.appsmith.external.models.RequestParamDTO; import com.appsmith.external.plugins.BasePlugin; import com.appsmith.external.plugins.PluginExecutor; +import com.appsmith.external.plugins.SmartSubstitutionInterface; import com.external.plugins.exceptions.SnowflakeErrorMessages; import com.external.utils.SnowflakeKeyUtils; import com.external.utils.SqlUtils; @@ -23,6 +34,7 @@ import com.zaxxer.hikari.pool.HikariPool; import lombok.extern.slf4j.Slf4j; import net.snowflake.client.jdbc.SnowflakeBasicDataSource; +import org.apache.commons.io.IOUtils; import org.bouncycastle.pkcs.PKCSException; import org.pf4j.Extension; import org.pf4j.PluginWrapper; @@ -31,16 +43,25 @@ import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; +import java.math.BigDecimal; import java.security.PrivateKey; import java.sql.*; import java.util.*; +import java.util.AbstractMap.SimpleEntry; +import java.util.stream.IntStream; +import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY; import static com.appsmith.external.constants.Authentication.DB_AUTH; import static com.appsmith.external.constants.Authentication.SNOWFLAKE_KEY_PAIR_AUTH; import static com.appsmith.external.constants.PluginConstants.PluginName.SNOWFLAKE_PLUGIN_NAME; +import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel; +import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex; +import static com.external.utils.ExecutionUtils.getRowsFromPreparedStatement; import static com.external.utils.ExecutionUtils.getRowsFromQueryResult; import static com.external.utils.SnowflakeDatasourceUtils.getConnectionFromHikariConnectionPool; import static com.external.utils.ValidationUtils.validateWarehouseDatabaseSchema; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; @Slf4j public class SnowflakePlugin extends BasePlugin { @@ -61,17 +82,86 @@ public SnowflakePlugin(PluginWrapper wrapper) { } @Extension - public static class SnowflakePluginExecutor implements PluginExecutor { + public static class SnowflakePluginExecutor + implements PluginExecutor, SmartSubstitutionInterface { private final Scheduler scheduler = Schedulers.boundedElastic(); + // Index of the "Use prepared statements" toggle within actionConfiguration.pluginSpecifiedTemplates. + private static final int PREPARED_STATEMENT_INDEX = 0; + + /** + * Overriding the default executeParameterized so that dynamic bindings ({{...}}) can be bound as + * PreparedStatement parameters instead of being substituted into the SQL text. When the user disables + * prepared statements (to bind identifiers/fragments that a PreparedStatement cannot parameterize), we + * fall back to the default mustache substitution path, matching the Postgres/MySQL/MSSQL plugins. + */ + @Override + public Mono executeParameterized( + HikariDataSource connection, + ExecuteActionDTO executeActionDTO, + DatasourceConfiguration datasourceConfiguration, + ActionConfiguration actionConfiguration) { + + log.debug(Thread.currentThread().getName() + ": executeParameterized() called for Snowflake plugin."); + String query = actionConfiguration.getBody(); + + if (!StringUtils.hasLength(query)) { + return Mono.error(new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + SnowflakeErrorMessages.MISSING_QUERY_ERROR_MSG)); + } + + Boolean isPreparedStatement; + final List properties = actionConfiguration.getPluginSpecifiedTemplates(); + if (properties == null + || properties.size() <= PREPARED_STATEMENT_INDEX + || properties.get(PREPARED_STATEMENT_INDEX) == null) { + // In case the prepared statement configuration is missing, default to true (safe by default). + isPreparedStatement = true; + } else { + Object psValue = properties.get(PREPARED_STATEMENT_INDEX).getValue(); + if (psValue instanceof Boolean) { + isPreparedStatement = (Boolean) psValue; + } else if (psValue instanceof String) { + // Only an explicit "false" disables prepared statements; any other value keeps it enabled. + isPreparedStatement = !"false".equalsIgnoreCase(((String) psValue).trim()); + } else { + isPreparedStatement = true; + } + } + + // In case of non-prepared statement, simply do bind replacement and execute the raw statement. + if (FALSE.equals(isPreparedStatement)) { + prepareConfigurationsForExecution(executeActionDTO, actionConfiguration, datasourceConfiguration); + return executeCommon(connection, actionConfiguration, FALSE, null, null); + } + + // Prepared statement: replace every binding with a `?` and bind the values on the PreparedStatement. + List mustacheKeysInOrder = MustacheHelper.extractMustacheKeysInOrder(query); + String updatedQuery = MustacheHelper.replaceMustacheWithQuestionMark(query, mustacheKeysInOrder); + actionConfiguration.setBody(updatedQuery); + return executeCommon(connection, actionConfiguration, TRUE, mustacheKeysInOrder, executeActionDTO); + } + @Override public Mono execute( HikariDataSource connection, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration) { + // Raw (non-parameterized) execution path. Reached only when prepared statements are disabled, after + // the framework has already performed mustache substitution on the action configuration. + return executeCommon(connection, actionConfiguration, FALSE, null, null); + } + + private Mono executeCommon( + HikariDataSource connection, + ActionConfiguration actionConfiguration, + Boolean preparedStatement, + List mustacheValuesInOrder, + ExecuteActionDTO executeActionDTO) { - log.debug(Thread.currentThread().getName() + ": execute() called for Snowflake plugin."); + log.debug(Thread.currentThread().getName() + ": executeCommon() called for Snowflake plugin."); String query = actionConfiguration.getBody(); if (!StringUtils.hasLength(query)) { @@ -80,6 +170,14 @@ public Mono execute( SnowflakeErrorMessages.MISSING_QUERY_ERROR_MSG)); } + final Map requestData = new HashMap<>(); + requestData.put("preparedStatement", TRUE.equals(preparedStatement)); + Map psParams = TRUE.equals(preparedStatement) ? new LinkedHashMap<>() : null; + String transformedQuery = + TRUE.equals(preparedStatement) ? replaceQuestionMarkWithDollarIndex(query) : query; + List requestParams = + List.of(new RequestParamDTO(ACTION_CONFIGURATION_BODY, transformedQuery, null, null, psParams)); + return Mono.fromCallable(() -> { log.debug(Thread.currentThread().getName() + ": Execute Snowflake Query"); Connection connectionFromPool; @@ -114,13 +212,41 @@ public Mono execute( threadsAwaitingConnection, totalConnections)); + // Declared outside the try so the prepared statement is always closed in the finally, + // even if binding a parameter throws before execution. + PreparedStatement preparedQuery = null; try { - // Connection staleness is checked as part of this method call. - return getRowsFromQueryResult(connectionFromPool, query); + // Connection staleness is checked as part of these method calls. + if (FALSE.equals(preparedStatement)) { + return getRowsFromQueryResult(connectionFromPool, query); + } + + preparedQuery = connectionFromPool.prepareStatement(query); + List> parameters = new ArrayList<>(); + preparedQuery = (PreparedStatement) smartSubstitutionOfBindings( + preparedQuery, mustacheValuesInOrder, executeActionDTO.getParams(), parameters); + + requestData.put("ps-parameters", parameters); + IntStream.range(0, parameters.size()) + .forEachOrdered(i -> psParams.put( + getPSParamLabel(i + 1), + new PsParameterDTO( + parameters.get(i).getKey(), + parameters.get(i).getValue()))); + + return getRowsFromPreparedStatement(connectionFromPool, preparedQuery); } catch (AppsmithPluginException | StaleConnectionException e) { throw e; } finally { + if (preparedQuery != null) { + try { + preparedQuery.close(); + } catch (SQLException e) { + log.error("Execute Error closing Snowflake prepared statement", e); + } + } + idleConnections = poolProxy.getIdleConnections(); activeConnections = poolProxy.getActiveConnections(); totalConnections = poolProxy.getTotalConnections(); @@ -146,12 +272,113 @@ public Mono execute( result.setIsExecutionSuccess(true); ActionExecutionRequest request = new ActionExecutionRequest(); request.setQuery(query); + request.setProperties(requestData); + request.setRequestParams(requestParams); result.setRequest(request); return result; }) .subscribeOn(scheduler); } + @Override + public Object substituteValueInInput( + int index, + String binding, + String value, + Object input, + List> insertedParams, + Object... args) + throws AppsmithPluginException { + + PreparedStatement preparedStatement = (PreparedStatement) input; + Param param = (Param) args[0]; + AppsmithType appsmithType = DataTypeServiceUtils.getAppsmithType(param.getClientDataType(), value); + DataType valueType = appsmithType.type(); + + Map.Entry parameter = new SimpleEntry<>(value, valueType.toString()); + insertedParams.add(parameter); + + try { + switch (valueType) { + case NULL: { + preparedStatement.setNull(index, Types.NULL); + break; + } + case BINARY: { + preparedStatement.setBinaryStream(index, IOUtils.toInputStream(value)); + break; + } + case BYTES: { + preparedStatement.setBytes(index, value.getBytes("UTF-8")); + break; + } + case INTEGER: { + preparedStatement.setInt(index, Integer.parseInt(value)); + break; + } + case LONG: { + preparedStatement.setLong(index, Long.parseLong(value)); + break; + } + case FLOAT: + case DOUBLE: { + preparedStatement.setBigDecimal(index, new BigDecimal(String.valueOf(value))); + break; + } + case BOOLEAN: { + preparedStatement.setBoolean(index, Boolean.parseBoolean(value)); + break; + } + case DATE: { + // Fully qualified because this file wildcard-imports both java.sql.* and java.util.*. + preparedStatement.setDate(index, java.sql.Date.valueOf(value)); + break; + } + case TIME: { + preparedStatement.setTime(index, Time.valueOf(value)); + break; + } + case TIMESTAMP: { + preparedStatement.setTimestamp(index, Timestamp.valueOf(value)); + break; + } + case STRING: + case JSON_OBJECT: { + preparedStatement.setString(index, value); + break; + } + case ARRAY: + case NULL_ARRAY: + // Array-typed values cannot be bound as a scalar JDBC parameter. Fail fast with a clear + // message rather than leaving the placeholder unbound and hitting a confusing driver error. + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(SnowflakeErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, binding)); + default: + // An unrecognized data type would otherwise leave the placeholder unbound while it is + // still tracked as an inserted parameter, surfacing as a confusing driver error later. + log.warn( + "Unrecognized data type {} for binding {}; skipping parameter binding", + valueType, + binding); + break; + } + } catch (SQLException | IllegalArgumentException | java.io.IOException e) { + if ((e instanceof SQLException) + && e.getMessage() != null + && e.getMessage().contains("The column index is out of range:")) { + // The parameter is likely being set inside a commented-out part of the query. Ignore it. + } else { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(SnowflakeErrorMessages.QUERY_PREPARATION_FAILED_ERROR_MSG, value, binding), + e.getMessage()); + } + } + + return preparedStatement; + } + @Override public Mono createConnectionClient( DatasourceConfiguration datasourceConfiguration, Properties properties) { @@ -393,11 +620,14 @@ public Mono getStructure( AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, invalids.toArray()[0]); } Statement statement = connectionFromPool.createStatement(); - final String columnsQuery = SqlUtils.COLUMNS_QUERY + "'" - + datasourceConfiguration + // Schema name comes from datasource configuration (requires MANAGE_DATASOURCES). Escape + // single quotes so the value cannot break out of the string literal in the metadata query. + final String schemaValue = String.valueOf(datasourceConfiguration .getProperties() .get(2) - .getValue() + "'"; + .getValue()) + .replace("'", "''"); + final String columnsQuery = SqlUtils.COLUMNS_QUERY + "'" + schemaValue + "'"; ResultSet resultSet = statement.executeQuery(columnsQuery); while (resultSet.next()) { diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java index 06740d5d1c52..7a17ffb2fc72 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java @@ -11,6 +11,12 @@ public class SnowflakeErrorMessages extends BasePluginErrorMessages { public static final String QUERY_EXECUTION_FAILED_ERROR_MSG = "Your query failed to execute. Please check more information in the error details."; + public static final String QUERY_PREPARATION_FAILED_ERROR_MSG = + "Query preparation failed while inserting value: %s for binding: {{%s}}. Please check the query again."; + + public static final String ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG = + "ARRAY parameters are not supported by prepared statements for this plugin. Binding: {{%s}}."; + public static final String UNABLE_TO_CREATE_CONNECTION_ERROR_MSG = "Unable to create connection to Snowflake URL"; public static final String GET_STRUCTURE_ERROR_MSG = diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/ExecutionUtils.java b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/ExecutionUtils.java index 2d0ae7c396f3..db785acf7aeb 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/ExecutionUtils.java +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/ExecutionUtils.java @@ -8,6 +8,7 @@ import net.snowflake.client.jdbc.SnowflakeReauthenticationRequest; import java.sql.Connection; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; @@ -45,24 +46,67 @@ public static List> getRowsFromQueryResult(Connection connec statement = connection.createStatement(); resultSet = statement.executeQuery(query); - ResultSetMetaData metaData = resultSet.getMetaData(); - int colCount = metaData.getColumnCount(); - - while (resultSet.next()) { - // Use `LinkedHashMap` here so that the column ordering is preserved in the response. - Map row = new LinkedHashMap<>(colCount); + readResultSet(resultSet, rowsList); + } catch (SQLException e) { + if (e instanceof SnowflakeReauthenticationRequest) { + throw new StaleConnectionException(e.getMessage()); + } + log.error("Exception caught when executing Snowflake query. Cause: ", e); + throw new AppsmithPluginException( + SnowflakePluginError.QUERY_EXECUTION_FAILED, + SnowflakeErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, + e.getMessage(), + "SQLSTATE: " + e.getSQLState()); - for (int i = 1; i <= colCount; i++) { - Object value = resultSet.getObject(i); - row.put(metaData.getColumnName(i), value); + } finally { + if (resultSet != null) { + try { + resultSet.close(); + } catch (SQLException e) { + log.error("Unable to close Snowflake resultSet. Cause: ", e); } - rowsList.add(row); } + if (statement != null) { + try { + statement.close(); + } catch (SQLException e) { + log.error("Unable to close Snowflake statement. Cause: ", e); + } + } + } + + return rowsList; + } + + /** + * Execute an already-bound PreparedStatement and return the resulting table as a list of rows. This is the + * parameterized counterpart of {@link #getRowsFromQueryResult}: dynamic binding values are bound on the + * statement rather than substituted into the SQL text. + * + * @param connection - Connection object, used only to check connection validity. + * @param statement - PreparedStatement with all parameters already bound. + * @return List of rows from the response table. + */ + public static List> getRowsFromPreparedStatement( + Connection connection, PreparedStatement statement) + throws AppsmithPluginException, StaleConnectionException { + List> rowsList = new ArrayList<>(); + ResultSet resultSet = null; + try { + // We do not use keep alive threads for our connections since these might become expensive + // Instead for every execution, we check for connection validity, + // and reset the connection if required + if (!connection.isValid(30)) { + throw new StaleConnectionException(CONNECTION_INVALID_ERROR_MSG); + } + + resultSet = statement.executeQuery(); + readResultSet(resultSet, rowsList); } catch (SQLException e) { if (e instanceof SnowflakeReauthenticationRequest) { throw new StaleConnectionException(e.getMessage()); } - log.error("Exception caught when executing Snowflake query. Cause: ", e); + log.error("Exception caught when executing Snowflake prepared statement. Cause: ", e); throw new AppsmithPluginException( SnowflakePluginError.QUERY_EXECUTION_FAILED, SnowflakeErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, @@ -88,4 +132,20 @@ public static List> getRowsFromQueryResult(Connection connec return rowsList; } + + private static void readResultSet(ResultSet resultSet, List> rowsList) throws SQLException { + ResultSetMetaData metaData = resultSet.getMetaData(); + int colCount = metaData.getColumnCount(); + + while (resultSet.next()) { + // Use `LinkedHashMap` here so that the column ordering is preserved in the response. + Map row = new LinkedHashMap<>(colCount); + + for (int i = 1; i <= colCount; i++) { + Object value = resultSet.getObject(i); + row.put(metaData.getColumnName(i), value); + } + rowsList.add(row); + } + } } diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/dependency.json b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/dependency.json new file mode 100644 index 000000000000..5953a9c44360 --- /dev/null +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/dependency.json @@ -0,0 +1,7 @@ +{ + "dependencies": { + "actionConfiguration.body": [ + "actionConfiguration.pluginSpecifiedTemplates[0].value" + ] + } +} diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/editor.json b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/editor.json index d960202031f2..6c2341675528 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/editor.json +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/editor.json @@ -12,8 +12,28 @@ { "label": "", "internalLabel": "Query", + "propertyName": "query_prepared", "configProperty": "actionConfiguration.body", - "controlType": "QUERY_DYNAMIC_TEXT" + "controlType": "QUERY_DYNAMIC_TEXT", + "evaluationSubstitutionType": "PARAMETER", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": false + } + }, + { + "label": "", + "internalLabel": "Query", + "propertyName": "query_non_prepared", + "configProperty": "actionConfiguration.body", + "controlType": "QUERY_DYNAMIC_TEXT", + "evaluationSubstitutionType": "TEMPLATE", + "hidden": { + "path": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "comparison": "EQUALS", + "value": true + } } ] } diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/setting.json b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/setting.json new file mode 100644 index 000000000000..74bebf3b4fa0 --- /dev/null +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/setting.json @@ -0,0 +1,54 @@ +{ + "setting": [ + { + "sectionName": "", + "id": 1, + "children": [ + { + "label": "Run behavior", + "configProperty": "runBehaviour", + "controlType": "DROP_DOWN", + "initialValue": "MANUAL", + "options": [ + { + "label": "Automatic", + "subText": "Query runs on page load or when a variable it depends on changes", + "value": "AUTOMATIC" + }, + { + "label": "On page load", + "subText": "Query runs when the page loads or when manually triggered", + "value": "ON_PAGE_LOAD" + }, + { + "label": "Manual", + "subText": "Query only runs when called in an event or JS with .run()", + "value": "MANUAL" + } + ] + }, + { + "label": "Request confirmation before running this query", + "configProperty": "confirmBeforeExecute", + "controlType": "SWITCH", + "tooltipText": "Ask confirmation from the user each time before refreshing data" + }, + { + "label": "Use prepared statements", + "tooltipText": "Prepared statements prevent SQL injections on your queries but do not support dynamic bindings outside values in your SQL", + "configProperty": "actionConfiguration.pluginSpecifiedTemplates[0].value", + "controlType": "SWITCH_WITH_CONFIRMATION", + "initialValue": true, + "modalType": "PREPARED_STATEMENT" + }, + { + "label": "Query timeout (in milliseconds)", + "subtitle": "Maximum time after which the query will return", + "configProperty": "actionConfiguration.timeoutInMillisecond", + "controlType": "INPUT_TEXT", + "dataType": "NUMBER" + } + ] + } + ] +} diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java b/app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java index 6c1e21fce3ee..5ec245e5533a 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java +++ b/app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java @@ -1,6 +1,9 @@ package com.external.plugins; import com.appsmith.external.constants.Authentication; +import com.appsmith.external.datatypes.ClientDataType; +import com.appsmith.external.dtos.ExecuteActionDTO; +import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionResult; @@ -8,6 +11,7 @@ import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.DatasourceTestResult; import com.appsmith.external.models.KeyPairAuth; +import com.appsmith.external.models.Param; import com.appsmith.external.models.Property; import com.appsmith.external.models.UploadedFile; import com.external.plugins.exceptions.SnowflakeErrorMessages; @@ -34,6 +38,9 @@ import java.security.*; import java.security.PrivateKey; import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.Statement; import java.util.*; @@ -46,7 +53,9 @@ import static com.appsmith.external.constants.Authentication.SNOWFLAKE_KEY_PAIR_AUTH; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; @Slf4j @@ -402,4 +411,213 @@ public void testReadEncryptedPrivateKeyReturnsValidPrivateKey() throws Exception auth.getPrivateKey().getDecodedContent(), auth.getPassphrase()); assertInstanceOf(PrivateKey.class, privateKey); } + + // ------------------------------------------------------------------ + // Review-fix regression tests + // ------------------------------------------------------------------ + + /** Builds a mocked Hikari pool that hands out the given connection and reports harmless pool stats. */ + private HikariDataSource mockPool(Connection connection) throws SQLException { + HikariPoolMXBean mxbean = mock(HikariPoolMXBean.class); + when(mxbean.getActiveConnections()).thenReturn(1); + when(mxbean.getIdleConnections()).thenReturn(4); + when(mxbean.getTotalConnections()).thenReturn(5); + when(mxbean.getThreadsAwaitingConnection()).thenReturn(0); + + HikariDataSource pool = mock(HikariDataSource.class); + when(pool.isClosed()).thenReturn(false); + when(pool.isRunning()).thenReturn(true); + when(pool.getConnection()).thenReturn(connection); + when(pool.getHikariPoolMXBean()).thenReturn(mxbean); + return pool; + } + + /** + * Runs executeParameterized against a connection where BOTH the prepared and raw paths return an empty result + * set, so a test can assert which path was chosen for a given prepared-statement setting. + */ + private Connection runSnowflake(List templates) throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isValid(30)).thenReturn(true); + + ResultSet rs = mock(ResultSet.class); + ResultSetMetaData meta = mock(ResultSetMetaData.class); + when(rs.getMetaData()).thenReturn(meta); + when(meta.getColumnCount()).thenReturn(0); + when(rs.next()).thenReturn(false); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + when(ps.executeQuery()).thenReturn(rs); + + Statement st = mock(Statement.class); + when(connection.createStatement()).thenReturn(st); + when(st.executeQuery(any())).thenReturn(rs); + + HikariDataSource pool = mockPool(connection); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + ac.setPluginSpecifiedTemplates(templates); + + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue("value"); + param.setClientDataType(ClientDataType.STRING); + dto.setParams(List.of(param)); + + StepVerifier.create(pluginExecutor.executeParameterized(pool, dto, new DatasourceConfiguration(), ac)) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + return connection; + } + + /** The prepared path is taken (binding bound as `?`, value never substituted into the SQL text). */ + @Test + public void testExecuteParameterizedPreparedStatementBindsAndClosesStatement() throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isValid(30)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + ResultSet rs = mock(ResultSet.class); + when(ps.executeQuery()).thenReturn(rs); + ResultSetMetaData meta = mock(ResultSetMetaData.class); + when(rs.getMetaData()).thenReturn(meta); + when(meta.getColumnCount()).thenReturn(0); + when(rs.next()).thenReturn(false); + + HikariDataSource pool = mockPool(connection); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + ac.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + final String paramValue = "O'Reilly -- value"; + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue(paramValue); + param.setClientDataType(ClientDataType.STRING); + dto.setParams(List.of(param)); + + StepVerifier.create(pluginExecutor.executeParameterized(pool, dto, new DatasourceConfiguration(), ac)) + .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) + .verifyComplete(); + + verify(connection).prepareStatement("SELECT * FROM users WHERE name = ?"); + verify(ps).setString(eq(1), eq(paramValue)); + verify(connection, never()).createStatement(); + verify(ps, atLeastOnce()).close(); + } + + /** The prepared-statement setting defaults to ON when absent or malformed. */ + @Test + public void testPreparedStatementDefaultsOnWhenSettingAbsentOrMalformed() throws SQLException { + Property boolTrue = new Property(); + boolTrue.setKey("preparedStatement"); + boolTrue.setValue(Boolean.TRUE); + + for (List templates : Arrays.asList( + (List) null, + new ArrayList(), + Arrays.asList((Property) null), + List.of(new Property("preparedStatement", "true")), + List.of(new Property("preparedStatement", "banana")), + List.of(boolTrue))) { + Connection connection = runSnowflake(templates); + verify(connection).prepareStatement("SELECT * FROM users WHERE name = ?"); + verify(connection, never()).createStatement(); + } + } + + /** Raw execution is used only when the user explicitly disables prepared statements. */ + @Test + public void testRawStatementOnlyWhenExplicitlyDisabled() throws SQLException { + Property boolFalse = new Property(); + boolFalse.setKey("preparedStatement"); + boolFalse.setValue(Boolean.FALSE); + + for (List templates : + Arrays.asList(List.of(new Property("preparedStatement", "false")), List.of(boolFalse))) { + Connection connection = runSnowflake(templates); + verify(connection).createStatement(); + verify(connection, never()).prepareStatement(any()); + } + } + + /** When binding a parameter throws, the prepared statement must still be closed (no resource leak). */ + @Test + public void testExecuteParameterizedClosesPreparedStatementWhenBindingFails() throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isValid(30)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + doThrow(new SQLException("driver rejected value")).when(ps).setString(anyInt(), anyString()); + + HikariDataSource pool = mockPool(connection); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM users WHERE name = {{Input1.text}}"); + ac.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.text"); + param.setValue("x"); + param.setClientDataType(ClientDataType.STRING); + dto.setParams(List.of(param)); + + StepVerifier.create(pluginExecutor.executeParameterized(pool, dto, new DatasourceConfiguration(), ac)) + .expectError(AppsmithPluginException.class) + .verify(); + + verify(ps).close(); + } + + /** Array-typed params cannot be bound as scalar JDBC parameters and must fail with a clear message. */ + @Test + public void testExecuteParameterizedRejectsArrayParams() throws SQLException { + Connection connection = mock(Connection.class); + when(connection.isValid(30)).thenReturn(true); + + PreparedStatement ps = mock(PreparedStatement.class); + when(connection.prepareStatement(any())).thenReturn(ps); + + HikariDataSource pool = mockPool(connection); + + ActionConfiguration ac = new ActionConfiguration(); + ac.setBody("SELECT * FROM t WHERE id = {{Input1.ids}}"); + ac.setPluginSpecifiedTemplates(List.of(new Property("preparedStatement", "true"))); + + ExecuteActionDTO dto = new ExecuteActionDTO(); + Param param = new Param(); + param.setKey("Input1.ids"); + param.setValue("[1, 2, 3]"); + param.setClientDataType(ClientDataType.ARRAY); + dto.setParams(List.of(param)); + + StepVerifier.create(pluginExecutor.executeParameterized(pool, dto, new DatasourceConfiguration(), ac)) + .expectErrorMatches(e -> e instanceof AppsmithPluginException + && e.getMessage() + .contains(String.format( + SnowflakeErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, "Input1.ids"))) + .verify(); + + verify(connection, never()).createStatement(); + verify(ps).close(); + } + + /** The plugin ships a dependency.json so the client re-evaluates the query body when the toggle flips. */ + @Test + public void testDependencyConfigLinksBodyToPreparedStatementToggle() throws IOException { + InputStream input = new ClassPathResource("dependency.json").getInputStream(); + String content = new BufferedReader(new InputStreamReader(input)) + .lines() + .collect(Collectors.joining(System.lineSeparator())); + assertTrue(content.contains("actionConfiguration.body")); + assertTrue(content.contains("actionConfiguration.pluginSpecifiedTemplates[0].value")); + } }