Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TESTING EXTERNAL SCRIPT: external merge request from Contributor #36603

Open
wants to merge 14 commits into
base: release
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/server/appsmith-plugins/mssqlPlugin/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<name>mssqlPlugin</name>

<dependencies>

<dependency>
<groupId>com.microsoft.sqlserver</groupId>
<artifactId>mssql-jdbc</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.external.plugins.exceptions.MssqlPluginError;
import com.external.plugins.utils.MssqlDatasourceUtils;
import com.external.plugins.utils.MssqlExecuteUtils;
import com.microsoft.sqlserver.jdbc.SQLServerException;
import com.zaxxer.hikari.HikariConfig;
import com.zaxxer.hikari.HikariDataSource;
import com.zaxxer.hikari.pool.HikariPool;
Expand Down Expand Up @@ -69,6 +70,9 @@

import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY;
import static com.appsmith.external.constants.PluginConstants.PluginName.MSSQL_PLUGIN_NAME;
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;
Comment on lines +73 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid redundant imports of connection error messages.

You have imported CONNECTION_CLOSED_ERROR_MSG, CONNECTION_INVALID_ERROR_MSG, and CONNECTION_NULL_ERROR_MSG from both com.appsmith.external.exceptions.pluginExceptions.BasePluginErrorMessages and com.external.plugins.exceptions.MssqlErrorMessages. This redundancy could lead to confusion and potential conflicts in error messaging.

Consider consolidating these constants into a single source or ensuring you import them from only one class to maintain clarity and prevent redundancy.

import static com.appsmith.external.helpers.PluginUtils.getIdenticalColumns;
import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel;
import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex;
Expand Down Expand Up @@ -115,17 +119,26 @@ public static class MssqlPluginExecutor implements PluginExecutor<HikariDataSour
private static final int PREPARED_STATEMENT_INDEX = 0;

