Add module system support

[XakepSDK]

Changes

1. Added java module system support
2. Bumped compiler version to Java 11
3. Compiling code with java 8 target
4. Compiling module info class with java 9 target

Compiler warns about 0 in module component (auth0), module name components can't end with digits. Not sure how to deal with this.

References

#483

Testing

• This change adds test coverage
• This change has been tested on the latest version of Java or why not

Checklist

• I have read the Auth0 general contribution guidelines
• I have read the Auth0 Code of Conduct
• All existing and new tests complete without errors

[XakepSDK]

I think this could be made in more clean manner, e.g. putting module-info.java in separate directory like java9 instead of java
Only if this was maven, it supports this for years already...

I've tried doing this, but compiler was complaining that jackson is not in module path

[Marcono1234]

Compiler warns about 0 in module component (auth0), module name components can't end with digits. Not sure how to deal with this.

I assume you were also the author of the related JDK discuss thread where you already referred to the relevant JLS section:

The name of a module should correspond to the name of its principal exported package. If a module does not have such a package, or if for legacy reasons it must have a name that does not correspond to one of its exported packages, then its name should still start with the reversed form of an Internet domain with which its author is associated.

(Though this is only a convention and not required)

Since both the principal exported package is com.auth0.jwt, and the domain of the author is auth0.com, using com.auth0.jwt as module name seems (in my opinion) to be the right choice.
The javac warning is apparently created to prevent users from encoding version information in module names (see JDK-8176802 and JDK-8216185), however the warning message is pretty imprecise / irritating.
Since here the intention is not to encode version information, and this restriction is not part of the JLS, using auth0 should probably be fine.

Your changes currently put the module-info.class in the top level directory of the JAR. This can cause issues for tools processing the content since the module-info.class was compiled with Java 9 (see for example Gson issue where this is causing issues). It might be good to either create a Multi Release JAR (and put the class file in the Java 9 folder) or maybe create a separate artifact variant using Gradle.
There is a Gradle blog post about this, but I don't know if Gradle supports 'variants' as described in that post yet (and I am not very familiar with Gradle).

[Marcono1234]

Maybe for the next major release the export for impl should be removed? See #487
What do you maintainers think about this?

[XakepSDK]

Removing it from opens will completely hide it from library users, it will break code if somebody depends on it
If it's for major release, then this could be changed right before major release, but i want to use this library with module system as soon as possible.

[XakepSDK]

I have made it multi-release jar, so module-info.class is in META-INF/versions/9, but module-info.java is still in the root of the sources, I've tried adding different sourceSet, but then it complains about dependencies and module-info can't see packages in main sourceSet.

[XakepSDK]

Ready to merge if you are ok with how it's achieved

[jimmyjames]

We will be looking at this change this week. If it's something we want to add, we'll need to make sure the CI failures are addressed. Thanks, we'll follow up later this week.

[XakepSDK]

At first it failed because docker hub gave HTTP 50x error, now it failed because gradle download speed was very low, maybe it will work on third re-run. 🤷‍♂️

[jimmyjames]

Change looks good - I don't love creating a multi-release jar, but as @Marcono1234 points out, without it there could be issues with tooling. I'm not sure how likely that is with this library, as it is not intended for Android use, and I see other libraries like Jackson include the module-info.class at the top-level. Going to do some more testing locally before merging, but appreciate the contribution here!

[jimmyjames]

@XakepSDK can you rebase your changes on the latest master, so we can get this change in to the next release? If not, we can plan to do it, though it may not make it into the next release. Thanks again for the contribution!

[XakepSDK]

I've tried and gradle refuses to work properly:

java.lang.IllegalArgumentException: Unsupported class file major version 60
at groovyjarjarasm.asm.ClassReader.(ClassReader.java:196)

Looks like i have to use gradle 7.0, but some plugin uses deprecated features...

1. Run gradle with java 11 and have Java 16 installed -> Could not target platform: 'Java SE 16' using tool chain: 'JDK 11 (11)'.
2. Run gradle with java 16 -> that IllegalArgumentException and module encapsulation violation (can be worked around with add-opens)

