Skip to content
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ public class MssqlPlugin extends BasePlugin {

private static final long MS_SQL_DEFAULT_PORT = 1433L;

private static final int DEFAULT_CONNECTION_TIMEOUT_SECONDS = 60;

public static final MssqlDatasourceUtils mssqlDatasourceUtils = new MssqlDatasourceUtils();

public MssqlPlugin(PluginWrapper wrapper) {
Expand Down Expand Up @@ -417,6 +419,11 @@ public Set<String> validateDatasource(@NonNull DatasourceConfiguration datasourc
}
}

int connectionTimeout = getConnectionTimeoutSeconds(datasourceConfiguration);
if (connectionTimeout < 0) {
invalids.add(MssqlErrorMessages.DS_INVALID_CONNECTION_TIMEOUT_ERROR_MSG);
}

return invalids;
}

Expand Down Expand Up @@ -626,6 +633,10 @@ private static HikariDataSource createConnectionPool(

addSslOptionsToUrlBuilder(datasourceConfiguration, urlBuilder);

int timeoutSeconds = getConnectionTimeoutSeconds(datasourceConfiguration);
urlBuilder.append("loginTimeout=").append(timeoutSeconds).append(";");
hikariConfig.setConnectionTimeout(timeoutSeconds * 1000L);

hikariConfig.setJdbcUrl(urlBuilder.toString());

try {
Expand All @@ -640,23 +651,39 @@ private static HikariDataSource createConnectionPool(
return hikariDatasource;
}

/**
* Extracts the connection timeout (in seconds) from the datasource properties.
* Returns the default timeout if no value is configured or the value is not a valid number.
*/
static int getConnectionTimeoutSeconds(DatasourceConfiguration datasourceConfiguration) {
List<Property> properties = datasourceConfiguration.getProperties();
if (properties != null && !properties.isEmpty()) {
Property timeoutProperty = properties.get(0);
if (timeoutProperty != null && timeoutProperty.getValue() != null) {
try {
return Integer.parseInt(
String.valueOf(timeoutProperty.getValue()).trim());
} catch (NumberFormatException e) {
// fall through to default
}
}
}
return DEFAULT_CONNECTION_TIMEOUT_SECONDS;
}
Comment on lines +658 to +672

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Hard-coded property index is fragile and error-prone.

The method assumes the connection timeout is always at properties.get(0), but this creates a brittle coupling to the form configuration. If properties are reordered or new properties are added, this will silently fail or read the wrong value.

Instead, search for the property by its key "connectionTimeout" to make the code resilient to configuration changes.

🔍 Proposed fix to search by key
 static int getConnectionTimeoutSeconds(DatasourceConfiguration datasourceConfiguration) {
     List<Property> properties = datasourceConfiguration.getProperties();
-    if (properties != null && !properties.isEmpty()) {
-        Property timeoutProperty = properties.get(0);
-        if (timeoutProperty != null && timeoutProperty.getValue() != null) {
+    if (properties != null) {
+        Property timeoutProperty = properties.stream()
+                .filter(p -> "connectionTimeout".equals(p.getKey()))
+                .findFirst()
+                .orElse(null);
+        if (timeoutProperty != null && timeoutProperty.getValue() != null) {
             try {
                 return Integer.parseInt(
                         String.valueOf(timeoutProperty.getValue()).trim());
             } catch (NumberFormatException e) {
                 // fall through to default
             }
         }
     }
     return DEFAULT_CONNECTION_TIMEOUT_SECONDS;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java`
around lines 658 - 672, The getConnectionTimeoutSeconds method currently reads
properties.get(0), which is fragile; change it to search the
datasourceConfiguration.getProperties() list for a Property whose key equals
"connectionTimeout" (use Property.getKey()), then if found and its getValue() is
non-null try to parse its trimmed string value to an int and return it,
otherwise fall back to DEFAULT_CONNECTION_TIMEOUT_SECONDS; keep the existing
NumberFormatException handling and default behavior.


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.
* - When SSL configuration is absent, default to disabling encryption so that
* connections to servers that do not require encryption work out of the box.
*/
if (datasourceConfiguration.getConnection() == null
|| datasourceConfiguration.getConnection().getSsl() == null
|| datasourceConfiguration.getConnection().getSsl().getAuthType() == null) {
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_ERROR,
"Appsmith server has failed to fetch SSL configuration from datasource configuration form. "
+ "Please reach out to Appsmith customer support to resolve this.");
urlBuilder.append("encrypt=false;");
return;
}

/*
* - By default, the driver configures SSL in the no verify mode.
*/
SSLDetails.AuthType sslAuthType =
datasourceConfiguration.getConnection().getSsl().getAuthType();
switch (sslAuthType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,7 @@ public class MssqlErrorMessages extends BasePluginErrorMessages {
public static final String DS_MISSING_USERNAME_ERROR_MSG = "Missing username for authentication.";

public static final String DS_MISSING_PASSWORD_ERROR_MSG = "Missing password for authentication.";

public static final String DS_INVALID_CONNECTION_TIMEOUT_ERROR_MSG =
"Connection timeout must be a non-negative number (in seconds).";
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,21 @@
"controlType": "INPUT_TEXT",
"placeholderText": "Database name",
"initialValue": "admin"
},
{
"label": "Connection timeout (seconds)",
"configProperty": "datasourceConfiguration.properties[0].key",
"initialValue": "connectionTimeout",
"hidden": true,
"controlType": "INPUT_TEXT"
},
{
"label": "Connection timeout (seconds)",
"configProperty": "datasourceConfiguration.properties[0].value",
"controlType": "INPUT_TEXT",
"dataType": "NUMBER",
"initialValue": "60",
"placeholderText": "60"
}
]
},
Expand Down Expand Up @@ -82,7 +97,7 @@
"label": "SSL mode",
"configProperty": "datasourceConfiguration.connection.ssl.authType",
"controlType": "DROP_DOWN",
"initialValue": "NO_VERIFY",
"initialValue": "DISABLE",
"options": [
{
"label": "Disable",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,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 @@ -773,4 +774,115 @@ public void testGetEndpointIdentifierForRateLimit_HostPresentPortAbsent_ReturnsC
})
.verifyComplete();
}

@Test
public void testSslDefaultsToDisable_whenConnectionIsNull() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setConnection(null);

Mono<HikariDataSource> dsConnectionMono = mssqlPluginExecutor.datasourceCreate(dsConfig);

StepVerifier.create(dsConnectionMono)
.assertNext(hikariDataSource -> {
assertNotNull(hikariDataSource);
assertTrue(hikariDataSource.getJdbcUrl().contains("encrypt=false"));
})
.verifyComplete();
}

@Test
public void testSslDefaultsToDisable_whenSslDetailsAreNull() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.getConnection().setSsl(null);

Mono<HikariDataSource> dsConnectionMono = mssqlPluginExecutor.datasourceCreate(dsConfig);

StepVerifier.create(dsConnectionMono)
.assertNext(hikariDataSource -> {
assertNotNull(hikariDataSource);
assertTrue(hikariDataSource.getJdbcUrl().contains("encrypt=false"));
})
.verifyComplete();
}

@Test
public void testSslDefaultsToDisable_whenAuthTypeIsNull() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.getConnection().getSsl().setAuthType(null);

Mono<HikariDataSource> dsConnectionMono = mssqlPluginExecutor.datasourceCreate(dsConfig);

StepVerifier.create(dsConnectionMono)
.assertNext(hikariDataSource -> {
assertNotNull(hikariDataSource);
assertTrue(hikariDataSource.getJdbcUrl().contains("encrypt=false"));
})
.verifyComplete();
}

@Test
public void testConnectionTimeout_defaultAppliedWhenNoPropertySet() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
// No properties set — default timeout should be used

Mono<HikariDataSource> dsConnectionMono = mssqlPluginExecutor.datasourceCreate(dsConfig);

StepVerifier.create(dsConnectionMono)
.assertNext(hikariDataSource -> {
assertNotNull(hikariDataSource);
String jdbcUrl = hikariDataSource.getJdbcUrl();
assertTrue(
jdbcUrl.contains("loginTimeout=60"),
"JDBC URL should contain default loginTimeout=60 but was: " + jdbcUrl);
assertEquals(
60000L,
hikariDataSource.getConnectionTimeout(),
"HikariCP connectionTimeout should be 60000ms");
})
.verifyComplete();
}

@Test
public void testConnectionTimeout_customValueApplied() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setProperties(List.of(new Property("connectionTimeout", "30")));

Mono<HikariDataSource> dsConnectionMono = mssqlPluginExecutor.datasourceCreate(dsConfig);

StepVerifier.create(dsConnectionMono)
.assertNext(hikariDataSource -> {
assertNotNull(hikariDataSource);
String jdbcUrl = hikariDataSource.getJdbcUrl();
assertTrue(
jdbcUrl.contains("loginTimeout=30"),
"JDBC URL should contain loginTimeout=30 but was: " + jdbcUrl);
assertEquals(
30000L,
hikariDataSource.getConnectionTimeout(),
"HikariCP connectionTimeout should be 30000ms");
})
.verifyComplete();
}

@Test
public void testConnectionTimeout_negativeValueFailsValidation() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setProperties(List.of(new Property("connectionTimeout", "-5")));

Set<String> invalids = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(
invalids.contains(MssqlErrorMessages.DS_INVALID_CONNECTION_TIMEOUT_ERROR_MSG),
"Validation should reject negative timeout values");
}

@Test
public void testConnectionTimeout_zeroValuePassesValidation() {
DatasourceConfiguration dsConfig = createDatasourceConfiguration(container);
dsConfig.setProperties(List.of(new Property("connectionTimeout", "0")));

Set<String> invalids = mssqlPluginExecutor.validateDatasource(dsConfig);
assertTrue(
!invalids.contains(MssqlErrorMessages.DS_INVALID_CONNECTION_TIMEOUT_ERROR_MSG),
"Validation should accept zero timeout (meaning no timeout)");
}
}