chore: shadow PR for external contribution #ce-35167 #36053
Conversation
…3/feat-postgres-conn-error-msg-improvements' into chore/external-contri-ce-35167
WalkthroughThe changes enhance error handling and validation logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PostgresPlugin
participant DatasourceValidation
participant ErrorHandler
User->>PostgresPlugin: Configure Datasource
PostgresPlugin->>DatasourceValidation: Validate Configuration
DatasourceValidation->>PostgresPlugin: Check Port
alt Port is Empty
PostgresPlugin->>ErrorHandler: Add Missing Port Error
else Port is Valid
PostgresPlugin->>ErrorHandler: Proceed with Connection
end
Tip New featuresWalkthrough comment now includes:
Notes:
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
|
github-actions
bot
added
the
skip-changelog
NilanshBansal
added
ok-to-test
![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.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (3 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 (6)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/exceptions/PostgresErrorMessages.java (2)
40-40: Great job adding a clear error message for missing port!The new constant
DS_MISSING_PORT_ERROR_MSGis declared correctly and has an appropriate error message. This will provide clear feedback to users when the port is missing in the PostgreSQL connection configuration.
55-55: Nice work adding a helpful error message for invalid host and port!The new constant
DS_INVALID_HOSTNAME_AND_PORT_MSGis declared correctly and has an appropriate error message. This will guide users to check both the host and port values when encountering connection issues, improving the troubleshooting process.app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (3)
692-694: Great job adding the port validation check!The code change looks good. It adds a validation for missing port and provides a clear error message if the port is empty.
49-50: The new imports look good!The
PSQLExceptionandPSQLStateimports are valid and seem to be used later in the file for improved error handling.
1328-1348: Excellent work enhancing the error handling!The code changes improve error handling by inspecting the underlying cause of the
PoolInitializationException.If the cause is a
PSQLExceptionwith a SQL state indicating a connection failure, it throws a specific error message about invalid hostname/port. This can help in diagnosing connection issues more effectively.For other causes, it throws a generic pool creation failed error with the underlying cause message, which is still an improvement over the previous handling.
app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (1)
601-608: Great work on adding this test case!This test method effectively validates the scenario when the port is not provided in the datasource configuration. It sets up the test data correctly, invokes the validation method, and asserts the expected error message.
The test follows the AAA pattern and is easy to understand. Nice job!
… of github.com:AnnaHariprasad5123/appsmith into chore/external-contri-ce-35167
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (2 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/exceptions/PostgresErrorMessages.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java
Additional comments not posted (1)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/exceptions/PostgresErrorMessages.java (1)
53-53: Excellent addition to error messagesThe new error message
DS_INVALID_HOSTNAME_AND_PORT_MSGis a thoughtful addition, enhancing the clarity of feedback provided to users during PostgreSQL configuration errors. It's succinct and aligns well with the existing messages in this class. Well done on maintaining consistency and clarity in error messaging.
… of github.com:AnnaHariprasad5123/appsmith into chore/external-contri-ce-35167
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/sagas/DatasourcesSagas.ts (1 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (2 hunks)
Additional comments not posted (1)
app/client/src/sagas/DatasourcesSagas.ts (1)
932-932: Approved: Improved error message formatting.The change from a comma to a newline for separating error messages enhances readability, which is crucial for debugging and user feedback. Ensure that the UI correctly handles and displays these multiline error messages.
...erver/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java
Show resolved
Hide resolved
This reverts commit b782bde.
Reverts #36053 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved formatting of error messages for invalid data sources to enhance clarity. - Simplified error handling in the Postgres plugin, which may affect the specificity of connection error messages. - Removed an outdated error message related to invalid hostname and port configurations in the Postgres plugin, impacting how related errors are reported to users. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## 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 <[email protected]>
…thorg#36202) Reverts appsmithorg#36053 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved formatting of error messages for invalid data sources to enhance clarity. - Simplified error handling in the Postgres plugin, which may affect the specificity of connection error messages. - Removed an outdated error message related to invalid hostname and port configurations in the Postgres plugin, impacting how related errors are reported to users. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10769007798
Commit: 3c97f02
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 09 Sep 2024 10:29:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests