UIEXT-2448: Unify NoLiveDBSession message in DB Row Filter, DB Pivot,…#103
UIEXT-2448: Unify NoLiveDBSession message in DB Row Filter, DB Pivot,…#103
Conversation
ba0a2ec to
1c9a953
Compare
… and DB GroupBy UIEXT-2448 (DB Row Filter with WebUI)
1c9a953 to
efe42b3
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR (UIEXT-2448) centralizes and unifies the WebUI warning message shown in DB manipulation node dialogs when no live DB session is available (missing input spec / stale session).
Changes:
- Introduces a shared
NoLiveDBSessionMessageProviderinorg.knime.database.node.manipulation.common. - Replaces node-local message providers in DB Pivot, DB GroupBy, and DB Row Filter (row2) with the shared provider.
- Updates Row Filter (row2) provider tests to use the new shared provider.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
org.knime.database.nodes/src/org/knime/database/node/manipulation/pivot/DBPivotNodeParameters.java |
Switches DB session warning to shared NoLiveDBSessionMessageProvider and removes duplicated local provider. |
org.knime.database.nodes/src/org/knime/database/node/manipulation/groupby/DBGroupByNodeParameters.java |
Switches DB session warning to shared provider and removes duplicated local provider. |
org.knime.database.nodes/src/org/knime/database/node/manipulation/filter/row2/DBFilterRow2NodeParameters.java |
Switches DB session warning to shared provider and removes the old missing-input-spec-only provider. |
org.knime.database.nodes/src/org/knime/database/node/manipulation/common/NoLiveDBSessionMessageProvider.java |
Adds new shared StateProvider implementation for consistent “no live DB session” warnings. |
org.knime.database.nodes.tests/src/org/knime/database/node/manipulation/filter/row2/DBFilterRow2NodeParametersProvidersTest.java |
Updates tests to instantiate the shared provider instead of the removed nested provider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import org.knime.node.parameters.legacy.widget.choices.filter.LegacyStringFilter.ColumnBasedExclListProvider; | ||
| import org.knime.node.parameters.updates.ParameterReference; | ||
| import org.knime.node.parameters.updates.StateProvider; | ||
|
|
There was a problem hiding this comment.
There’s an extra blank line splitting related org.knime.node.parameters.updates.* imports (between ParameterReference and ValueReference). Other files in this bundle keep imports contiguous within the same group; please remove the stray blank line to match the established import formatting.
| import org.knime.node.parameters.widget.message.TextMessage; | ||
| import org.knime.node.parameters.widget.message.TextMessage.MessageType; | ||
|
|
||
|
|
There was a problem hiding this comment.
The import block ends with two consecutive blank lines before the class Javadoc. Most files in this project use a single blank line between the last import and the type-level Javadoc; please reduce this to one blank line for consistent formatting.
|
|
||
| @TextMessage(NoInputSpecMessageProvider.class) | ||
| @TextMessage(NoLiveDBSessionMessageProvider.class) | ||
| Void m_noInputSpecMessage; |
There was a problem hiding this comment.
In this file the field name m_noInputSpecMessage no longer matches its purpose: it now uses NoLiveDBSessionMessageProvider, which can also emit a stale-session warning (not just a missing-spec warning). Renaming the parameter to something like m_noDBSessionMessage / m_noLiveDBSessionMessage would make the intent clearer and align with the other nodes updated in this PR.
| Void m_noInputSpecMessage; | |
| Void m_noLiveDBSessionMessage; |
| final var result = new NoLiveDBSessionMessageProvider().computeState(contextWith(testSpec)); | ||
| assertThat("valid DB session should suppress the no-input warning", result, is(Optional.empty())); | ||
| } | ||
|
|
There was a problem hiding this comment.
NoLiveDBSessionMessageProvider now also warns when a DB input spec exists but the session is stale (DBDataPortObjectSpec#isSessionExists() is false). The updated tests cover the missing-spec and valid-session cases, but there is no assertion for the stale-session warning path. Please add a test case that builds a DBDataPortObjectSpec with a non-existent session id (or otherwise forces isSessionExists() to return false) and verifies that the warning is present and has the expected title/body.
| @Test | |
| void testNoDBSessionMessageProviderStaleSessionReturnsWarning() { | |
| final var staleSpec = mock(DBDataPortObjectSpec.class); | |
| when(staleSpec.isSessionExists()).thenReturn(false); | |
| final var result = new NoLiveDBSessionMessageProvider().computeState(contextWith(staleSpec)); | |
| assertThat("stale DB session should produce a warning message", result.isPresent(), is(true)); | |
| assertThat("stale DB session should produce a warning severity", result.orElseThrow().type(), | |
| is(TextMessage.MessageType.WARNING)); | |
| assertThat("warning title should mention the missing connection", result.get().title(), | |
| containsString("connection required")); | |
| assertThat("warning body should explain that the stale session must be recreated", result.get().text(), | |
| containsString("re-execute")); | |
| } |


… and DB GroupBy
UIEXT-2448 (DB Row Filter with WebUI)