From cf880ff045d94e3aaf4005938df973ae0a5db7a9 Mon Sep 17 00:00:00 2001 From: sneha122 Date: Mon, 15 Jul 2024 13:30:48 +0530 Subject: [PATCH] fix: removed initialization fail timeout to show errors (#34875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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" ### :mag: Cypress test results > [!TIP] > ๐ŸŸข ๐ŸŸข ๐ŸŸข All cypress tests have passed! ๐ŸŽ‰ ๐ŸŽ‰ ๐ŸŽ‰ > Workflow run: > Commit: 8e21675e47f31ebf846c04dbab40193b571f5273 > Cypress dashboard. > Tags: `@tag.Datasource, @tag.Sanity` > Spec: >
Mon, 15 Jul 2024 07:27:31 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## 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. --------- Co-authored-by: โ€œsneha122โ€ <โ€œsneha@appsmith.comโ€> --- .../com/external/plugins/SnowflakePlugin.java | 9 ---- .../external/plugins/SnowflakePluginTest.java | 48 +++++++++++-------- 2 files changed, 27 insertions(+), 30 deletions(-) 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 89e0e71ebe..e178c61df4 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 @@ -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; 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 4d95a880d0..6c1e21fce3 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 @@ -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 doesn't match any of the subject alternative names"; + Set invalids = datasourceTestResult.getInvalids(); + String errorMessage = invalids.iterator().next(); + assertTrue(errorMessage.contains(expectedErrorMessage)); }) .verifyComplete(); } @@ -351,15 +352,17 @@ public class SnowflakePluginTest { Mono 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 doesn't match any of the subject alternative names"; + assertTrue(error.getMessage().contains(expectedErrorMessage)); + }); } @Test @@ -378,14 +381,17 @@ public class SnowflakePluginTest { Mono 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 doesn't match any of the subject alternative names"; + assertTrue(error.getMessage().contains(expectedErrorMessage)); + }); } public void testReadEncryptedPrivateKeyReturnsValidPrivateKey() throws Exception {