Skip to content

[MSHARED-1416] - Review and improve the term "Jdk revision" #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
belingueres wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[MSHARED-1416] - Review and improve the term "Jdk revision" #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/main/java/org/apache/maven/shared/jar/JarData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,23 @@ public int getNumPackages() {
return jarClasses.getPackages().size();
}

/**
* Get the maximum Java class version of the JAR, if it can be determined.
* @return the maximum Java class version, or null if it cannot be determined
* @since 3.2.1
*/
public String getMaxJavaClassVersion() {
return jarClasses.getMaxJavaClassVersion();
}

/**
* Get the JDK revision of the JAR, if it can be determined.
* @return the JDK revision, or null if it cannot be determined
* @deprecated since 3.2.1, use {@link #getMaxJavaClassVersion()} instead, which is more accurate name.
*/
@Deprecated
public String getJdkRevision() {
return jarClasses.getJdkRevision();
return getMaxJavaClassVersion();
}

public void setJarIdentification(JarIdentification jarIdentification) {
Expand Down
46 changes: 41 additions & 5 deletions src/main/java/org/apache/maven/shared/jar/classes/JarClasses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ public class JarClasses {
private boolean isDebugPresent;

/**
* The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular
* classes are not used, this is the minimum JVM that guarantees compatibility.
* The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular

@elharo elharo Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "Java class version" is a thing. This also needs to be very clear about whether you're talking about the class file format version or the Java version

* classes are not used, this is the minimum JVM version that guarantees compatibility.

@elharo elharo Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so is it JVM version or Java byte code version?

@belingueres belingueres Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JVM version

@elharo elharo Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really doubt that's right. If it is, then the field is incorrectly named. One of them is certainly wrong.

@belingueres belingueres Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc comment most probably is confusing. How about this comment:

/**
 * The highest Java version required by any class in the JAR (e.g. "17" if the newest
 * class file version present corresponds to Java 17).
 * <p>
 * Because this is the highest requirement across all classes, it also serves as the
 * minimum JVM version needed to safely run the entire JAR. The JAR may still happen
 * to run on an older JVM if the classes requiring this version are never invoked,
 * but that isn't guaranteed.
 */

@elharo elharo Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is right, the field name is wrong. I'm prettys sure the field name is wrong whether this is right or not.

@belingueres belingueres Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field name is the one agreed in the issue's discussion (#100)

@elharo elharo Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't mean the field name is correct. And the docs are still inconsistent.

*/
private String jdkRevision;
private String maxJavaClassVersion;

/**
* Constructor to create an empty instance.
Expand Down Expand Up @@ -130,12 +130,48 @@ public void setDebugPresent(boolean hasDebugSymbols) {
this.isDebugPresent = hasDebugSymbols;
}

/**
* Get the highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular
* classes are not used, this is the minimum JVM version that guarantees compatibility.
*
* @return the highest Java class version available in any class files, or null if not known.
*
* @since 3.2.1
*/
public String getMaxJavaClassVersion() {
return maxJavaClassVersion;
}

/**
* Set the highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular
* classes are not used, this is the minimum JVM version that guarantees compatibility.
*
* @param maxJavaClassVersion the highest Java class version available in any class files.
*
* @since 3.2.1
*/
public void setMaxJavaClassVersion(String maxJavaClassVersion) {

@elharo elharo Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a setter at all?

@belingueres belingueres Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it is the only way to set a value to the variable.

this.maxJavaClassVersion = maxJavaClassVersion;
}

/**
* The JDK revision of the classes in the JAR.
* @return the JDK revision of the classes in the JAR, or null if not known.
* @deprecated since 3.2.1, use {@link #getMaxJavaClassVersion()} instead, which is more accurate name.
*/
@Deprecated
public String getJdkRevision() {
return jdkRevision;
return getMaxJavaClassVersion();
}

/**
* Set the JDK revision of the classes in the JAR.
* @param jdkRevision the JDK revision of the classes in the JAR.
* @deprecated since 3.2.1, use {@link #setMaxJavaClassVersion(String)} instead, which is more accurate name.
*/
@Deprecated
public void setJdkRevision(String jdkRevision) {
this.jdkRevision = jdkRevision;
setMaxJavaClassVersion(jdkRevision);
}

public List<String> getMethods() {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/apache/maven/shared/jar/classes/JarClassesAnalysis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ private JarClasses analyze(String jarFilename, List<JarEntry> classList) {
maxVersion = moduleInfoVersion;
}

Optional.ofNullable(JAVA_CLASS_VERSIONS.get(maxVersion)).ifPresent(classes::setJdkRevision);
Optional.ofNullable(JAVA_CLASS_VERSIONS.get(maxVersion)).ifPresent(classes::setMaxJavaClassVersion);

return classes;
}
Expand Down
87 changes: 66 additions & 21 deletions src/test/java/org/apache/maven/shared/jar/classes/JarClassesAnalyzerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void analyzeJarWithInvalidClassFile() throws Exception {
assertTrue(jclass.getClassNames().isEmpty());
assertTrue(jclass.getPackages().isEmpty());
assertTrue(jclass.getImports().isEmpty());
assertNull(jclass.getJdkRevision());
assertNull(jclass.getMaxJavaClassVersion());
assertTrue(jclass.getMethods().isEmpty());
}

Expand Down Expand Up @@ -145,14 +145,14 @@ static String[][] testAnalyzeJarVersion() {
void testAnalyzeJarVersion(String jarName, String expectedRevision) throws Exception {
JarClasses jclass = getJarClasses(jarName);

assertEquals(expectedRevision, jclass.getJdkRevision());
assertEquals(expectedRevision, jclass.getMaxJavaClassVersion());
}

@Test
void analyzeJarWithModuleInfoClass() throws Exception {
JarData jarData = getJarData("tomcat-jni-9.0.75.jar");
JarClasses jclass = jarData.getJarClasses();
assertEquals("1.8", jclass.getJdkRevision());
assertEquals("1.8", jclass.getMaxJavaClassVersion());
}

@Test
Expand All @@ -166,7 +166,7 @@ void analyzeJarWithOnlyModuleInfoClass() throws Exception {
assertTrue(jclass.getPackages().isEmpty());
assertTrue(jclass.getClassNames().isEmpty());
assertTrue(jclass.getMethods().isEmpty());
assertNull(jclass.getJdkRevision());
assertNull(jclass.getMaxJavaClassVersion());

JarVersionedRuntimes jarVersionedRuntimes = jarData.getVersionedRuntimes();
assertNotNull(jarVersionedRuntimes);
Expand All @@ -176,7 +176,7 @@ void analyzeJarWithOnlyModuleInfoClass() throws Exception {

JarVersionedRuntime jarVersionedRuntime11 = jarVersionedRuntimes.getJarVersionedRuntime(11);
JarClasses jarClasses11 = jarVersionedRuntime11.getJarClasses();
assertEquals("11", jarClasses11.getJdkRevision());
assertEquals("11", jarClasses11.getMaxJavaClassVersion());
assertTrue(jarClasses11.getImports().isEmpty());
assertEquals(1, jarClasses11.getPackages().size());
assertEquals("", jarClasses11.getPackages().get(0));
Expand All @@ -193,7 +193,7 @@ void analyzeMultiReleaseJarVersion() throws Exception {
// root level information
assertEquals(17, jarData.getNumRootEntries());
JarClasses jclass = jarData.getJarClasses();
assertEquals("1.8", jclass.getJdkRevision());
assertEquals("1.8", jclass.getMaxJavaClassVersion());
assertFalse(jclass.getImports().isEmpty());
assertEquals(1, jclass.getPackages().size());
assertEquals(1, jclass.getClassNames().size());
Expand All @@ -208,7 +208,7 @@ void analyzeMultiReleaseJarVersion() throws Exception {

JarVersionedRuntime jarVersionedRuntime9 = jarVersionedRuntimes.getJarVersionedRuntime(9);
JarClasses jarClasses9 = jarVersionedRuntime9.getJarClasses();
assertEquals("9", jarClasses9.getJdkRevision());
assertEquals("9", jarClasses9.getMaxJavaClassVersion());
assertFalse(jarClasses9.getImports().isEmpty());
assertEquals(1, jarClasses9.getPackages().size());
assertEquals(1, jarClasses9.getClassNames().size());
Expand All @@ -218,7 +218,7 @@ void analyzeMultiReleaseJarVersion() throws Exception {

JarVersionedRuntime jarVersionedRuntime11 = jarVersionedRuntimes.getJarVersionedRuntime(11);
JarClasses jarClasses11 = jarVersionedRuntime11.getJarClasses();
assertEquals("11", jarClasses11.getJdkRevision());
assertEquals("11", jarClasses11.getMaxJavaClassVersion());
assertFalse(jarClasses11.getImports().isEmpty());
assertEquals(1, jarClasses11.getPackages().size());
assertEquals(1, jarClasses11.getClassNames().size());
Expand All @@ -238,25 +238,25 @@ void analyzeMultiReleaseJarVersion() throws Exception {
jarVersionedRuntimes
.getBestFitJarVersionedRuntime(9)
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"9",
jarVersionedRuntimes
.getBestFitJarVersionedRuntime(10)
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"11",
jarVersionedRuntimes
.getBestFitJarVersionedRuntime(11)
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"11",
jarVersionedRuntimes
.getBestFitJarVersionedRuntime(20)
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());

assertThrows(
NullPointerException.class,
Expand All @@ -269,22 +269,22 @@ void analyzeMultiReleaseJarVersion() throws Exception {
"9",
getBestFitReleaseBySystemProperty(jarVersionedRuntimes, "9")
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"9",
getBestFitReleaseBySystemProperty(jarVersionedRuntimes, "10")
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"11",
getBestFitReleaseBySystemProperty(jarVersionedRuntimes, "11")
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
assertEquals(
"11",
getBestFitReleaseBySystemProperty(jarVersionedRuntimes, "20")
.getJarClasses()
.getJdkRevision());
.getMaxJavaClassVersion());
}

/**
Expand All @@ -298,7 +298,7 @@ void analyzeMultiReleaseJarWithVersion11HasALowerJdkRevisionClass() {
JarData jarData = getJarData("multi-release-version-with-lower-jdk-revision-class-0.0.1.jar");
JarClasses jclass = jarData.getJarClasses();

assertNull(jclass.getJdkRevision());
assertNull(jclass.getMaxJavaClassVersion());

JarVersionedRuntimes jarVersionedRuntimes = jarData.getVersionedRuntimes();
assertNotNull(jarVersionedRuntimes);
Expand All @@ -309,7 +309,7 @@ void analyzeMultiReleaseJarWithVersion11HasALowerJdkRevisionClass() {

JarVersionedRuntime jarVersionedRuntime11 = jarVersionedRuntimes.getJarVersionedRuntime(11);
JarClasses jarClasses11 = jarVersionedRuntime11.getJarClasses();
assertEquals("1.8", jarClasses11.getJdkRevision());
assertEquals("1.8", jarClasses11.getMaxJavaClassVersion());
},
"It should not raise an exception");
}
Expand All @@ -324,7 +324,7 @@ void analyzeMultiReleaseJarResourcesOnly() {
JarData jarData = getJarData("multi-release-resources-only-0.0.1.jar");
JarClasses jclass = jarData.getJarClasses();

assertEquals("1.8", jclass.getJdkRevision());
assertEquals("1.8", jclass.getMaxJavaClassVersion());

JarVersionedRuntimes jarVersionedRuntimes = jarData.getVersionedRuntimes();
assertNotNull(jarVersionedRuntimes);
Expand All @@ -336,16 +336,61 @@ void analyzeMultiReleaseJarResourcesOnly() {
JarVersionedRuntime jarVersionedRuntime9 = jarVersionedRuntimes.getJarVersionedRuntime(9);
JarClasses jarClasses9 = jarVersionedRuntime9.getJarClasses();
// no classes found
assertNull(jarClasses9.getJdkRevision());
assertNull(jarClasses9.getMaxJavaClassVersion());

JarVersionedRuntime jarVersionedRuntime11 = jarVersionedRuntimes.getJarVersionedRuntime(11);
JarClasses jarClasses11 = jarVersionedRuntime11.getJarClasses();
// no classes found
assertNull(jarClasses11.getJdkRevision());
assertNull(jarClasses11.getMaxJavaClassVersion());
},
"It should not raise an exception");
}

/**
* Ensures that the deprecated JDK revision property is consistent with the max Java class version property.
* Delete this test when the deprecated JDK revision property is removed, as it will no longer be relevant.
*/
@Test
void deprecatedJdkRevisionEquivalentToMaxJavaClassVersionInJarClasses() throws Exception {
JarClasses jclass = getJarClasses("helloworld-1.8.jar");

assertEquals("1.8", jclass.getJdkRevision());
assertEquals("1.8", jclass.getMaxJavaClassVersion());

jclass.setJdkRevision("1.7");
assertEquals("1.7", jclass.getJdkRevision());
assertEquals("1.7", jclass.getMaxJavaClassVersion());

jclass.setMaxJavaClassVersion("25");
assertEquals("25", jclass.getJdkRevision());
assertEquals("25", jclass.getMaxJavaClassVersion());

jclass.setJdkRevision(null);
assertNull(jclass.getJdkRevision());
assertNull(jclass.getMaxJavaClassVersion());

jclass.setMaxJavaClassVersion(null);
assertNull(jclass.getJdkRevision());
assertNull(jclass.getMaxJavaClassVersion());
}

/**
* Ensures that the deprecated JDK revision property is consistent with the max Java class version.
* Delete this test when the deprecated JDK revision property is removed, as it will no longer be relevant.
*/
@Test
void deprecatedJdkRevisionEquivalentToMaxJavaClassVersionInJarData() throws Exception {
JarData jarData = getJarData("helloworld-1.8.jar");
JarClasses jclass = jarData.getJarClasses();

String expectedMaxVersion = "1.8";

assertEquals(expectedMaxVersion, jarData.getJdkRevision());
assertEquals(expectedMaxVersion, jarData.getMaxJavaClassVersion());
assertEquals(expectedMaxVersion, jclass.getJdkRevision());
assertEquals(expectedMaxVersion, jclass.getMaxJavaClassVersion());
}

private void assertEntriesContains(List<JarEntry> list, final String entryToFind) {
assertTrue(list.stream().anyMatch(entry -> entry.getName().equals(entryToFind)));
}
Expand Down