Skip to content

Commit 653da37

Browse files
Fikri-20jonesbusy
andauthored
fix: make JDK.min(Set, String) actually use the jdks parameter (#1713)
* fix: make JDK.min(Set, String) actually use the jdks parameter The previous implementation had a precedence bug: if (jdks == null || jdks.isEmpty() && jenkinsVersion == null) Because && binds tighter than ||, this evaluated as: if (jdks == null || (jdks.isEmpty() && jenkinsVersion == null)) As a result, whenever jdks was non-null and non-empty the method fell through to a single-line return that completely ignored the jdks argument. A plugin declaring e.g. [JAVA_17, JAVA_21] would get back JAVA_11 for Jenkins 2.479.3, even though 11 is not in the declared set. The fix rewrites the method to: 1. Fast-path on null jenkinsVersion (delegate to single-arg min). 2. Compute the version-range list once. 3. Intersect the caller-supplied jdks with that list and return the minimum; fall back to the range minimum only when the intersection is empty. Also replaces the bare orElseThrow() with orElse(JDK.min()) to avoid a cryptic NoSuchElementException when a future version boundary leaves the supported list empty. * fix: apply Spotless formatting to JDK.java * fix: address Copilot review feedback on JDK.min(Set, String) - Treat empty jenkinsVersion as absent (consistent with get(String)) - Return min(jdks) on no-intersection fallback instead of version-min - Add tests for empty version string and no-overlap cases - Clarify Javadoc to document no-intersection fallback behavior --------- Co-authored-by: Valentin Delaye <jonesbusy@users.noreply.github.com>
1 parent 6459213 commit 653da37

2 files changed

Lines changed: 34 additions & 6 deletions

File tree

  • plugin-modernizer-core/src
    • main/java/io/jenkins/tools/pluginmodernizer/core/model
    • test/java/io/jenkins/tools/pluginmodernizer/core/model

plugin-modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/model/JDK.java

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -259,15 +259,27 @@ public static JDK min(Set<JDK> jdks) {
259259
}
260260

261261
/**
262-
* Return the minimum JDK
263-
* @param jdks List of JDKS. Can be null or empty
264-
* @return The minimum JDK. If the list is empty, return the minimum JDK available
262+
* Return the minimum JDK that is both declared in {@code jdks} and supported by
263+
* {@code jenkinsVersion}. Falls back to {@code min(jdks)} when either parameter
264+
* is absent, or when no declared JDK overlaps the version-supported set.
265+
*
266+
* @param jdks JDKs declared by the plugin (e.g. from its Jenkinsfile). Can be null or empty.
267+
* @param jenkinsVersion Jenkins core version the plugin targets. Can be null or empty.
268+
* @return The minimum compatible JDK, never null.
265269
*/
266270
public static JDK min(Set<JDK> jdks, String jenkinsVersion) {
267-
if (jdks == null || jdks.isEmpty() && jenkinsVersion == null) {
268-
return JDK.min();
271+
if (jenkinsVersion == null || jenkinsVersion.isEmpty()) {
272+
return min(jdks);
273+
}
274+
List<JDK> supportedByVersion = get(jenkinsVersion);
275+
if (jdks == null || jdks.isEmpty()) {
276+
return supportedByVersion.stream().min(JDK::compareMajor).orElse(JDK.min());
269277
}
270-
return JDK.get(jenkinsVersion).stream().min(JDK::compareMajor).orElseThrow();
278+
// Intersect: lowest JDK that is both in the plugin's declared set and supported by this Jenkins version
279+
return jdks.stream()
280+
.filter(supportedByVersion::contains)
281+
.min(JDK::compareMajor)
282+
.orElseGet(() -> min(jdks));
271283
}
272284

273285
/**

plugin-modernizer-core/src/test/java/io/jenkins/tools/pluginmodernizer/core/model/JDKTest.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,22 @@ public void min() {
6565
assertEquals(JDK.JAVA_11, JDK.min(Set.of(JDK.values()), "2.361.1"));
6666
assertEquals(JDK.JAVA_11, JDK.min(Set.of(JDK.values()), "2.462.3"));
6767
assertEquals(JDK.JAVA_17, JDK.min(Set.of(JDK.values()), "2.479.3"));
68+
// jdks subset: the result must respect the declared JDKs, not just the Jenkins version range.
69+
// Jenkins 2.479.3 supports Java 17 and 21; a plugin declaring [17,21] must yield 17, not 11.
70+
assertEquals(JDK.JAVA_17, JDK.min(Set.of(JDK.JAVA_17, JDK.JAVA_21), "2.479.3"));
71+
// Plugin declares only Java 21; Jenkins 2.462.3 supports up to Java 21, so the result is 21.
72+
assertEquals(JDK.JAVA_21, JDK.min(Set.of(JDK.JAVA_21), "2.462.3"));
73+
// null / empty jdks fall back to version-range minimum
74+
assertEquals(JDK.JAVA_11, JDK.min(null, "2.387.3"));
75+
assertEquals(JDK.JAVA_11, JDK.min(Set.of(), "2.387.3"));
76+
// null version falls back to single-param min()
77+
assertEquals(JDK.JAVA_8, JDK.min(null, null));
78+
// empty version string is treated the same as null
79+
assertEquals(JDK.JAVA_8, JDK.min(null, ""));
80+
// no overlap: plugin declares only JAVA_8, but Jenkins 2.479.3 only supports 17+ → falls back to min(jdks)
81+
assertEquals(JDK.JAVA_8, JDK.min(Set.of(JDK.JAVA_8), "2.479.3"));
82+
// no overlap: plugin declares only JAVA_25, but Jenkins 2.346.1 supports 8-17 → falls back to min(jdks)
83+
assertEquals(JDK.JAVA_25, JDK.min(Set.of(JDK.JAVA_25), "2.346.1"));
6884
}
6985

7086
@Test

0 commit comments

Comments
 (0)