AP-25387: Replaces all usages of deprecated HubItemVersion with ItemVersion #37
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces deprecated HubItemVersion API usage with the newer ItemVersion API across component manipulation code. The changes modernize the version handling mechanism for Hub components by using the new URL-based version parsing and application methods.
Key Changes:
- Replaced
HubItemVersionwithItemVersionand updated version parsing to useURLResolverUtil - Refactored URI construction to use
URIBuilderwith proper query parameter handling - Removed deprecated update command logic and simplified error handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ManipulateComponents.java | Updated version parsing from HubItemVersion.of() to URLResolverUtil.parseVersion(), replaced direct URI manipulation with URIBuilder, removed deprecated getUpdateComponentCommand() method, and simplified exception handling |
| ComponentAPI.java | Updated method call to reflect renamed openChangeComponentItemVersionDialog() method |
Comments suppressed due to low confidence (1)
org.knime.ui.java/src/eclipse/org/knime/ui/java/api/ManipulateComponents.java:1
- The error message is vague about why URI construction failed. Consider including information about the target version being applied or the specific query parameters that caused the issue to help users diagnose the problem.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final var newSrcUriBuilder = new URIBuilder(srcUri); | ||
| final var newQueryParams = URLResolverUtil.applyTo(targetVersion, newSrcUriBuilder.getQueryParams()); | ||
| newSrcUriBuilder.setParameters(newQueryParams); |
There was a problem hiding this comment.
The query parameters are retrieved, modified, and set back in separate steps. Consider checking if URLResolverUtil.applyTo() could be simplified or if this pattern could be encapsulated in a helper method to reduce the cognitive load of understanding the URI modification flow.
|
Bit confused, I think there should be a corresponding knime-gateway PR? https://knime-com.atlassian.net/browse/NXT-2173?focusedCommentId=182143 |
3nol
force-pushed
the
todo/AP-25387-remove-deprecated-hubitemversion
branch
from
1be509b to
430be89
Compare
…ersion AP-25387 (Remove deprecated `HubItemVersion` and its `LinkType`)
3nol
force-pushed
the
todo/AP-25387-remove-deprecated-hubitemversion
branch
from
430be89 to
aa00375
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| final var srcUri = component.getTemplateInformation().getSourceURI(); | ||
| final var currentVersion = HubItemVersion.of(srcUri).orElse(HubItemVersion.currentState()); | ||
| final var currentVersion = URLResolverUtil.parseVersion(srcUri.getQuery()).orElseGet(ItemVersion::currentState); |
There was a problem hiding this comment.
The original code used orElse(HubItemVersion.currentState()) which evaluates the default eagerly, while the new code uses orElseGet(ItemVersion::currentState) which evaluates lazily. If ItemVersion.currentState() has side effects or is computationally expensive, this change in evaluation strategy could alter behavior. Consider whether this semantic change is intentional.
| .build(); | ||
| } | ||
|
|
||
| // (2) Perform component update to fetch its info. |
There was a problem hiding this comment.
The comment references step '(2)' but there's no corresponding comment for step '(1)' visible in the unchanged code context. While step '(1)' appears at line 117, the numbering should be consistent throughout the method to avoid confusion.
No description provided.