Skip to content

UIEXT-3487 WebUI-Migration Binary Objects to PNGs#44

Open
tcrundall-tng wants to merge 2 commits intomasterfrom
enh/UIEXT-3487-webui-migration-binary-objects-to-pngs
Open

UIEXT-3487 WebUI-Migration Binary Objects to PNGs#44
tcrundall-tng wants to merge 2 commits intomasterfrom
enh/UIEXT-3487-webui-migration-binary-objects-to-pngs

Conversation

@tcrundall-tng
Copy link

No description provided.

…oicesProvider

AP-23709 (Support username and password fields instead of flow variables in OAuth2 Authenticator nodes)
@tcrundall-tng tcrundall-tng requested a review from a team as a code owner March 25, 2026 11:20
@tcrundall-tng tcrundall-tng requested review from Copilot and knime-ghub-bot and removed request for a team March 25, 2026 11:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the “Binary Objects to PNGs” node to the WebUI parameter framework (including generated node description) and updates Box Authenticator settings to use the newer credentials widget/migration setup.

Changes:

  • Introduce BinaryObjectsToPNGsNodeParameters and migrate the node factory to WebUI dialogs + programmatic node description (removing the legacy XML description).
  • Add auto-guessing of the binary-object input column during configure() for “Binary Objects to PNGs”.
  • Update Box Authenticator app credentials settings to use LegacyCredentials with validation and flow-variable migration; bump plugin/test versions and update snapshots.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
org.knime.ext.box.authenticator/src-deprecated/org/knime/ext/box/authenticator/node/BoxAuthenticatorSettings.java Annotates nested app settings with @ValueReference to enable WebUI validation wiring.
org.knime.ext.box.authenticator/src-deprecated/org/knime/ext/box/authenticator/node/BoxAppSettings.java Switches from flow-variable string to LegacyCredentials + custom validation/migration.
org.knime.ext.box.authenticator/META-INF/MANIFEST.MF Bumps bundle version to 5.12.0.qualifier.
org.knime.ext.box.authenticator.tests/pom.xml Aligns test plugin revision to 5.12.0.
org.knime.ext.box.authenticator.tests/files/test_snapshots/org.knime.ext.box.authenticator.node.BoxAuthenticatorSettingsTest.snap Updates snapshot to new credential schema/ui format.
org.knime.ext.box.authenticator.tests/META-INF/MANIFEST.MF Bumps fragment version and host lower bound to 5.12.0.
org.knime.base.filehandling/src/org/knime/base/filehandling/binaryobjectstopngs/BinaryObjectsToPNGsNodeParameters.java Adds new WebUI NodeParameters model for the node (column, output mode, new column name).
org.knime.base.filehandling/src/org/knime/base/filehandling/binaryobjectstopngs/BinaryObjectsToPNGsNodeModel.java Adds column auto-guessing in configure() when selection is empty.
org.knime.base.filehandling/src/org/knime/base/filehandling/binaryobjectstopngs/BinaryObjectsToPNGsNodeFactory.xml Removes legacy XML node description in favor of programmatic description.
org.knime.base.filehandling/src/org/knime/base/filehandling/binaryobjectstopngs/BinaryObjectsToPNGsNodeFactory.java Migrates to DefaultNodeDialog/KAI and programmatic node description.
org.knime.base.filehandling.tests/src/org/knime/base/filehandling/binaryobjectstopngs/BinaryObjectsToPNGsNodeParametersTest.java Replaces legacy dialog test with snapshot tests for WebUI parameters/settings.
org.knime.base.filehandling.tests/files/test_snapshots/org.knime.base.filehandling.binaryobjectstopngs.BinaryObjectsToPNGsNodeParametersTest1.snap Adds JSON-forms snapshot for a populated settings instance.
org.knime.base.filehandling.tests/files/test_snapshots/org.knime.base.filehandling.binaryobjectstopngs.BinaryObjectsToPNGsNodeParametersTest0.settings.xml.snap Adds settings-XML structure snapshot for loaded settings.
org.knime.base.filehandling.tests/files/test_snapshots/org.knime.base.filehandling.binaryobjectstopngs.BinaryObjectsToPNGsNodeParametersTest.snap Adds JSON-forms snapshot for defaults.
org.knime.base.filehandling.tests/files/node_settings/BinaryObjectsToPNGsNodeParameters.xml Adds a representative node settings fixture used by snapshots.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +130 to +148
private enum OutputMode {
@Label(value = "Replace", description = "Replaces the selected binary object column with the PNG column.")
Replace, //
@Label(value = "Append", description = "Appends a new PNG column to the table.")
Append;
}

