Add-on version handling - #5720
Conversation
28d542c to
a8bf325
Compare
|
Since I just discovered the problem with marketplace link resolution, I added the fix to this PR. It fits nicely to the other fixes, and concerns the same code. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/marketplace-link-resolution/169828/4 |
4ef2cf3 to
b3fb72e
Compare
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
…ounts', to make sure that we always grab the last one Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
b3fb72e to
364bd58
Compare
|
I've rebased this PR and resolved where this and #5710 "stepped on each other's toes". |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new, core-level version model (Version and VersionRange) and migrates add-on version handling away from string parsing (and away from the marketplace-specific BundleVersion) to enable consistent comparison, sorting, and compatibility evaluation across core, REST, and marketplace add-on services.
Changes:
- Add
org.openhab.core.common.VersionandVersionRange(plus tests) and remove marketplaceBundleVersion. - Migrate
Addonversion fromStringto@Nullable Version, including REST serialization via a newAddonDTOand updated storage handling in remote add-on services. - Improve marketplace parsing/robustness (e.g., handler
supports()null-safety, version range parsing, resource link extraction, and GitHub “raw” link handling).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.core/src/test/java/org/openhab/core/common/VersionTest.java | Adds unit tests for the new Version comparison/parsing behavior. |
| bundles/org.openhab.core/src/test/java/org/openhab/core/common/VersionRangeTest.java | Adds unit tests for VersionRange parsing, normalization, and inclusion rules. |
| bundles/org.openhab.core/src/main/java/org/openhab/core/common/VersionRange.java | New core VersionRange implementation used for compatibility checks. |
| bundles/org.openhab.core/src/main/java/org/openhab/core/common/Version.java | New core Version implementation compatible with OSGi versions. |
| bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/KarafAddonService.java | Uses Version for Karaf feature-derived add-on versions. |
| bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/addons/AddonResource.java | Switches REST representation to AddonDTO (streamed DTO output, updated OpenAPI schemas). |
| bundles/org.openhab.core.io.console/src/main/java/org/openhab/core/io/console/internal/extension/AddonConsoleCommandExtension.java | Adjusts console output for nullable Version instead of blank strings. |
| bundles/org.openhab.core.addon/src/test/java/org/openhab/core/addon/AddonTest.java | Adds tests covering the updated Addon model and builder behavior with Version. |
| bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/internal/JarFileAddonService.java | Uses Version for bundle-derived add-on versions. |
| bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/dto/AddonDTO.java | New DTO used for add-on serialization/deserialization (REST + storage). |
| bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/Addon.java | Migrates add-on version to @Nullable Version, adjusts builder and immutability/copying behavior. |
| bundles/org.openhab.core.addon.marketplace/src/test/java/org/openhab/core/addon/marketplace/test/TestAddonService.java | Updates marketplace tests to use Version and DTO storage serialization. |
| bundles/org.openhab.core.addon.marketplace/src/test/java/org/openhab/core/addon/marketplace/test/TestAddonHandler.java | Makes supports() contentType nullable (null-safe matching). |
| bundles/org.openhab.core.addon.marketplace/src/test/java/org/openhab/core/addon/marketplace/BundleVersionTest.java | Removes tests for deprecated BundleVersion. |
| bundles/org.openhab.core.addon.marketplace/src/test/java/org/openhab/core/addon/marketplace/AbstractRemoteAddonServiceTest.java | Updates mocks to return Version instead of string versions. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/MarketplaceAddonHandler.java | Updates handler API to accept nullable contentType. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/json/JsonAddonService.java | Uses VersionRange for compatibility evaluation and Version for add-on versions. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityUIWidgetAddonHandler.java | Updates supports() signature and improves install error messaging. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityTransformationAddonHandler.java | Updates supports() signature to accept nullable contentType. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityRuleTemplateAddonHandler.java | Updates supports() signature to accept nullable contentType. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityMarketplaceAddonService.java | Reworks compatibility parsing to use VersionRange, improves resource link extraction, and adds GitHub raw-link normalization. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityBundleAddonHandler.java | Makes supports() null-safe and minor refactor in install. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityBlockLibaryAddonHandler.java | Updates supports() signature to accept nullable contentType. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/BundleVersion.java | Removes deprecated marketplace-only version implementation. |
| bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/AbstractRemoteAddonService.java | Moves core versioning to Version, switches storage serialization to AddonDTO, updates refresh gating and logging. |
| bundles/org.openhab.core.addon.marketplace.karaf/src/main/java/org/openhab/core/addon/marketplace/karaf/internal/community/CommunityKarafAddonHandler.java | Updates supports() signature to accept nullable contentType. |
| bundles/org.openhab.core.addon.eclipse/src/main/java/org/openhab/core/addon/eclipse/internal/EclipseAddonService.java | Uses Version for bundle-derived add-on versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I am just wondering if there is anything specific to our version definitions and ranges that is not covered by standard maven versions and ranges. I put some version comparison logic in a recent UpgradeTool PR where I directly use the maven classes: #5379. Here is a reference to the maven classes: https://maven.apache.org/ref/3.6.2/maven-artifact/apidocs/org/apache/maven/artifact/versioning/package-summary.html |
I wrote this code 1.5 years ago, so the details aren't exactly fresh, but yes, I believe there are some differences. One is that this code takes into account the "special OH rules" for the order of milestone -> release candidate -> release, while standard Maven just do alphabetical comparison or something like that. Another is that I dislike Maven's shorthand form, where if you only specify one version, that automatically means "this or newer". I think that's ambiguous, it would just as well mean "only this version specifically". The current logic doesn't accept anything that doesn't have two endpoints, so it would just consider it invalid. In this PR however, I implemented support for open-ended, but you'd have to indicate that it's open-ended: This was my choice, but I think it's needed, because you often have no idea when it will stop working., which makes people guess, which again leads to marketplace add-ons just suddenly "disappearing" when people upgrade for no other reason than that nobody has bumped the upper limit. When you don't know, I think it's better to make it open-ended, and the day it breaks on some version, it's easy to modify the add-on entry and put the appropriate upper endpoint there. Maven also accepts a "range" without any brackets/parentheses, as I mentioned above, which "means" this or any newer version. That sort of works for Maven because it's specified in dedicated "version" fields, however, we need to "recognize it" as part of a larger text (the title), so allowing such expressions would be very unfortunate. By having a stricter syntax, we can be reasonably certain that when we find a "match" in the title, it is actually a version range, and not something different. That said, I've made My idea when I made this was to have one definition that is used everywhere, avoiding confusion and inconsistency. edit: Don't forget that we're not only dealing with Maven. This integrates "seamlessly" with OSGi's version and version range concepts, and I must admit that I had more focus on aligning with OSGi than with Maven. |
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
943605b to
ccf0718
Compare
I think Maven does that right. It is OSGI that cannot handle this properly. OSGI versions are
We can like or dislike this, but it is a fairly standard way of doing things. It also allows the full syntax I believe. OSGI, depending on the tool used (BND, Eclipse) may actually do the same thing. I kind of feel like we are reinventing the wheel. Is there anything OSGI specific in your code? |
Maven don't have these concepts either. What Maven has, is the very messy I've never heard anything about Maven having rules for milestones, release candidates etc... I think it only treats whatever you put as the "qualifier" as an alphanumeric value, except the magical Regardless, Maven has this other "huge problem" that it doesn't differentiate between a version and a version range. "Everything" in a way is a version range. I think that's very messy.
I don't know which wheel we're reinventing, or what you mean by "OSGi specific", I originally made these as extension of OSGi's Today we have I assume that what you want to use instead is one of these? https://maven.apache.org/ref/3.9.16/maven-artifact/xref/org/apache/maven/artifact/versioning/DefaultArtifactVersion.html I must admit that I haven't studied in details how they work, because they are, in my view, quite mismatched to what is needed here. It should also be said that I have done a lot more around versioning and the marketplace in a never completed branch (because I met obstacles I don't know how to get around), where both |
This is a misunderstanding. The last "number" is just the alphanumeric qualifier. The reason it is used on the marketplace is that it's the only way to get |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Disclosure: This review was generated with AI assistance. The findings and comments were reviewed before submission.
| this.keywords = addon.getKeywords(); | ||
| List<@NonNull String> stringList = addon.getCountries(); | ||
| if (!stringList.isEmpty()) { | ||
| this.countries = stringList; |
There was a problem hiding this comment.
Could we preserve the existing REST response contract by always using empty collections instead of leaving these fields null?
MainUI currently assumes that fields such as loggerPackages and properties are present and accesses them using expressions such as addon.loggerPackages.length and Object.keys(addon.properties).length.
Because Gson normally omits null fields, add-ons without these values may no longer contain loggerPackages, properties, or countries in the response, potentially causing errors on the add-on details page.
Initializing these fields with empty collections, or assigning them unconditionally, would preserve the previous response shape.
There was a problem hiding this comment.
It was intended for them to be missing from the DTO if empty, like most other fields. I've already made a PR in MainUI that makes it handle that: openhab/openhab-webui#4365.
It's very "arbitrary" when some fields have to be initialized if empty, while most don't, which is why I preferred to unity the behavior. Also, since there never was a DTO class in the past, the serialization wasn't exactly "designed", but became what it became based on hot Addon itself behaved.
| } | ||
| Version v1 = addon1.getVersion(); | ||
| Version v2 = addon2.getVersion(); | ||
| return v1 == null && v2 == null ? 0 : v2 == null ? 1 : v2.compareTo(v1); |
There was a problem hiding this comment.
This comparator appears to violate the Comparator contract when exactly one add-on has no version.
For a versioned add-on a and an unversioned add-on b, both of these comparisons return a positive value:
compare(a, b)
compare(b, a)The first result comes from v2 == null, while the second comes from Version.compareTo(null) returning 1. This makes the comparator non-antisymmetric and may result in incorrect ordering or an IllegalArgumentException during sorting.
Could both one-null cases be handled explicitly? It would also be useful to add tests for (null, null), (null, version), and (version, null).
| boolean oss = SNAPSHOT_PATTERN.matcher(other.qualifier).matches(); | ||
| if (ql < 0 && oql < 0) { | ||
| // Both aren't positive integers, snapshots are newer, otherwise do a simple string comparison | ||
| if (ss) { |
There was a problem hiding this comment.
The snapshot comparison appears to be non-antisymmetric when both qualifiers are snapshot variants with different capitalization.
In that case, both ss and oss are true, so the first if (ss) returns 1. Reversing the operands also returns 1. The later branch intended to compare two differently capitalized snapshot qualifiers is therefore unreachable.
Could the ss && oss case be handled before the cases where only one qualifier is a snapshot? A test that verifies comparator symmetry for SNAPSHOT and snapshot would help prevent this regression.
| } | ||
|
|
||
| Version v = addonEntry.version == null || addonEntry.version.isBlank() ? null | ||
| : Version.valueOf(addonEntry.version); |
There was a problem hiding this comment.
Could version parsing be isolated per repository entry?
Previously, the version was accepted as an arbitrary string. With this change, a single entry containing a version syntax unsupported by Version.valueOf() can throw from inside the stream mapping and abort processing of the entire repository.
A malformed version should probably be logged and treated as unknown, or cause only that individual entry to be skipped. It should not make all add-ons from the source disappear.
| public Builder withProperties(Map<String, Object> properties) { | ||
| this.properties.putAll(properties); | ||
| public Builder withProperties(@Nullable Map<@NonNull String, @NonNull Object> properties) { | ||
| this.properties = properties; |
There was a problem hiding this comment.
withProperties() previously copied the supplied values into the builder’s mutable map. It now stores the supplied map directly.
Apart from changing the previous merge semantics, this can also cause an unexpected exception:
builder.withProperties(Map.of("one", 1))
.withProperty("two", 2);Since Map.of() is immutable, withProperty() will throw UnsupportedOperationException. A mutable map supplied by a caller can also be modified indirectly through the builder.
Could the supplied map be defensively copied into a new LinkedHashMap? A test using an immutable map followed by withProperty() would cover this case.
There was a problem hiding this comment.
Sure, we can make defensive copies all we want. My assumption was that people will either use withProperties() or withProperty(), not both, and if they must use both, then they'd better not provide an immutable map first. But I can make it handle "anything we want". When it comes to the merge semantics, the primary reason for setting the map directly, is that it should be possible to "null" the properties. Previously, you couldn't do that, you could only add. But that can be solved too, by have two different paths, where null means "set it to null", while non-null means "add to what's already here".
|
@wborn @mherwege I'm a bit unsure of what to do here. I could address the above comments, but if @mherwege is opposed to the whole idea of what I'm doing, is it pointless? I don't mind addressing the points, but I don't think it's "that much fun" that I'll gladly do it knowing that it will never go anywhere. As said above, I have a lot more work that is built upon the above, so some of the "details" that might seem excessive are there because they are needed in the code that is built on top of it. That doesn't make them "bad" in themselves as I see it, but seeing the rest of the code might make the motivation a bit clearer. I won't ask you to try to review that though, both since it's quite complex, and because it's not finished and has some unresolved challenges in it. But those challenges aren't about the version handling itself, but about how to store the version information in a way that it won't be lost (various add-on types are stored in very different ways locally, and not all of them have an obvious way to store "extra information"), and it's about |
It is not that I am opposed to what you are doing. I questionned if creating specific logic to be maintained going forward is the right thing to do, if there may be other ways to achieve this. You seem to think there is none. I must say I am not yet entirely convinced about that, but that does not mean I will oppose this. Overall, the code looks good to me. |
There is none that I've been able to identify that fits the current requirements/needs. I don't think that logic will require much maintenance until there is a change in OHs "versioning scheme", and if that happens, I would think that it's beneficial to be able to adjust the logic accordingly, in one place, and then carry on. |
Back in early 2025, I did a lot of work to try to made add-ons versioned, so that e.g. the marketplace would have a concept of the version of an add-on, allow upgrade, downgrade, update notification etc. This work stalled for a couple of reasons that I wasn't able to solve, and this is still the case, but I've kept the branch in case I see a solution to these issues some time in the future.
From time to time, the handling of add-on versions during upgrades, the fact that marketplace add-ons just "disappear" during upgrade, failures in the version range system and similar comes up, as it did again lately: https://community.openhab.org/t/marketplace-versioning-issues/169748
Knowing that I've solved many of those little things that are frustrations and obstacles for people to this day, a long time ago, I finally decided to try to extract just the version handling from the branch with the earlier work, and this PR is the result.
I've tried to remove everything that isn't directly related to the add-on version, but I've kept a few pure bugfixes just because, why not fix them when I know that they are there, and the fix has already been found..?
The changes in the PR centers about two new classes,
VersionandVersionRange. They makeBundleVersionunnecessary, so it has been removed.VersionandVersionRangeclosely follow the logic of OSGi's classes with the same names, but they are adjusted to fit with "OH's rules". In fact, I originally made them as extensions to the OSGi versions, but that caused so many problems during serialization/deserialization that I had to abandon that. They are still "compatible" with the OSGi classes and have methods for easy conversion both ways, without using string parsing.Versionis used in theAddonclass itself, instead of basing everything on strings and string parsing. This makes it easy to compare, sort and "process" versions consistently throughout the system, and since it is easily convertible to/from OSGi'sVersion, it integrates well with version information obtained for bundles.VersonRangeis only used for the marketplace, but is also available system-wide, should the need arise elsewhere.In addition, I've addressed a number of parsing issues that happen for some marketplace add-ons under some circumstances, every change I've made there is based on things I've "observed in the wild". The annotation and bugfix for
MarketplaceAddonHandler.supports()is one of those, but I considered it important enough to include. When the NPE hits, the marketplace parsing is aborted and the user will see no marketplace add-ons at all.Last, I included an unrelated fix, that I've long thought should have been brought in: Special treatment for GitHub links. Since GitHub is widely used for hosting marketplace resources, there have been numerous add-ons that have "used the wrong link" (I have notified those I've come across, so I think most of them have been rectified, but this will continue to happen in the future). GitHub have two ways to link a file, it's the "normal" link, which take you to a web page that shows the file, and then there's the "raw" link, which links only to the file itself. For the marketplace to work, people must use the "raw" link, but it's easy to miss. The fix here inspects links that are GitHub only, and checks whether they are "raw" or not. If they aren't, it adds a query parameter that GitHub provides that makes sure that when the link is used, you're forwarded to the "raw" file instead of getting the webpage view of the file.