jamesfredley commented on issue #14058: URL: https://github.com/apache/grails-core/issues/14058#issuecomment-4570394721
This is a real issue, and it has not been resolved by the plugin discovery rework. I dug into it to clarify the goal of the ticket and to confirm whether the existing test coverage actually exercises the affected code path. ## What this ticket is really about The plugin compatibility check compares the plugin's declared `grailsVersion` range against the running Grails version. The original code referenced in the description lived in `DefaultGrailsPluginManager` and delegated the actual version comparison to `grails.plugins.VersionComparator`. That comparator is still used today, and it does **not** understand milestone (`M`) or release candidate (`RC`) qualifiers. The ticket asks us to test (and, as it turns out, fix) the compatibility check when the plugin and/or the application is on a milestone or snapshot version. ## Root cause `VersionComparator` tokenises a version by splitting on `.` and then keeping only the tokens that are purely numeric (`==~ /\d+/`). `-SNAPSHOT` / `.BUILD-SNAPSHOT` are handled specially, but `M`/`RC` qualifiers are not, so: - `7.0.0-M1` -> `["7","0","0-M1"]` -> `[7, 0]` (the patch `0` **and** the `-M1` qualifier are both dropped) - `7.0.5-M1` -> `[7, 0]` (the patch `5` is silently lost, so it compares as `7.0`) - `7.0.0.M1` -> `[7, 0, 0]` (the `M1` is dropped, so it compares **equal** to the final `7.0.0`) ## Concrete bugs this produces 1. **Lost patch number.** A plugin requiring `7.0.3 > *` running on `7.0.5-M1` is wrongly reported as incompatible, because `7.0.5-M1` is parsed as `7.0` which is less than `7.0.3`. 2. **All milestones/RCs of a base version compare equal.** `7.0.0-M1`, `7.0.0-M2`, `7.0.0-RC1` are indistinguishable, and each compares equal to the final `7.0.0`. So a plugin that requires the final `7.0.0` is considered compatible with a pre-release `7.0.0-RC1`, and milestone ordering is meaningless. Because the result of the check is only logged (it does not block plugin loading), the practical symptom is misleading "may not be compatible with this application" warnings - which is precisely what end users hit while running on the current `7.0.0-RC*` / `-M*` builds. ## Why it looked like it already had coverage The repository has three separate version representations: | Class | Handles `M`/`RC` correctly? | Tested for `M`/`RC`? | Used by the plugin compatibility check? | |-------|------------------------------|----------------------|-----------------------------------------| | `grails.plugins.VersionComparator` | No | No (before this change) | **Yes** | | `org.grails.datastore.mapping.core.grailsversion.GrailsVersion` / `Snapshot` | Yes | Yes | No | | `grails.init.GrailsVersion` (wrapper) | Yes | Yes | No | The extensive milestone/RC tests live on the `GrailsVersion`/`Snapshot` classes, which the plugin compatibility check never calls. The comparator that the check actually uses had a single `BUILD-SNAPSHOT` case and zero milestone/RC cases. ## Fix (PR linked below) `VersionComparator` now parses each version into its numeric components plus an optional qualifier, compares the numeric components first (zero padded so `7.0` == `7.0.0`), and only then breaks ties on the qualifier, following the same ordering as `GrailsVersion`/`Snapshot`: ``` 7.0.0-M1 < 7.0.0-M2 < 7.0.0-RC1 < 7.0.0-RC2 < 7.0.0-SNAPSHOT < 7.0.0 ``` Dotted (`7.0.0.M1`) and hyphenated (`7.0.0-M1`) forms are treated as equivalent, matching is case insensitive, and unrecognised qualifiers are still treated as a final release to keep existing behaviour. Coverage was added for the comparator (`VersionComparatorSpec`) and for the integrated compatibility check (`DefaultGrailsPluginManagerSpec`) with milestone, RC and snapshot versions on both the application and the plugin. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
