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

feat: add errors to wrong mssql configuration #34472

Open
wants to merge 13 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 @@ -28,6 +28,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 @@ -68,6 +69,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;
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 @@ -114,17 +118,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 @@ -135,7 +148,8 @@ public Mono<ActionExecutionResult> executeParameterized(
ActionConfiguration actionConfiguration) {

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 @@ -204,10 +218,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
// and should then trigger the destruction and recreation of the pool.
return Mono.error(
e instanceof StaleConnectionException
Expand Down Expand Up @@ -235,7 +252,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.", error);
Expand Down Expand Up @@ -363,30 +381,47 @@ public void datasourceDestroy(HikariDataSource connection) {
public Set<String> validateDatasource(@NonNull DatasourceConfiguration datasourceConfiguration) {
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);
}
}
saiprabhu-dandanayak marked this conversation as resolved.
Show resolved Hide resolved

private void validateEndpoints(DatasourceConfiguration datasourceConfiguration, Set<String> invalids) {
if (StringUtils.isEmpty(datasourceConfiguration.getUrl())
saiprabhu-dandanayak marked this conversation as resolved.
Show resolved Hide resolved
&& 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 @@ -490,10 +525,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 @@ -543,7 +582,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 @@ -560,8 +600,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 @@ -591,14 +633,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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiprabhu-dandanayak Can you please create error constants for such errors? instead of using magic strings in the code


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.",
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiprabhu-dandanayak Can we please check how we have defined error messages in case of postgres plugin and follow the same strategy here? In postgres we are also using something call PSQLState object for error codes, can we follow similar approach here?

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 @@ -607,7 +680,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
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiprabhu-dandanayak Can you please fix the indentation here? It is messing up the code and makes it difficult to understand the actual changes made

* 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 @@ -231,6 +233,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