feat: add MSSQL datasource configuration summary#41841
Closed
lupingblaine wants to merge 4 commits into
Closed
Conversation
Contributor
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughImplements MssqlPlugin.testDatasource returning DatasourceTestResult with a human-readable configuration summary (SSL/auth display lines), updates the plugin helpers and unit test to assert the additional messages, and changes the client saga to show server-provided datasource test messages in the success toast. ChangesMSSQL plugin datasource test & summary
Client datasource toast update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (1)
784-788: 💤 Low valueInconsistent indentation in assertion block.
The lines inside the
assertNextlambda have extra indentation. While this doesn't affect functionality, it's inconsistent with the indentation style used in other tests in this file (e.g., lines 142-146, 669-673).♻️ Align indentation with existing test style
StepVerifier.create(dsConnectionMono) .assertNext(hikariDataSource -> { - assertNotNull(hikariDataSource); - assertTrue(hikariDataSource.getJdbcUrl().contains("encrypt=false")); + assertNotNull(hikariDataSource); + assertTrue(hikariDataSource.getJdbcUrl().contains("encrypt=false")); }) .verifyComplete();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java` around lines 784 - 788, The assertion block inside the Reactor test's .assertNext lambda has extra indentation that differs from the file's standard; locate the chain using .assertNext(hikariDataSource -> { ... }) and .verifyComplete() and reformat the lines within the lambda (the assertNotNull and assertTrue calls referencing hikariDataSource and getJdbcUrl().contains("encrypt=false")) to match the surrounding test style (remove the extra indent so the two assertions align with other tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java`:
- Around line 776-789: The test fails because addSslOptionsToUrlBuilder(...)
throws when Connection.getSsl().getAuthType() is null; update
addSslOptionsToUrlBuilder (used by datasourceCreate(...)) to treat a null
authType as the disabled/default case (do not throw), and build the JDBC URL
with encrypt=false in that path so the test expecting "encrypt=false" passes;
also adjust the indentation of the assertions inside the assertNext(...) lambda
in MssqlPluginTest.testSslDefaultsToDisable_whenAuthTypeIsNull to align
assertNotNull(...) and assertTrue(...).
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java`:
- Around line 784-788: The assertion block inside the Reactor test's .assertNext
lambda has extra indentation that differs from the file's standard; locate the
chain using .assertNext(hikariDataSource -> { ... }) and .verifyComplete() and
reformat the lines within the lambda (the assertNotNull and assertTrue calls
referencing hikariDataSource and getJdbcUrl().contains("encrypt=false")) to
match the surrounding test style (remove the extra indent so the two assertions
align with other tests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41087161-9bea-49e2-999b-5521118f795d
📒 Files selected for processing (1)
app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
lupingblaine
changed the title
lupingblaine
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/ce/sagas/DatasourcesSagas.ts`:
- Around line 1007-1014: The toast currently passes successMessage (built via
createMessage and messages.join("\n")) as plain text to toast.show which
collapses newline characters; instead, change the call so the toast content
preserves line breaks — for example, map the messages (and
createMessage(DATASOURCE_VALID, payload.name)) into an array of React nodes and
interleave explicit line breaks (or split successMessage on "\n" and render
pieces with <br/>), then pass that ReactNode to toast.show; alternatively adjust
the toast text component (ToastBody/Text) to use CSS white-space:
pre-line/pre-wrap so newlines render—update the code paths in
DatasourcesSagas.ts around successMessage, createMessage, messages, and the
toast.show invocation accordingly.
In
`@app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java`:
- Around line 410-435: The summary builder (buildDatasourceConfigurationSummary)
defaults a null SSL authType to DISABLE but that code is dead because
datasourceCreate → createConnectionPool → addSslOptionsToUrlBuilder currently
throws when authType is null; to fix, make addSslOptionsToUrlBuilder tolerate a
null SSLDetails.AuthType by removing the throw and defaulting null to
SSLDetails.AuthType.DISABLE (e.g., use ObjectUtils.defaultIfNull or equivalent
inside addSslOptionsToUrlBuilder), so createConnectionPool / datasourceCreate no
longer fail on missing authType and the summary fallback remains consistent with
connection setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7b28434-0bc4-4b17-9181-11f53bd60abb
📒 Files selected for processing (3)
app/client/src/ce/sagas/DatasourcesSagas.tsapp/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.javaapp/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java
Comment on lines
+410
to
+435
| private Set<String> buildDatasourceConfigurationSummary(DatasourceConfiguration datasourceConfiguration) { | ||
| Set<String> summary = new LinkedHashSet<>(); | ||
| summary.add("Summary"); | ||
| summary.add("Database: MSSQL"); | ||
|
|
||
| if (!isEmpty(datasourceConfiguration.getEndpoints())) { | ||
| Endpoint endpoint = datasourceConfiguration.getEndpoints().get(0); | ||
| summary.add("Host: " + endpoint.getHost()); | ||
| summary.add("Port: " + getPort(endpoint)); | ||
| } | ||
|
|
||
| SSLDetails.AuthType sslAuthType = datasourceConfiguration.getConnection() != null | ||
| && datasourceConfiguration.getConnection().getSsl() != null | ||
| ? datasourceConfiguration.getConnection().getSsl().getAuthType() | ||
| : null; | ||
| SSLDetails.AuthType resolvedSslAuthType = | ||
| ObjectUtils.defaultIfNull(sslAuthType, SSLDetails.AuthType.DISABLE); | ||
| summary.add("SSL: " + getSslDisplayName(resolvedSslAuthType)); | ||
|
|
||
| DBAuth authentication = (DBAuth) datasourceConfiguration.getAuthentication(); | ||
| if (authentication != null && authentication.getAuthType() != null) { | ||
| summary.add("Authentication: " + getAuthenticationDisplayName(authentication.getAuthType())); | ||
| } | ||
|
|
||
| return summary; | ||
| } |
Contributor
There was a problem hiding this comment.
Dead code: ObjectUtils.defaultIfNull at line 426 is unreachable when authType is null.
The logic at lines 421–427 assumes sslAuthType can be null and defaults it to DISABLE for display purposes. However, buildDatasourceConfigurationSummary is only called on line 392 after datasourceCreate succeeds.
Since datasourceCreate → createConnectionPool (line 381) → addSslOptionsToUrlBuilder (line 699) throws when authType is null (lines 721–722), a null authType will cause datasource creation to fail before this method is ever invoked.
This creates two problems:
- Dead code: Line 426's fallback to
DISABLEcan never execute whenauthTypeis actually null. - Inconsistent behavior: The summary builder assumes null authType should display as "Disabled", but datasource creation rejects it with an exception.
Additionally, the PR description states this adds a "compatibility fallback for missing auth type," but the implementation at lines 721–722 does the opposite—it throws rather than falling back.
🔧 Recommended fix
Option 1 (if null authType should be supported): Remove the null check at line 722 and handle null authType in addSslOptionsToUrlBuilder by defaulting to DISABLE:
if (datasourceConfiguration.getConnection() == null
- || datasourceConfiguration.getConnection().getSsl() == null
- || datasourceConfiguration.getConnection().getSsl().getAuthType() == null) {
+ || datasourceConfiguration.getConnection().getSsl() == null) {
throw new AppsmithPluginException(...);
}
-SSLDetails.AuthType sslAuthType =
- datasourceConfiguration.getConnection().getSsl().getAuthType();
+SSLDetails.AuthType sslAuthType = ObjectUtils.defaultIfNull(
+ datasourceConfiguration.getConnection().getSsl().getAuthType(),
+ SSLDetails.AuthType.DISABLE);Option 2 (if null authType should be rejected): Remove the dead defaulting logic from buildDatasourceConfigurationSummary since it can never be reached:
-SSLDetails.AuthType sslAuthType = datasourceConfiguration.getConnection() != null
- && datasourceConfiguration.getConnection().getSsl() != null
- ? datasourceConfiguration.getConnection().getSsl().getAuthType()
- : null;
-SSLDetails.AuthType resolvedSslAuthType =
- ObjectUtils.defaultIfNull(sslAuthType, SSLDetails.AuthType.DISABLE);
+// Safe to call getAuthType() without null check because datasourceCreate would have thrown otherwise
+SSLDetails.AuthType resolvedSslAuthType =
+ datasourceConfiguration.getConnection().getSsl().getAuthType();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java`
around lines 410 - 435, The summary builder
(buildDatasourceConfigurationSummary) defaults a null SSL authType to DISABLE
but that code is dead because datasourceCreate → createConnectionPool →
addSslOptionsToUrlBuilder currently throws when authType is null; to fix, make
addSslOptionsToUrlBuilder tolerate a null SSLDetails.AuthType by removing the
throw and defaulting null to SSLDetails.AuthType.DISABLE (e.g., use
ObjectUtils.defaultIfNull or equivalent inside addSslOptionsToUrlBuilder), so
createConnectionPool / datasourceCreate no longer fail on missing authType and
the summary fallback remains consistent with connection setup.
|
This PR has been closed because of inactivity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a datasource configuration summary for successful MSSQL connection tests.
After a successful connection test, the success message now includes:
Why
This helps users confirm which datasource configuration was tested instead of only seeing a generic success message.
Testing
./build.sh -DskipTestsSummary by CodeRabbit
Release Notes
New Features
Improvements