/**
* Persists the OutputMode enum as the legacy string values "Append" / "Replace" under key "replace".
*/
private static final class OutputModePersistor implements NodeParametersPersistor<OutputMode> {

private static final String CFG_KEY = "replace";

@Override
public OutputMode load(final NodeSettingsRO settings) throws InvalidSettingsException {
final String value = settings.getString(CFG_KEY);
return OutputMode.valueOf(value);
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OutputMode enum constants are mixed-case (Replace/Append), which is inconsistent with the existing ReplacePolicy enum in the same package (REPLACE/APPEND) and other node-parameter enums in this repository. Consider renaming constants to uppercase and adjusting OutputModePersistor to map the legacy persisted strings ("Replace"/"Append") to the new constants, similar to other legacy persistors (e.g., PNGsToBinaryObjectsNodeSettings.LegacyReplacePersistor).

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deliberate, such that the enum name matches the persisted string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally reuse existing enums but I can see your point.

You could reuse the old enum and add a "getFromValue" method which you would use in the load method of this persistor similar to here which also would resolve these sonar issues.

But since the old enum is only used in this node I could also live with your solution. I'll let you decide.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It initially felt strange to me to have two enums (which is unavoidable) and use the old enum as a mapping to the persistence key/layer, but after more thought I think that actually makes sense. The old enum is therefore the source of truth for how the keys are defined and serves as a mapping between display and persistence.

I'll do as you suggest (assuming I've understood).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Didn't do an exhaustive scan through all values, instead just compared string value to one and chose the other if not matching.

@tcrundall-tng tcrundall-tng force-pushed the enh/UIEXT-3487-webui-migration-binary-objects-to-pngs branch from 6417edf to bf48300 Compare March 25, 2026 11:40
@tcrundall-tng
Copy link
Author

⚠️ Should only be merged once #43 is merged ⚠️

Copy link
Contributor

@mgohm mgohm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #43 is merged and @tcrundall-tng checked my comment its ready to be merged

Comment on lines +130 to +148
private enum OutputMode {
@Label(value = "Replace", description = "Replaces the selected binary object column with the PNG column.")
Replace, //
@Label(value = "Append", description = "Appends a new PNG column to the table.")
Append;
}

/**
* Persists the OutputMode enum as the legacy string values "Append" / "Replace" under key "replace".
*/
private static final class OutputModePersistor implements NodeParametersPersistor<OutputMode> {

private static final String CFG_KEY = "replace";

@Override
public OutputMode load(final NodeSettingsRO settings) throws InvalidSettingsException {
final String value = settings.getString(CFG_KEY);
return OutputMode.valueOf(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally reuse existing enums but I can see your point.

You could reuse the old enum and add a "getFromValue" method which you would use in the load method of this persistor similar to here which also would resolve these sonar issues.

But since the old enum is only used in this node I could also live with your solution. I'll let you decide.

UIEXT-3487 (WebUI-Migration Binary Objects to PNGs)
@tcrundall-tng tcrundall-tng force-pushed the enh/UIEXT-3487-webui-migration-binary-objects-to-pngs branch from bf48300 to cfa9c34 Compare March 25, 2026 14:33
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
83.3% Coverage on New Code (required ≥ 85%)
6.2% Duplication on New Code (required ≤ 1%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants