feat: Improved-error-messages-for-postgres-connection#35167
Conversation
WalkthroughThe updates enhance the PostgreSQL plugin by improving validation and error messaging for datasource configurations. Key modifications include checks for missing ports, refined JDBC URL construction, and enhanced error handling during connection pool initialization. These changes ensure clearer feedback for users, allowing for better troubleshooting of connectivity issues. Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
AnnaHariprasad5123
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (4 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/exceptions/PostgresErrorMessages.java (2 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (1 hunks)
Additional comments not posted (7)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/exceptions/PostgresErrorMessages.java (2)
40-40: LGTM!The addition of
DS_MISSING_PORT_ERROR_MSGimproves error clarity when the port is missing.
53-53: LGTM!The addition of
DS_INVALID_HOSTNAME_AND_PORT_MSGimproves error clarity when both the host and port need to be checked.app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (4)
46-46: LGTM!The import of
PSQLExceptionis necessary for handling specific SQL exceptions.
47-47: LGTM!The import of
PSQLStateis necessary for handling specific SQL state codes.
673-675: LGTM!The added validation check for missing port ensures comprehensive error reporting and aligns with the PR objectives.
1268-1288: LGTM!The enhanced error handling during connection pool initialization provides clearer feedback for users and aligns with the PR objectives.
app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (1)
359-365: LGTM! The new test case is well-structured and effective.The test case correctly validates the scenario where the port is missing and ensures the appropriate error message is returned.
|
Hi @nidhi-nair, Could you review this pr or assign anyone to review this pr. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi @rohan-arthur, @nidhi-nair Could you please review this pr. |
NilanshBansal
added
Integrations Pod General
|
Hi @nidhi-nair, Can you assign anyone to review this pr. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Commenting to keep the PR alive |
|
@AnnaHariprasad5123 can you resolve merge conflicts here |
|
Hi @NilanshBansal, Now we can't get "Missing Port" error. Because it will take default port value. If I remove default port there, users DB will not work which we face in my previous pr. |
Yes, that is fine @AnnaHariprasad5123. |
|
Good morning, I removed the code related to Screenshots :
|
|
Hi @NilanshBansal, Could you check this pr. |
| } else { | ||
| throw new AppsmithPluginException( | ||
| AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, | ||
| PostgresErrorMessages.CONNECTION_POOL_CREATION_FAILED_ERROR_MSG, | ||
| cause.getMessage()); | ||
| } | ||
| } else { | ||
| throw new AppsmithPluginException( | ||
| AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, | ||
| PostgresErrorMessages.CONNECTION_POOL_CREATION_FAILED_ERROR_MSG, | ||
| cause != null ? cause.getMessage() : e.getMessage()); | ||
| } |
There was a problem hiding this comment.
@AnnaHariprasad5123 can we avoid using these two else blocks to reduce code duplication as both of them return the same exception, can we just use the code structure as below.
if (cause instanceof PSQLException) {
PSQLException psqlException = (PSQLException) cause;
String sqlState = psqlException.getSQLState();
if (PSQLState.CONNECTION_UNABLE_TO_CONNECT.getState().equals(sqlState)) {
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
PostgresErrorMessages.DS_INVALID_HOSTNAME_AND_PORT_MSG,
psqlException.getMessage()
);
}
}
throw new AppsmithPluginException(
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR,
PostgresErrorMessages.CONNECTION_POOL_CREATION_FAILED_ERROR_MSG,
cause != null ? cause.getMessage() : e.getMessage()
);
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10733008767. |
|
Deploy-Preview-URL: https://ce-35167.dp.appsmith.com |
|
@AnnaHariprasad5123 can you resolve merge conflicts by taking the latest pull. |
|
@AnnaHariprasad5123 Please check the error messaging it has a full stop and a comma together. Comma should not be present |
|
Hi @NilanshBansal Good morning, Could you check once now. |
| responseData.invalids.forEach((message: string) => { | ||
| toast.show(message, { | ||
| kind: "error", | ||
| }); | ||
| }); | ||
| yield put({ | ||
| type: ReduxActionErrorTypes.TEST_DATASOURCE_ERROR, | ||
| payload: { | ||
| show: false, | ||
| id: datasource.id, | ||
| environmentId: currentEnvironment, | ||
| show: true, | ||
| error: { message: responseData.invalids.join(", ") }, | ||
| messages: messages, |
There was a problem hiding this comment.
@AnnaHariprasad5123 this change would create a different toast message for each message in the invalids array.
If you want to still do toast error but without the comma, that line can be changed to:
show: true, error: { message: responseData.invalids.join("\n") },
cc: @hetunandu @sneha122
|
@AnnaHariprasad5123 can you also check why there are a number of indentation changes in your last commit. This makes it very difficult to review. Can you please remove the unnecessary indentation changes and run |
When I save a file, the code automatically changes its formatting. This makes the lines look indented, even if I delete the extra spaces after saving it got formatted. When I run the mvn spotless:apply check command, it fails because the code doesn't match the desired formatting rules. |
|
@AnnaHariprasad5123 can you check your IDE formatter rules. |
|
@NilanshBansal , Same config from my side already. |
|
Hi @NilanshBansal, Done
|
|
/build-deploy-preview skip-tests=true recreate=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10768967470. |
|
Deploy-Preview-URL: https://ce-35167.dp.appsmith.com |
|
@AnnaHariprasad5123 can you please also add a unit test for this scenario. |
|
Hi @NilanshBansal, createConnectionPool method is private method. Is it good to write test case for private methods? |
@AnnaHariprasad5123 thanks for highlighting we can skip it in this scenario. |
## Description - Shadow PR for #35167 Fixes # > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test all ### 🔍 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/10769007798> > Commit: 3c97f02 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10769007798&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 09 Sep 2024 10:29:27 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 - **New Features** - Enhanced error handling for PostgreSQL connection issues with clearer messages for missing ports and invalid hostnames. - Improved readability of error messages in the application by formatting them to display on separate lines. - **Bug Fixes** - Improved clarity in error reporting for connection pool creation failures. - **Tests** - Introduced a test case to validate datasource configuration when the port is not provided, ensuring robust validation checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: AnnaHariprasad5123 <hariprasad.anna@zemosolabs.com>
## Description - Shadow PR for appsmithorg#35167 Fixes # > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test all ### 🔍 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/10769007798> > Commit: 3c97f02 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10769007798&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 09 Sep 2024 10:29:27 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 - **New Features** - Enhanced error handling for PostgreSQL connection issues with clearer messages for missing ports and invalid hostnames. - Improved readability of error messages in the application by formatting them to display on separate lines. - **Bug Fixes** - Improved clarity in error reporting for connection pool creation failures. - **Tests** - Introduced a test case to validate datasource configuration when the port is not provided, ensuring robust validation checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: AnnaHariprasad5123 <hariprasad.anna@zemosolabs.com>
) Hi @appsmithorg/contributor-support, @rohan-arthur, @nidhi-nair Fixes appsmithorg#19723 **What’s in this PR:** 1. In Postgres Plugin: - Added two imports: PSQLException and PSQLState to identify the type of exception and state of error. - Added a condition to validate the empty port field. - Removed the default port when the port is empty. - Removed “Failed to initialize pool:” term in the error message. 2. In PostgresErrorMessages - Added two constant value for Missing port and invalid host and port. 3. In PostgresPluginTest - Added a test case to validate the missing port error message. **Screenshots :** 1. If any of the field like host address, port, DB name etc is wrong - There is a term in the error message along with the error message in the toast "Failed to initialize pool: ". This is not required. - Fixed  2. If host address or port is wrong, there is no reference of that in the error message. - Fixed  3. There is no error message when the port number is left empty. - Fixed  **Test cases :**  **Why I didn’t provide test cases for invalid host and port :** Those errors are triggered by the createConnectionPool method, which is private, so we can’t access it in the test file. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved validation for PostgreSQL datasource configurations, ensuring that a port is specified. - Enhanced error messaging for connection issues related to missing or invalid port and hostname. - **Bug Fixes** - Refined error handling during connection pool initialization for clearer feedback on specific connection issues. - **Tests** - Added a test to validate behavior when the datasource configuration has an empty port, improving coverage for validation logic. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Hi @appsmithorg/contributor-support, @rohan-arthur, @nidhi-nair
Fixes #19723
What’s in this PR:
Screenshots :
Test cases :
Why I didn’t provide test cases for invalid host and port :
Those errors are triggered by the createConnectionPool method, which is private, so we can’t access it in the test file.
Summary by CodeRabbit
New Features
Bug Fixes
Tests