Deprecate JdkVersion (for optimistic compatibility with newer JDK generations) [SPR-13312]

[spring-projects-issues]

Michał Sobkiewicz opened SPR-13312 and commented

After hitting something similar to #16518 (with legacy code using Spring 2.5), I'd like to suggest some refactoring. There will be similar bugs in the future because of how org.springframework.core.JdkVersion is designed. It breaks when you use JVM which was not included in constants.

If full version information is needed at runtime (which I doubt), see
org.apache.maven.artifact.versioning.ComparableVersion as an example of how to handle versioning properly. However, if checking runtime compatibility is the only thing needed (which is the case, I think), it can be done in a much simpler way.

I attached my own helper, JvmVersion. It provides everything that is necessary "to allow for automatically adapting to the present platform's capabilities". I know that there were no enums before Java 5, but, well, Java 5 is over 10 years old, Spring 4 requires Java 6. As you can see, JvmVersion delegates to Package#isCompatibleWith(String) - so you don't have to handle system properties or dot notation manually - it just works. You could refactor existing code and deprecate JdkVersion to avoid new bugs. It would make me very proud of myself ;-)

Sorry for not creating pull request - it's just one file... Hope you will find it useful anyway.

PS. See standardReflectionAvailable property in org.springframework.core.DefaultParameterNameDiscoverer (Spring 4.2) as an example of what can go wrong.

---

Affects: 4.2 GA

Attachments:

• JvmVersion.java (406 bytes)

Issue Links:

• Upgrade to AspectJ 1.9 beta 3 (for build compatibility with JDK 9 build 95+) [SPR-13839] #18412 Upgrade to AspectJ 1.9 beta 3 (for build compatibility with JDK 9 build 95+)

Referenced from: commits acb44f9, e0f012f, 27e9db8, 5e9a968, bec3b0f

[spring-projects-issues]

Michał Sobkiewicz commented

I forgot about example usage. Here it is:

if (JvmVersion.JAVA_1_8.isCompatibleWithRuntime()) {
  // ...
}

[spring-projects-issues]

Michał Sobkiewicz commented

Links to mentioned classes / methods:

• https://maven.apache.org/ref/3.1.0/maven-artifact/apidocs/org/apache/maven/artifact/versioning/ComparableVersion.html
• https://docs.oracle.com/javase/8/docs/api/java/lang/Package.html#isCompatibleWith-java.lang.String-
• https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/DefaultParameterNameDiscoverer.java
• https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/JdkVersion.java

[spring-projects-issues]

Stéphane Nicoll commented

I am not sure I understand the link between your issue and #16518. You're not expecting us to recognize a JDK version that a) did not exist when we released that version of the framework and b) does not work at all with said version.

What's a point of being able to recognize Java 8 using Spring 2.5 if you can't effectively run a Spring 2.5-based app with Java 8?

[spring-projects-issues]

Michał Sobkiewicz commented

My point is that nothing should prevent you from running Spring 2.5 with Java 8 or using Spring 4.2 optional features designed for Java 8 with Java 10.
The link between my issue and #16518 is as follows: JdkVersion was buggy, #16518 appeared because of that, nothing changed, there was no conclusions, JdkVersion is still buggy.

a) I'm not expecting you to recognize JDK versions that didn't exist when framework was released. It's rather what you are trying to do. Obviously, you can't create constant for versions that are not yet known (let them be static, enums, whatever). I'm trying to show you that code based on JdkVersion won't work properly on future JVMs, just because it has no constant for future JVMs - it can't - and that's the point! Instead of comparing constants, you should test if one of known versions is compatible with the current runtime - but without creating constant for current. That's all you need - I suppose - and JvmVersion does exactly and only that (it has just one method, it can't be simpler). If you really need more, please try to develop something similar to ComparableVersion from maven.

b) If it doesn't work because you use language features that was not present at the time of releasing framework in that framework (enums, try-withresources, lambdas, modules, value-based classes, ...), it's ok. I don't mind. You can't do anything with that. But if it breaks because something like bug or bad design, that's another story.

Suppose Spring 4.2 is binary compatible with Java 10. It's likely to be true as Java versions are expected to be binary backwards-compatible. Now look at:

1. https://github.com/spring-projects/spring-framework/blob/v4.2.0.RELEASE/spring-core/src/main/java/org/springframework/core/DefaultParameterNameDiscoverer.java#L35 and
2. https://github.com/spring-projects/spring-framework/blob/v4.2.0.RELEASE/spring-core/src/main/java/org/springframework/core/JdkVersion.java#L87
   It will break, without a doubt. At least one Spring 4.2 feature will not work with Java 10, even if it could, because of a simple bug. Maybe other. This is the only one I'm aware of. You can prevent similar bugs in the future easily.

By the way, another source of potential bugs is following assumption:
3) https://github.com/spring-projects/spring-framework/blob/v4.2.0.RELEASE/spring-core/src/main/java/org/springframework/core/JdkVersion.java#L75
How about "1.10.1.7-r25"? Is it 1.6? 1.7? 1.10? Or maybe it depends?

It's not about Spring 2.5 really. We are switching to String 3.2.9 or Spring 4.2 anyway. It's about reliable, error-free code.

[spring-projects-issues]

Juergen Hoeller commented

In the end, JdkVersion is an outdated class which we should get rid of. In addition to your points (which I generally agree with), JDK 9 is likely to have a different version scheme ("9.x.x" instead of "1.9.x), so this whole version string parsing is even more fragile. We need to avoid this completely.

As a consequence, let me repurpose this issue towards deprecating JdkVersion, getting rid of remaining use of JdkVersion in our codebase. We generally try to detect the specific API that we're trying to use, so DefaultParameterNameDiscoverer can simply check for the presence of java.lang.reflect.Executable (which is only there on JDK 8+). We're doing that in most places already; DefaultParameterNameDiscoverer is just one of three remaining JdkVersion checks in the entire framework codebase. The goal is for there to be no such checks anymore as of 4.2.1.

Juergen

[spring-projects-issues]

Michał Sobkiewicz commented

Makes sense. I'll check 4.2.1 then, just to sleep well, as soon as it's released.
Thank you!