Java 17 Compatibility

[nicky9door]

These are the minimum changes needed for Java 17 to address the reflection issues specified in #173

I think only the first commit is actually needed to handle the reflection issues. The other commits were necessary to build using jdk 17. They could be removed but I left them in case it makes sense to create a separate java 17 specific artifact

A couple notes:

• Non-public members are ignored for classes under 'java.' and 'javax.' packages
• Since Java 11(?), getResourceAsStream only looks within the module of the class which caused a couple tests to fail. Using getSystemResourceAsStream seems to work
• ASM needed to be updated to Opcode version 7 (minimum needed for Java 11). Note that Opcode 8 and 9 is available
• Scala needed to be updated in order to compile. I updated to version 2.12. Version 2.13 is more current but it removed CanBuildFrom and I have too little knowledge of Scala to know the proper way of updating the code

[EugenCepoi]

I think this is not enough. The current impl of isVisible does apply a filter on things like transient, static, volatile, etc. You probably still want to leverage the same filter logic. It's worth adding some tests too.

[nicky9door]

I could probably reverse the steps. Run the existing visibility check first, then the additional check for java/javax class afterwards

[EugenCepoi]

Yep this sounds like a good option.

[EugenCepoi]

Hey @nicky9door, thanks for going ahead with this PR!

I think for now it would be good to still stay on earlier versions of Java for better compatibility with more users. I know also that lots of large companies are still on Scala 2.11 and Java 11 or so. That said those changes are useful. So you could split them. Have one PR for the actual change with some unit tests. And another PR that we can keep around for the Java and Scala changes.

What do you think?

[nicky9door]

I think the earliest version of Java that this could be tested with would be Java 9. There is a JVM option --illegal-access which is by default set to 'permit' up to Java 15. The default changed to deny in Java 16. In java 17, this option was removed and 'deny' became the default behaviour. So I think that as long as we can set the jvm flag to 'deny' then we can test the access behaviour as it is implemented in Java 17. I'm not really sure how this would be implemented short of having to have multiple build runs with the different command line options (possibly using multiple maven profiles?)

The other alternative is to have a separate artifact for genson that contains this change (i.e. genson-jdk17) that would be built and deployed independently. This is probably more time consuming and while I don't mind trying to help, it might make more sense to see if, and to what degree others are affected by this issue. If the access exceptions are only occurring on a small scale and can be avoided using filters/excludes, it might not be worth going through this complicated process

[EugenCepoi]

I guess you did check that it worked by trying to ser/de some JDK types in Java 17, so you know it works. I think you could write a unit test to ensure that for some JDK type we resolve only the public fields, which is what we want. Then we don't need Java 17.

I agree that splitting it in a separate module is an option but not worth it IMO.

[nicky9door]

Yeah, its kind of hard because the reflection code hasn't changed and it still works until java 16/17. And even then it still works except for java/javax classes.

FYI, without the visibility filter change, the test testASMResolverShouldNotFailWhenUsingBootstrapClassloader in JsonDeserializationTest throws the following exception on java 17:

java.lang.reflect.InaccessibleObjectException: Unable to make final void java.lang.Throwable.setCause(java.lang.Throwable) accessible: module java.base does not "opens java.lang" to unnamed module @d7b1517

This is because setCause is package private. But I don't really know how this would be testable because the visibility filter would either be on and the exception never gets thrown so the test passes

There are a couple of other things that I've considered:

• Making the visibility filter conditional on the java version. This would involve checking the system properties for the java version. This should be relatively safe but still kind of dependent on the JVM implementation
• Instead of using the visibility filter, we can wrap the calls to setAccessbile in a try-catch. This might be slightly better in terms of preserving behaviour but ignoring exceptions feels like a bit of a code smell

[EugenCepoi]

Ah cool so that test does hit the issue you're fixing for Java 17 and by fixing the filter it does fix the test, so it's all good. Update the PR with the fixed impl and remove the Java/Scala stuff and we can go ahead and merge it.

[nicky9door]

Done... closing this PR. I have provided two options in #177 and #178. Use whichever you prefer :)