/**
* Instead of using the default executeParametrized provided by pluginExecutor, this implementation affords an opportunity
* to use PreparedStatement (if configured) which requires the variable substitution, etc. to happen in a particular format
* supported by PreparedStatement. In case of PreparedStatement turned off, the action and datasource configurations are
* Instead of using the default executeParametrized provided by pluginExecutor,
* this implementation affords an opportunity
* to use PreparedStatement (if configured) which requires the variable
* substitution, etc. to happen in a particular format
* supported by PreparedStatement. In case of PreparedStatement turned off, the
* action and datasource configurations are
* prepared (binding replacement) using PluginExecutor.variableSubstitution
*
* @param hikariDSConnection : This is the connection that is established to the data source. This connection is according
* @param hikariDSConnection : This is the connection that is established
* to the data source. This connection is
* according
* to the parameters in Datasource Configuration
* @param executeActionDTO : This is the data structure sent by the client during execute. This contains the params
* @param executeActionDTO : This is the data structure sent by the
* client during execute. This contains the
* params
* which would be used for substitution
* @param datasourceConfiguration : These are the configurations which have been used to create a Datasource from a Plugin
* @param actionConfiguration : These are the configurations which have been used to create an Action from a Datasource.
* @param datasourceConfiguration : These are the configurations which have been
* used to create a Datasource from a Plugin
* @param actionConfiguration : These are the configurations which have been
* used to create an Action from a Datasource.
* @return
*/
@Override
Expand All @@ -137,7 +150,8 @@ public Mono<ActionExecutionResult> executeParameterized(

log.debug(Thread.currentThread().getName() + ": executeParameterized() called for MSSQL plugin.");
String query = actionConfiguration.getBody();
// Check for query parameter before performing the probably expensive fetch connection from the pool op.
// Check for query parameter before performing the probably expensive fetch
// connection from the pool op.
if (!StringUtils.hasLength(query)) {
return Mono.error(new AppsmithPluginException(
AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, MssqlErrorMessages.MISSING_QUERY_ERROR_MSG));
Expand Down Expand Up @@ -208,10 +222,13 @@ public Mono<ActionExecutionResult> executeCommon(
sqlConnectionFromPool = mssqlDatasourceUtils.getConnectionFromHikariConnectionPool(
hikariDSConnection, MSSQL_PLUGIN_NAME);
} catch (SQLException | StaleConnectionException e) {
// The function can throw either StaleConnectionException or SQLException. The underlying
// The function can throw either StaleConnectionException or SQLException. The
// underlying
// hikari
// library throws SQLException in case the pool is closed or there is an issue initializing
// the connection pool which can also be translated in our world to StaleConnectionException
// library throws SQLException in case the pool is closed or there is an issue
// initializing
// the connection pool which can also be translated in our world to
// StaleConnectionException
Comment on lines +225 to +231
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance exception handling for better specificity.

Wrapping all SQLException instances that are not StaleConnectionException into a StaleConnectionException may obscure the original cause of the exception. This could make debugging more challenging.

Consider handling specific SQLException subtypes separately to preserve the original exception information. For instance, you might catch SQLTransientConnectionException for transient issues while allowing other exceptions to propagate with their original messages.

// and should then trigger the destruction and recreation of the pool.
return Mono.error(
e instanceof StaleConnectionException
Expand Down Expand Up @@ -239,7 +256,8 @@ public Mono<ActionExecutionResult> executeCommon(
}
}
} catch (SQLException error) {
// This exception is thrown only when the timeout to `isValid` is negative. Since, that's
// This exception is thrown only when the timeout to `isValid` is negative.
// Since, that's
// not the case,
// here, this should never happen.
log.error("Error checking validity of MsSQL connection.");
Expand Down Expand Up @@ -375,30 +393,47 @@ public Set<String> validateDatasource(@NonNull DatasourceConfiguration datasourc
log.debug(Thread.currentThread().getName() + ": validateDatasource() called for MSSQL plugin.");
Set<String> invalids = new HashSet<>();

if (isEmpty(datasourceConfiguration.getEndpoints())) {
invalids.add(MssqlErrorMessages.DS_MISSING_ENDPOINT_ERROR_MSG);
}
validateConnectionMode(datasourceConfiguration, invalids);
validateEndpoints(datasourceConfiguration, invalids);
validateAuthentication(datasourceConfiguration, invalids);
return invalids;
}

private void validateConnectionMode(DatasourceConfiguration datasourceConfiguration, Set<String> invalids) {
if (datasourceConfiguration.getConnection() != null
&& datasourceConfiguration.getConnection().getMode() == null) {
invalids.add(MssqlErrorMessages.DS_MISSING_CONNECTION_MODE_ERROR_MSG);
}
}
Comment on lines +402 to +407
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure null connection configurations are properly handled.

In validateConnectionMode, if datasourceConfiguration.getConnection() is null, no error is added to invalids. This might lead to scenarios where missing connection configurations go unnoticed.

Consider adding a null check and an appropriate error message:

if (datasourceConfiguration.getConnection() == null) {
    invalids.add(MssqlErrorMessages.DS_MISSING_CONNECTION_CONFIGURATION_ERROR_MSG);
    return;
}

This change will ensure that users are alerted when the connection configuration is missing entirely.


private void validateEndpoints(DatasourceConfiguration datasourceConfiguration, Set<String> invalids) {
if (StringUtils.isEmpty(datasourceConfiguration.getUrl())
&& isEmpty(datasourceConfiguration.getEndpoints())) {
invalids.add(MssqlErrorMessages.DS_MISSING_ENDPOINT_ERROR_MSG);
} else if (!isEmpty(datasourceConfiguration.getEndpoints())) {
for (final Endpoint endpoint : datasourceConfiguration.getEndpoints()) {
if (isBlank(endpoint.getHost())) {
invalids.add(MssqlErrorMessages.DS_MISSING_HOSTNAME_ERROR_MSG);
}
}
}
}

private void validateAuthentication(DatasourceConfiguration datasourceConfiguration, Set<String> invalids) {
DBAuth auth = (DBAuth) datasourceConfiguration.getAuthentication();
if (auth == null) {
invalids.add(MssqlErrorMessages.DS_MISSING_AUTHENTICATION_DETAILS_ERROR_MSG);

} else {
if (StringUtils.isEmpty(auth.getUsername())) {
if (isBlank(auth.getUsername())) {
invalids.add(MssqlErrorMessages.DS_MISSING_USERNAME_ERROR_MSG);
}

if (StringUtils.isEmpty(auth.getPassword())) {
if (isBlank(auth.getPassword())) {
invalids.add(MssqlErrorMessages.DS_MISSING_PASSWORD_ERROR_MSG);
}
if (isBlank(auth.getDatabaseName())) {
invalids.add(MssqlErrorMessages.DS_MISSING_DATABASE_NAME_ERROR_MSG);
}
}

return invalids;
}

@Override
Expand Down Expand Up @@ -503,10 +538,14 @@ public Object substituteValueInInput(
}

/**
* MsSQL plugin makes use of a common template that is available for SQL query which is also used for other SQL
* type plugins e.g. Postgres to create select, insert, update, delete, find queries for the CRUD page. In
* case of MsSQL the template select query needs to be replaced because its syntax does not match with MsSQL
* syntax. Hence, this method updates the template select query with the correct syntax select query for MsSQL.
* MsSQL plugin makes use of a common template that is available for SQL query
* which is also used for other SQL
* type plugins e.g. Postgres to create select, insert, update, delete, find
* queries for the CRUD page. In
* case of MsSQL the template select query needs to be replaced because its
* syntax does not match with MsSQL
* syntax. Hence, this method updates the template select query with the correct
* syntax select query for MsSQL.
*/
@Override
public Mono<Void> sanitizeGenerateCRUDPageTemplateInfo(
Expand Down Expand Up @@ -556,7 +595,8 @@ public static long getPort(Endpoint endpoint) {
}

/**
* This function is blocking in nature which connects to the database and creates a connection pool
* This function is blocking in nature which connects to the database and
* creates a connection pool
*
* @param datasourceConfiguration
* @return connection pool
Expand All @@ -573,8 +613,10 @@ private static HikariDataSource createConnectionPool(DatasourceConfiguration dat
hikariConfig.setDriverClassName(JDBC_DRIVER);
hikariConfig.setMinimumIdle(MINIMUM_POOL_SIZE);
hikariConfig.setMaximumPoolSize(MAXIMUM_POOL_SIZE);
// Configuring leak detection threshold for 60 seconds. Any connection which hasn't been released in 60 seconds
// should get tracked (may be falsely for long running queries) as leaked connection
// Configuring leak detection threshold for 60 seconds. Any connection which
// hasn't been released in 60 seconds
// should get tracked (may be falsely for long running queries) as leaked
// connection
hikariConfig.setLeakDetectionThreshold(LEAK_DETECTION_TIME_MS);

authentication = (DBAuth) datasourceConfiguration.getAuthentication();
Expand Down Expand Up @@ -604,14 +646,45 @@ private static HikariDataSource createConnectionPool(DatasourceConfiguration dat
addSslOptionsToUrlBuilder(datasourceConfiguration, urlBuilder);

hikariConfig.setJdbcUrl(urlBuilder.toString());

try {
hikariDatasource = new HikariDataSource(hikariConfig);
} catch (HikariPool.PoolInitializationException e) {
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
MssqlErrorMessages.CONNECTION_POOL_CREATION_FAILED_ERROR_MSG,
e.getMessage());
Throwable cause = e.getCause();
if (cause instanceof SQLServerException) {
SQLServerException sqlException = (SQLServerException) cause;
String sqlState = sqlException.getSQLState();

if ("08S01".equals(sqlState)) {

throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Invalid host or port",
sqlException.getMessage());
} else if ("08S02".equals(sqlState)) {

throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Could not connect to server. Check host and port.",
sqlException.getMessage());
} else if ("28000".equals(sqlState)) {

throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Invalid username or password",
sqlException.getMessage());
} else {

throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Connection pool creation failed: " + sqlState,
sqlException.getMessage());
}
} else {
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
"Connection pool creation failed",
cause != null ? cause.getMessage() : e.getMessage());
}
}

return hikariDatasource;
Expand All @@ -620,7 +693,8 @@ private static HikariDataSource createConnectionPool(DatasourceConfiguration dat
private static void addSslOptionsToUrlBuilder(
DatasourceConfiguration datasourceConfiguration, StringBuilder urlBuilder) throws AppsmithPluginException {
/*
* - Ideally, it is never expected to be null because the SSL dropdown is set to a initial value.
* - Ideally, it is never expected to be null because the SSL dropdown is set to
* a initial value.
*/
if (datasourceConfiguration.getConnection() == null
|| datasourceConfiguration.getConnection().getSsl() == null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ public class MssqlErrorMessages extends BasePluginErrorMessages {
************************************************************************************************************************************************
*/
public static final String DS_MISSING_ENDPOINT_ERROR_MSG = "Missing endpoint.";

public static final String DS_MISSING_HOSTNAME_ERROR_MSG = "Host value cannot be empty.";
public static final String DS_MISSING_DATABASE_NAME_ERROR_MSG = "Missing database name.";
public static final String DS_MISSING_CONNECTION_MODE_ERROR_MSG = "Missing connection mode.";

public static final String DS_MISSING_AUTHENTICATION_DETAILS_ERROR_MSG = "Missing authentication details.";

public static final String DS_MISSING_USERNAME_ERROR_MSG = "Missing username for authentication.";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.appsmith.external.models.ActionConfiguration;
import com.appsmith.external.models.ActionExecutionRequest;
import com.appsmith.external.models.ActionExecutionResult;
import com.appsmith.external.models.Connection;
import com.appsmith.external.models.DBAuth;
import com.appsmith.external.models.DatasourceConfiguration;
import com.appsmith.external.models.DatasourceTestResult;
Expand All @@ -15,6 +16,7 @@
import com.appsmith.external.models.PsParameterDTO;
import com.appsmith.external.models.RequestParamDTO;
import com.appsmith.external.models.SSLDetails;
import com.external.plugins.exceptions.MssqlErrorMessages;
import com.external.plugins.exceptions.MssqlPluginError;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand Down Expand Up @@ -233,6 +235,50 @@ public void invalidTestConnectMsSqlContainer() {
.verify();
}

@Test
void testValidateDatasource_NullCredentials_returnsWithInvalids() {

DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);

dsConfig.setConnection(new Connection());
DBAuth auth = (DBAuth) dsConfig.getAuthentication();
auth.setUsername(null);
auth.setPassword(null);
auth.setDatabaseName(null);

Set<String> expectedOutput = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(expectedOutput.contains(MssqlErrorMessages.DS_MISSING_USERNAME_ERROR_MSG));
assertTrue(expectedOutput.contains(MssqlErrorMessages.DS_MISSING_PASSWORD_ERROR_MSG));
assertTrue(expectedOutput.contains(MssqlErrorMessages.DS_MISSING_DATABASE_NAME_ERROR_MSG));
}

@Test
void testValidateDatasource_NullEndPoint() {

DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setEndpoints(null);
Set<String> output = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(output.contains(MssqlErrorMessages.DS_MISSING_ENDPOINT_ERROR_MSG));
}

@Test
void testValidateDatasource_NullHost() {

DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setEndpoints(List.of(new Endpoint("", 1433L)));
Set<String> output = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(output.contains(MssqlErrorMessages.DS_MISSING_HOSTNAME_ERROR_MSG));
}

@Test
void testValidateDatasource_NullAuthentication() {

DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setAuthentication(null);
Set<String> output = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(output.contains(MssqlErrorMessages.DS_MISSING_AUTHENTICATION_DETAILS_ERROR_MSG));
}

@Test
public void testPreparedStatementWithoutQuotes() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
Expand Down
Loading