Conversation
bernd-wiswedel
requested review from
Copilot and
knime-ghub-bot
and removed request for
a team and
Copilot
bernd-wiswedel
force-pushed
the
bug/AP-25686_try_catch_rwe_error_msgs
branch
from
8c1e14b to
3425b65
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates node execution status handling to carry structured NodeMessage objects (instead of plain strings), improving error propagation—especially for Try/Catch scopes—and aligns call sites to use the richer message type.
Changes:
- Add
getNodeMessage()toNodeContainerExecutionStatusand introducenewFailure(NodeMessage)(deprecatingnewFailure(String)). - Update Try/Catch error handling to prepend context via
NodeMessagerather than stringifying status. - Update SubNodeContainer failure creation to preserve the full
NodeMessageand add an API tooling filter for the interface change.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| org.knime.core/src/eclipse/org/knime/core/node/workflow/execresult/NodeContainerExecutionStatus.java | Adds getNodeMessage() and a NodeMessage-based failure factory; deprecates string-based failure creation. |
| org.knime.core/src/eclipse/org/knime/core/node/workflow/execresult/NodeContainerExecutionResult.java | Marks getNodeMessage() as an override of the new interface method. |
| org.knime.core/src/eclipse/org/knime/core/node/workflow/SubNodeContainer.java | Uses newFailure(NodeMessage) to preserve node messages on failure. |
| org.knime.core/src/eclipse/org/knime/core/node/workflow/NodeExecutionJob.java | Uses status.getNodeMessage() to build Try/Catch failure messages consistently. |
| org.knime.core/.settings/.api_filters | Suppresses API tooling warnings for the internal interface change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -98,6 +116,12 @@ public NodeContainerExecutionStatus getChildStatus(final int idSuffix) { | |||
| return this; | |||
| } | |||
|
|
|||
|
|
|||
| @Override | |||
| public NodeMessage getNodeMessage() { | |||
| return message; | |||
| } | |||
|
|
|||
| /** @return false */ | |||
| @Override | |||
| public boolean isSuccess() { | |||
| @@ -107,7 +131,7 @@ public boolean isSuccess() { | |||
| /** {@inheritDoc} */ | |||
| @Override | |||
| public String toString() { | |||
| return message; | |||
| return message.getMessage(); | |||
There was a problem hiding this comment.
newFailure(NodeMessage message) assumes message is non-null (used in getNodeMessage() and toString()), but there is no null check. Given the interface contract that getNodeMessage() returns non-null, please enforce non-null here (e.g., requireNonNull / CheckUtils) or normalize null to NodeMessage.NONE (or an ERROR placeholder) to avoid potential NPEs.
| @Deprecated(since = "5.12", forRemoval = true) | ||
| static NodeContainerExecutionStatus newFailure(final String message) { | ||
| return newFailure(new NodeMessage(NodeMessage.Type.ERROR, message)); | ||
| } |
There was a problem hiding this comment.
The newFailure(String) overload is now deprecated/forRemoval, but this interface still uses it internally (e.g., to initialize the FAILURE constant). To avoid introducing new deprecation warnings (and future breakage once removed), update internal call sites to use newFailure(NodeMessage) directly.
| @Override | ||
| public NodeMessage getNodeMessage() { | ||
| return m_message; | ||
| } |
There was a problem hiding this comment.
getNodeMessage() currently returns m_message directly, but m_message can be null (e.g., if not set yet or if deserialization loads older results without the field). Since NodeContainerExecutionStatus#getNodeMessage() is documented as non-null and callers now invoke methods on it (e.g., prependMessage(...)), please normalize null to NodeMessage.NONE (and consider doing the same in setMessage(...)).
|
No description provided.