fix: removed initialization fail timeout to show errors (#34875)
## Description When we create snowflake datasource with wrong credentials -> test configuration/query execution takes about 25 seconds to complete and then throws generic error message of `Unable to create connection to snowflake URL`. This error message does not provide any action items for user. The reason this generic error message gets thrown is because we had set `initializationFailTimeout` hikari config property to -1, which means it will create a datasource object but will not create connection to it. Only when we do testDatasource, we fetch the hikari datasource object using `getConnection` method. This method tries to establish connection to database and keeps trying until it is successful or timeout of 25 seconds is reached, that's why in our case hikari would throw interrupted during connection acquisition exception after 25 seconds and on our plugin code, we would wrap this up in a generic error message mentioned above. The fix is to remove the code where we are setting this initializationFailTimeout, the default value for this property is 1ms and what this is does is, it tries to establish connection with snowflake db within that 1ms and if it can't it throws PoolInitializationException along with error message sent by snowflake jdbc driver itself, thus we can use this message to show it in the UI. This initialization was set to -1 in [PR](https://github.com/appsmithorg/appsmith/pull/23270) which we had fixed for handling wrong credentials. ### Steps to test: 1. Create snowflake datasource with wrong credentials (In both basic and key pair auth) 2. Test the configuration 3. Save the datasources and execute a query on it (check the error message) 4. Test #16140 5. Test #22035 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Datasource, @tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9934985863> > Commit: 8e21675e47f31ebf846c04dbab40193b571f5273 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9934985863&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource, @tag.Sanity` > Spec: > <hr>Mon, 15 Jul 2024 07:27:31 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved connection initialization behavior in the Snowflake plugin, enhancing startup validations and reliability. - **Tests** - Updated and refined test cases for datasource, key pair authentication, and basic authentication in the Snowflake plugin to ensure robustness and accuracy. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
This commit is contained in:
sneha122
GitHub
parent
a5cb2fb642
commit
cf880ff045
|
|
@ -189,12 +189,6 @@ public class SnowflakePlugin extends BasePlugin {
|
|||
properties.setProperty("maximunPoolSize", String.valueOf(MAXIMUM_POOL_SIZE));
|
||||
properties.setProperty(
|
||||
SNOWFLAKE_DB_LOGIN_TIMEOUT_PROPERTY_KEY, String.valueOf(SNOWFLAKE_DB_LOGIN_TIMEOUT_VALUE_SEC));
|
||||
/**
|
||||
* Setting the value for setInitializationFailTimeout to -1 to
|
||||
* bypass any connection attempt and validation during startup
|
||||
* @see https://www.javadoc.io/doc/com.zaxxer/HikariCP/latest/com/zaxxer/hikari/HikariConfig.html
|
||||
*/
|
||||
properties.setProperty("initializationFailTimeout", String.valueOf(-1));
|
||||
properties.setProperty("connectionTimeoutMillis", String.valueOf(CONNECTION_TIMEOUT_MILLISECONDS));
|
||||
return properties;
|
||||
}
|
||||
|
|
@ -532,9 +526,6 @@ public class SnowflakePlugin extends BasePlugin {
|
|||
config.setMinimumIdle(Integer.parseInt(properties.get("minimumIdle").toString()));
|
||||
config.setMaximumPoolSize(
|
||||
Integer.parseInt(properties.get("maximunPoolSize").toString()));
|
||||
|
||||
config.setInitializationFailTimeout(
|
||||
Long.parseLong(properties.get("initializationFailTimeout").toString()));
|
||||
config.setConnectionTimeout(
|
||||
Long.parseLong(properties.get("connectionTimeoutMillis").toString()));
|
||||
return config;
|
||||
|
|
|
|||
|
|
@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.type.TypeFactory;
|
|||
import com.zaxxer.hikari.HikariDataSource;
|
||||
import com.zaxxer.hikari.HikariPoolMXBean;
|
||||
import lombok.extern.slf4j.Slf4j;
|
||||
import net.snowflake.client.jdbc.SnowflakeBasicDataSource;
|
||||
import net.snowflake.client.jdbc.SnowflakeReauthenticationRequest;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.mockito.MockedStatic;
|
||||
|
|
@ -186,9 +185,11 @@ public class SnowflakePluginTest {
|
|||
StepVerifier.create(pluginExecutor.testDatasource(datasourceConfiguration))
|
||||
.assertNext(datasourceTestResult -> {
|
||||
assertNotNull(datasourceTestResult);
|
||||
assertTrue(datasourceTestResult
|
||||
.getInvalids()
|
||||
.contains(SnowflakeErrorMessages.UNABLE_TO_CREATE_CONNECTION_ERROR_MSG));
|
||||
String expectedErrorMessage =
|
||||
"Certificate for <invalid.host.name.snowflakecomputing.com> doesn't match any of the subject alternative names";
|
||||
Set<String> invalids = datasourceTestResult.getInvalids();
|
||||
String errorMessage = invalids.iterator().next();
|
||||
assertTrue(errorMessage.contains(expectedErrorMessage));
|
||||
})
|
||||
.verifyComplete();
|
||||
}
|
||||
|
|
@ -351,15 +352,17 @@ public class SnowflakePluginTest {
|
|||
|
||||
Mono<HikariDataSource> datasourceCreateMono = pluginExecutor.datasourceCreate(datasourceConfiguration);
|
||||
|
||||
StepVerifier.create(datasourceCreateMono)
|
||||
.assertNext(dataSource -> {
|
||||
// These are null as these get set inside datasource object and not on HikariConfig itself
|
||||
// For key pair authentication, username and password should be null
|
||||
assertEquals(null, dataSource.getUsername());
|
||||
assertEquals(null, dataSource.getPassword());
|
||||
assertInstanceOf(SnowflakeBasicDataSource.class, dataSource.getDataSource());
|
||||
})
|
||||
.verifyComplete();
|
||||
StepVerifier.create(datasourceCreateMono).verifyErrorSatisfies(error -> {
|
||||
// This error is getting thrown because the account name we are sending in datasource config is not valid
|
||||
// snowflake account
|
||||
// but this ensures that hikariConfig is constructed correctly without any issues
|
||||
// and exception occurs only when we try to create hikari datasource from hikari config
|
||||
// Thus test still validates the creation of hikari config successfully
|
||||
assertTrue(error instanceof RuntimeException);
|
||||
String expectedErrorMessage =
|
||||
"Certificate for <invalid.host.name.snowflakecomputing.com> doesn't match any of the subject alternative names";
|
||||
assertTrue(error.getMessage().contains(expectedErrorMessage));
|
||||
});
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -378,14 +381,17 @@ public class SnowflakePluginTest {
|
|||
|
||||
Mono<HikariDataSource> datasourceCreateMono = pluginExecutor.datasourceCreate(datasourceConfiguration);
|
||||
|
||||
StepVerifier.create(datasourceCreateMono)
|
||||
.assertNext(dataSource -> {
|
||||
// Datasource object would be null in this case as it gets set only in case of key pair config
|
||||
assertEquals("test", dataSource.getUsername());
|
||||
assertEquals("test", dataSource.getPassword());
|
||||
assertEquals(null, dataSource.getDataSource());
|
||||
})
|
||||
.verifyComplete();
|
||||
StepVerifier.create(datasourceCreateMono).verifyErrorSatisfies(error -> {
|
||||
// This error is getting thrown because the account name we are sending in datasource config is not valid
|
||||
// snowflake account
|
||||
// but this ensures that hikariConfig is constructed correctly without any issues
|
||||
// and exception occurs only when we try to create hikari datasource from hikari config
|
||||
// Thus test still validates the creation of hikari config successfully
|
||||
assertTrue(error instanceof RuntimeException);
|
||||
String expectedErrorMessage =
|
||||
"Certificate for <invalid.host.name.snowflakecomputing.com> doesn't match any of the subject alternative names";
|
||||
assertTrue(error.getMessage().contains(expectedErrorMessage));
|
||||
});
|
||||
}
|
||||
|
||||
public void testReadEncryptedPrivateKeyReturnsValidPrivateKey() throws Exception {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user