I think, i can make it work, but with Java 11, not Java 16

[Marcono1234]

@XakepSDK, are you using gradlew or a local Gradle installation?
With gradlew I am able to build to project (see this branch; your changes rebased onto master).

[jimmyjames]

@XakepSDK do you need some help getting this rebased with the latest changes?

[XakepSDK]

Should i hide impl? New package-info let's me do this.
UPD: i removed impl package export

[Marcono1234]

UPD: i removed impl package export

To the maintainers: It might be good to explicitly mention this in the changelog since it can be a breaking change for users of the module system which previously accessed classes from that package.

[jimmyjames]

To the maintainers: It might be good to explicitly mention this in the changelog since it can be a breaking change for users of the module system which previously accessed classes from that package.

Why not keep the impl package as exported, as you discussed above? Like you mentioned, we could consider removing exporting that package in a future major release.

[XakepSDK]

Why not keep the impl package as exported, as you discussed above?

Then jackson will become transitive dependency, because impl classes expose jackson classes

This change shouldn't affect non-modularized projects.

[jimmyjames]

Why not keep the impl package as exported, as you discussed above?

Then jackson will become transitive dependency, because impl classes expose jackson classes

This change shouldn't affect non-modularized projects.

I understand it would be best to not export the impl package, and only impacts modularized projects, but we still do not want to introduce a breaking change. If someone were using this library in a modularized project, what would be the specific breaking change, and how would they address it? If this change cannot be done in a non-breaking way to provide value, then it's something we would consider for the next major version, but not in v3.

[XakepSDK]

If someone were using this library in a modularized project, what would be the specific breaking change

Package impl will be invisible for that project

how would they address it

1. Depends on usage, easiest is
   Do not use impl package

2. If their modularied library depends on this library and JVM is started in modularized mode, then
   Add --add-exports com.auth0.jwt/com.auth0.jwt.impl=projectModule, where projectModule is module name of dependent code

3. If their non-modularized library depends on this library and JVM is started in modularized mode, then
   Add --add-exports com.auth0.jwt/com.auth0.jwt.impl=ALL-UNNAMED

4. Start JVM in non-modularized mode, it will work as if module system doesn't exist

[jimmyjames]

If someone were using this library in a modularized project, what would be the specific breaking change

Package impl will be invisible for that project

how would they address it

1. Depends on usage, easiest is
   Do not use impl package
2. If their modularied library depends on this library and JVM is started in modularized mode, then
   Add --add-exports com.auth0.jwt/com.auth0.jwt.impl=projectModule, where projectModule is module name of dependent code
3. If their non-modularized library depends on this library and JVM is started in modularized mode, then
   Add --add-exports com.auth0.jwt/com.auth0.jwt.impl=ALL-UNNAMED
4. Start JVM in non-modularized mode, it will work as if module system doesn't exist

Thanks for those details. Given the change to not export the impl package may require changes for existing usages, we shouldn't make that change now. If you'd like to update the PR to export the package, we can re-review and look to add the change in the 3.x release stream.

[overheadhunter]

Thanks for those details. Given the change to not export the impl package may require changes for existing usages, we shouldn't make that change now. If you'd like to update the PR to export the package, we can re-review and look to add the change in the 3.x release stream.

I would very much like the latter option (update PR instead of further delays): For 2.x, add jackson as a transitive requirement and export impl. For 3.x clean up the module descriptor as originally proposed by @XakepSDK.

[XakepSDK]

Done. Exported impl and made jackson transitive.

[lbalmaceda]

@XakepSDK I'm looking at this PR alongside @jimmyjames. Java Modules are fairly new to us and we've tried a few different combinations of parameters to test this PR with an app that was making use of "internal" classes. Here are our findings:

JDK 8

Targetting JDK 8 and thus not using modules, didn't cause any issues with our code.

// build.gradle
sourceCompatibility = 8
targetCompatibility = 8

JDK 9

With Java 9 we should have access to java modules. We declare a src/java/module-info.java file with the following content:

// src/java/module-info.java
module org.example {
    requires com.auth0.jwt;
}

Changing the source+target JDK to 9 caused the app to not compile but throw an "error: module not found: com.auth0.jwt".

// build.gradle
sourceCompatibility = 9
targetCompatibility = 9

@jimmyjames pointed out there was a line needed in the configuration to make it not throw that error. We added the following lines to our build.gradle file.

// build.gradle
java {
    // without it, the module com.auth0.jwt is not found
    modularity.inferModulePath = true
}

But that caused the Jackson module (transitive, as declared in this PR's module-info file) to not be found: "error: module not found: com.fasterxml.jackson.databind". Of course, adding the databind dependency in the build.gradle file solves it, but we expect this library's exported module to take care of that for us, because of the "transitive" scope/directive on Jackson.

We also found that we could use the "java.toolchain" DSL to specify the version. We did that instead of using the 2 lines we used before.

java {
    toolchain {
        languageVersion = JavaLanguageVersion.of(9)
    }
    modularity.inferModulePath = true
}

Unfortunately, the result was the same. The Jackson dependency would still be required as an explicit dependency added in the "dependencies" block.

Targetting JDK 11

We tested changing the java.toolchain.languageVersion to 11, like you do in this PR, and that makes it work. The app finds the JWT module and compiles correctly without us having to declare Jackson's dependency in the build.gradle file.

java {
    toolchain {
        languageVersion = JavaLanguageVersion.of(11)
    }
    modularity.inferModulePath = true
}

Our open questions:

1. Why do we need to add the modularity.inferModulePath = true for our own module to be found?
2. Why does targeting JDK 9 causes transitive modules not to be found?
3. Why does it work correctly when JDK 11 is targeted?

We think it would be interesting if you can provide a sample app that you typically use to test module-enabled java apps. Attached you will find a zip with the one we crafted, with the characteristics detailed previously in the last scenario (JDK 11). It requires the changes on this branch exported to the local maven repository. You could do that by running ./gradlew clean assemble publishMavenJavaPublicationToMavenLocal.

To be clear, the PR diff looks OK, but we still want to perform some manual testing as this is completely new and out of our knowledge. Thanks!

jdk-modules-sample.zip

[XakepSDK]

1. Why do we need to add the modularity.inferModulePath = true for our own module to be found?
   Only Maven has proper module system support. Gradle 7.0 got some module system support by default. For Gradle < 7.0 you have to use this property or special plugin, for >= 7.0, it should work without that property.

2. Why does targeting JDK 9 causes transitive modules not to be found?
   I think this is because jackson is declared as implementation in build.gradle, while it should be api

3. Why does it work correctly when JDK 11 is targeted?
   I don't know. Probably another bug in gradle.

I pushed change from implementation to api. Could you please re-test?

[Marcono1234]

I pushed change from implementation to api.

Wouldn't this increase future incompatibilities? Usage of Jackson appears to be an internal implementation detail, if you now make it api then other projects might rely on java-jwt bringing Jackson, so if this is reverted again in the future it might cause more breaking changes for projects using java-jwt.

Unfortunately, the result was the same. The Jackson dependency would still be required as an explicit dependency added in the "dependencies" block.

I am not that familiar with the module system, but that is what I would expect for all Java versions. Maybe Gradle infers this information somehow, but the module system itself only defines the relationships between modules; it neither defines where the modules can be found, that is the job of the build tool (e.g. Maven, Gradle, or manually adding the JAR files), nor which version of a module is used, see also this StackOverflow question (actually it does encode the version, but you cannot directly specify versions of dependencies, see that StackOverflow question).

[XakepSDK]

Usage of Jackson appears to be an internal implementation detail

It has to be transitive and be api because of PayloadSerializer class.
If it's possible to make PayloadSerializer package-private, then jackson can be hidden.
I think only this class exposes jackson classes.

[overheadhunter]

other projects might rely on java-jwt bringing Jackson, so if this is reverted again in the future it might cause more breaking changes for projects using java-jwt

If downstream projects use jackson themselves, they should depend on it directly. Relying on never-changing transitive dependencies is a configuration error.

[lbalmaceda]

@XakepSDK Thanks for the pointers there! I've upgraded my test project's Gradle wrapper version to 7.1 and was able to remove the modularity.inferModulePath = true line without the "com.auth0.jwt module not found" error being surfaced. I'm also not required to specify the Jackson dependency as a dependency of my own project (I've not pulled the implementation -> api change you pushed). However, I still cannot make it work without the Jackson dependency if I target JDK 9. I'd like to give it a try using Maven instead of Gradle and see if there's any difference, would you mind sharing the pom file you are using?

It has to be transitive and be api because of PayloadSerializer class.

Devs shouldn't be using that class directly. Hopefully, a follow-up major can improve a bit the package structure and prevent these implementation details to be exposed. But that's a change that we cannot include in this next minor.

Earlier when we talked about exposing the "impl" package as part of the module, we agreed to keep it in case someone was using these classes today. Because, while them using it today is not our intention, it's still possible. If that's the case and there are devs out there using these classes directly, they must already be requiring the Jackson dependency on their own projects. Otherwise, they would be facing a "class not found" exception.

If we change the scope of the Jackson dependency from implementation into api, we would expose it even more and would cause a bigger breaking change the moment we decide to change to a different JSON serializer library. I agree with @Marcono1234 here; I say we keep it as implementation.

[XakepSDK]

However, I still cannot make it work without the Jackson dependency if I target JDK 9

Can you share your test project for jdk9 and error you are getting?

I'd like to give it a try using Maven instead of Gradle and see if there's any difference, would you mind sharing the pom file you are using?

Just install java-jwt in local repo and add it as a dependency, should work.

Since it's decided to not switch from implementation to api, then i make jackson not transitive in module descriptor too.

[lbalmaceda]

I made sure to have the latest changes from this PR and re-installed the java-jwt library locally using the following command.

./gradlew clean assemble publishMavenJavaPublicationToMavenLocal

jdk-modules-sample-mvn.zip

This is the Maven project I'm running locally. It's analogous to the Gradle one I shared in last week's comment above. For this one, I had to add the maven-compiler-plugin version 3.8.1 or above to the pom.xml file, otherwise, the error "module not found: com.auth0.jwt" was raised.

I'm not asked to add the Jackson dependency. But of course, if I want to make use of the PayloadSerializer class which requires these classes, the compiler warns me that the Jackson dependency is not part of the project. That's ok, that's what I'd have expected. This was is meant to be internal so any devs using it directly today will still need (this hasn't changed) to require the Jackson dependency explicitly 👍 !

This is how I'm running the project:

mvn clean install
mvn compile exec:java -Dexec.mainClass="org.example.App"

I've tried setting the compiler source and target to 9 and then to 11.

<properties>
    <!--    11 works as well    -->
    <maven.compiler.source>9</maven.compiler.source>
    <maven.compiler.target>9</maven.compiler.target>
</properties>

The output is always successful.

[INFO] --- exec-maven-plugin:3.0.0:java (default-cli) @ jdk-modules-sample-mvn ---
====> All good! 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

Summary of my findings using a Maven-based project:

• No need to include the Jackson dependency.
• Requires the maven-compile-plugin.
• Targetting 9 and 11 works OK. Targetting 8 works and doesn't make use of modules.

Taking these findings back to the Gradle project, for documentation purposes.

Summary of my findings using a Gradle-based project:

• No need to include the Jackson dependency.
• No need to specify modularity.inferModulePath.
• Targetting 9 and 11 works OK. Targetting 8 works and doesn't make use of modules.

[lbalmaceda]

@XakepSDK @overheadhunter @Marcono1234 @jimmyjames thanks for the good conversation! We will include this PR in the next minor release.

[overheadhunter]

Deferred again? 😢 never mind, seems like GitHub Releases and Maven are out of sync.