feat: add errors to wrong mssql configuration #34472
Conversation
…g-mssql-configuration' into external-contri/Issue-#24726/feat-wrong-mssql-configuration $ the commit. Merge changes from contributor for issue appsmithorg#24726 regarding MSSQL configuration fix
…tion' of https://github.com/appsmithorg/appsmith into external-contri/Issue-#24726/feat-wrong-mssql-configuration
WalkthroughThe recent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MssqlPlugin
participant DriverManager
participant Database
Client->>MssqlPlugin: validateDatasource(config)
MssqlPlugin->>MssqlPlugin: validateConnectionMode(config)
MssqlPlugin->>MssqlPlugin: validateEndpoints(config)
MssqlPlugin->>MssqlPlugin: validateAuthentication(config)
alt Valid Configuration
MssqlPlugin->>DriverManager: getConnection()
DriverManager->>Database: Connect
Database->>DriverManager: Connection Success
DriverManager->>MssqlPlugin: Connection Success
else Invalid Configuration
MssqlPlugin->>Client: Return error message
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java (3 hunks)
- app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/exceptions/MssqlErrorMessages.java (1 hunks)
- app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (2 hunks)
Additional comments not posted (6)
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/exceptions/MssqlErrorMessages.java (1)
29-30: New error messages added for missing hostname and database nameThe addition of these error messages enhances the robustness of the error handling by providing more specific feedback when critical configuration details are missing.
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java (3)
52-52: Added import forjava.sql.DriverManagerThis import is necessary for establishing database connections directly, which is used later in the
createConnectionPoolmethod to validate the configuration. This is a good practice as it ensures that the connection parameters are correct before proceeding further.
367-371: New validation methods added for datasource configurationThese methods enhance the robustness of the datasource configuration by checking for null or invalid values in connection mode, endpoints, and authentication details. This is crucial for preventing runtime errors due to misconfiguration.
613-628: Connection validation increateConnectionPoolmethodThe direct use of
DriverManager.getConnectionto validate the connection is a robust way to ensure that the datasource is correctly configured before attempting to create a connection pool. This preemptive check helps in identifying issues early in the deployment cycle, reducing downtime and user frustration.app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (2)
12-19: Added imports for various models and exceptionsThese imports are necessary for the new tests that have been added, ensuring that all required classes and packages are available. This helps maintain the modularity and readability of the test code.
236-252: New tests added to validate datasource configurationsThese tests are crucial for ensuring that the datasource validation logic works as expected under various edge cases, such as null credentials, endpoints, and authentication details. This helps in catching potential issues early, improving the reliability of the application.
Also applies to: 253-260, 261-269, 271-278
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Show resolved
Hide resolved
saiprabhu-dandanayak
changed the title
saiprabhu-dandanayak
changed the title
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Hi @rohan-arthur |
|
Hi @Nikhil-Nandagopal , @rohan-arthur this pr is also pending from long if it is possible to prioritize |
|
@saiprabhu-dandanayak I have created a shadow PR for running some automated tests and checks.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
app/server/appsmith-plugins/jsPlugin/bin/src/main/java/com/external/plugins/JSPlugin$JSPluginExecutor.classis excluded by!**/*.classapp/server/appsmith-plugins/jsPlugin/bin/src/main/java/com/external/plugins/JSPlugin.classis excluded by!**/*.class
Files selected for processing (2)
- app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java (3 hunks)
- app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (3 hunks)
Additional comments not posted (10)
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java (5)
367-370: Ensure comprehensive validation.The integration of the new validation methods into
validateDatasourceis correct. Ensure that all potential error cases are covered.
373-378: Improve error message clarity.Consider providing more detailed error messages that specify which part of the configuration is missing or misconfigured.
- invalids.add(MssqlErrorMessages.DS_MISSING_CONNECTION_MODE_ERROR_MSG); + invalids.add("Connection mode is missing in the datasource configuration. Please ensure it is specified.");
380-391: Improve error message clarity.Consider providing more detailed error messages that specify which part of the configuration is missing or misconfigured.
- invalids.add(MssqlErrorMessages.DS_MISSING_ENDPOINT_ERROR_MSG); + invalids.add("Endpoint is missing in the datasource configuration. Please ensure it is specified.");
393-406: Improve error message clarity.Consider providing more detailed error messages that specify which part of the configuration is missing or misconfigured.
- invalids.add(MssqlErrorMessages.DS_MISSING_AUTHENTICATION_DETAILS_ERROR_MSG); + invalids.add("Authentication details are missing in the datasource configuration. Please ensure they are specified.");
613-628: Connection validation and error handling improvements are effective.The additions to validate the connection using
DriverManager.getConnection()and the improved error handling are effective. Ensure thorough testing of these changes.app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (5)
236-251: Test for null credentials is effective.The test method correctly validates the datasource and identifies missing credentials. Ensure that similar tests cover all potential error cases.
253-260: Test for null endpoint is effective.The test method correctly validates the datasource and identifies a missing endpoint. Ensure that similar tests cover all potential error cases.
262-269: Test for null host is effective.The test method correctly validates the datasource and identifies a missing host. Ensure that similar tests cover all potential error cases.
271-278: Test for null authentication details is effective.The test method correctly validates the datasource and identifies missing authentication details. Ensure that similar tests cover all potential error cases.
236-278: Comprehensive datasource validation tests.The added tests comprehensively cover various error scenarios for datasource validation. Ensure that any new potential error cases are also covered in the future.
Hi @rohan-arthur , fixed testcases by running Testcase screenshot
|
|
Hi @rishabhrathod01 , I have checked it twice , and i have attached a screenshot for the same , there was an error in mssqlpugin testcases due to missing imports statements , but I have fixed it , will recheck it again. |
|
@saiprabhu-dandanayak won't be required. That was a false alert by the initial CI run. All the tests have passed now. #34877 |
…smith into external-contri/Issue-#24726/feat-wrong-mssql-configuration
|
There was a problem hiding this comment.
@saiprabhu-dandanayak Why is it showing changes in these two files?
JSPlugin$JSPluginExecutor.class and JSPlugin.class? I don't think these changes should be part of the PR
Hi @sneha122 , deleted those files |
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Show resolved
Hide resolved
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Outdated
Show resolved
Hide resolved
| @@ -592,13 +610,27 @@ private static HikariDataSource createConnectionPool(DatasourceConfiguration dat | |||
|
|
|||
| hikariConfig.setJdbcUrl(urlBuilder.toString()); | |||
|
|
|||
| try { | |||
| // Try to establish a connection to validate the configuration | |||
| DriverManager.getConnection( | |||
There was a problem hiding this comment.
Why do we have to explicitly do establish connection using DriverManager? When we create HikariDataSource it should automatically send us valid error message with PoolInitializationException, is it not happening now?
There was a problem hiding this comment.
If we are not explicitly establishing connection using DriverManager , when i give wrong host name like prabhu and invalid port like 3434 we are not getting any error.
when invalid host address is given
when invalid host address and port no are given
There was a problem hiding this comment.
@saiprabhu-dandanayak As we can see in above screenshot, we do get errors when no port and invalid host and port is entered, I am not understanding why establishing connection with driver manager is required. If you check the snowflake connection code here, This also behaves in the similar manner, when we create a new datasource using HikariDataSource constructor. It tries establishing connection with underlying database, if it fails it throws an exception called PoolInitializationException with error message from the driver. We can simply then show this error message in the UI, we wont have to establish connection using driverManager, I hope this makes sense. Please check out the snowflake plugin code as well for reference
There was a problem hiding this comment.
ok , tq for your feedback.
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
Outdated
Show resolved
Hide resolved
|
Hello @saiprabhu-dandanayak Any updates on the code review comments? I am still seeing changes in pom.xml |
|
Hi @sneha122 , i have removed explicit |
Can you please check the pom.xml once? I am still seeing changes in it. Can you please check once what formatter you are using? I am seeing changes to existing code as well where its getting cut off to next line, I believe you may have a line limit formatter on which is causing longer lines to get cut into multiple lines, we don't want to do that. |
NilanshBansal
added
Integrations Pod General
|
@saiprabhu-dandanayak any updates on this? We haven't heard back from you since the last 2 weeks. |
…smith into external-contri/Issue-#24726/feat-wrong-mssql-configuration
| SQLServerException sqlException = (SQLServerException) cause; | ||
| String sqlState = sqlException.getSQLState(); | ||
|
|
||
| if ("08S01".equals(sqlState)) { |
There was a problem hiding this comment.
@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, | ||
| "Could not connect to server. Check host and port.", |
There was a problem hiding this comment.
@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?
| @@ -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 | |||
There was a problem hiding this comment.
@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
|
Hi @saiprabhu-dandanayak, Any updates on this? I have created shadow PR here for running test cases: #36603 |
Description
Fixes #24726
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests