Skip to content
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.

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

Make dependency on java.sql optional #1707

Merged
merged 6 commits into from Aug 25, 2021
Merged

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented May 24, 2020

Note: This is based on #1656
Fixes #1629

Makes the dependency on the java.sql module optional. If its classes are present at runtime, the respective type adapters will be used, otherwise Gson will not try to load the classes and will therefore not fail with a ClassNotFoundException anymore. The entry point for all SQL type adapters is now SqlTypesSupport.

I am however, not that familiar with the module system so it would be good if others who are more familiar with it can check if this is a sane implementation.

Especially #1500 (@Degubi) is confusing me because from what I understood about the JPMS requires transitive is only so modules depending on Gson would also be "reading" java.sql, which however makes no sense because Gson does not expose these types publicly anywhere, so it should not provide them transitively. To my understanding requires should have been enough to require java.sql being present at runtime, however based on the ClassNotFoundException reports this was not the case?
Or are they using tools (as it is the case here which break the module system)? Though that would be a rather common issue then, see the question "java.sql.Time exception".

Also pinging @nicolaiparlog, if you have any spare time, any hints regarding the JPMS would be appreciated :)
Sorry to pester you in case you don't have any time.

@nipafx
Copy link

@nipafx nipafx commented May 26, 2020

Don't worry, I don't pester easily. 😉 I'm happy to provide input, but keep in mind that I don't know GSON or the diff in detail (don't have the time to study them right now), so I may be off. Caveat emptor, I guess. Also, I wrote a blog post about optional dependencies that may help understand the feature.

As I understand the situation, GSON only needs to work with java.sql types if the user already uses them, right? I.e. there's never a situation where a GSON user doesn't need the java.sql module, but GSON uses its types anyway. If that is so, requires static is the right way to go.

Note that requires static alone does not make the module available at run time. That means, even if GSON is launched on a Java runtime that contains the module, it will only be available at run time if some other module requires it (or the app is launched from the class path).

In case you're interested, the blog post shows a method isModulePresent(String) that, well, does what the name suggests. 😁 You can use it (I hereby provide it under the Apache Software License 2.0) if you need to programmatically determine whether java.sql made it into the module graph.

I agree with your confusion about #1500. Turning requires into requires transitive doesn't make a difference regarding its presence at run time - it is present either way (another vlog post). I agree with Alan Bateman's comment - this looks like class loader shenanigans to me.

@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented May 26, 2020

Thanks for your clarification! Yes Gson does not need any java.sql types on its own, it only provides (de-)serialization support for them in case they are used.

Your isModulePresent(String) is quite verbose for what it tries to solve. Though maybe there is no better / more complete way of doing this (I have created a StackOverflow question for that). However, Gson cannot use this solution because it wants to support Java 6, so this pull requests checks using Class.forName for a class present in java.sql, assuming that the other classes must be present then as well:

Class.forName("java.sql.Date");
sqlTypesSupport = true;
} catch (ClassNotFoundException classNotFoundException) {
sqlTypesSupport = false;

Regarding previously reported ClassNotFoundException and class loader shenanigans: It looks like Gson itself does not create or uses any special class loaders, so I assume the callers have a broken setup then.

@mP1
Copy link

@mP1 mP1 commented Aug 4, 2020

Heres another example of the same problem this time with CLosureCompiler, so+++ from me as well.

[INFO]         java.lang.SecurityException: Prohibited package name: java.sql
[INFO]         	at java.base/java.lang.ClassLoader.preDefineClass(ClassLoader.java:889)
[INFO]         	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1005)
[INFO]         	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
[INFO]         	at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:545)
[INFO]         	at java.base/java.net.URLClassLoader.access$100(URLClassLoader.java:83)
[INFO]         	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:453)
[INFO]         	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:447)
[INFO]         	at java.base/java.security.AccessController.doPrivileged(Native Method)
[INFO]         	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:446)
[INFO]         	at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClassFromSelf(ClassRealm.java:425)
[INFO]         	at org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy.loadClass(SelfFirstStrategy.java:42)
[INFO]         	at org.codehaus.plexus.classworlds.realm.ClassRealm.unsynchronizedLoadClass(ClassRealm.java:271)
[INFO]         	at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass(ClassRealm.java:247)
[INFO]         	at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass(ClassRealm.java:239)
[INFO]         	at com.google.gson.Gson.<init>(Gson.java:265)
[INFO]         	at com.google.gson.Gson.<init>(Gson.java:186)
[INFO]         	at com.google.javascript.jscomp.AbstractCommandLineRunner.<init>(AbstractCommandLineRunner.java:224)

@neilwightman-intergral
Copy link

@neilwightman-intergral neilwightman-intergral commented Aug 13, 2020

This pull request worked great for me in a java agent (which cant use java.sql)

When building it I got this test failure though :

Failed tests: testDateFormatString(com.google.gson.internal.bind.util.ISO8601UtilsTest): expected:<2018-06-2[5]> but was:<2018-06-2[4]>

I'm wondering if this is a timezone issue.
Tests on master branch passed.

TZ="Europe/Berlin"
Ubuntu 20.04

@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Aug 13, 2020

Yes the ISO8601UtilsTest is faulty and will be fixed by #1665 / #1687

@concision
Copy link

@concision concision commented Sep 22, 2020

Thanks for your work on resolving this issue with this pull request, @Marcono1234.

@inder123, Is it possible we could get a review on this PR?

When using the jlink CLI tool to build a custom minimal JRE, gson's explicit module dependency adds another 10MB+ of Java module dependencies. A prominent use case of using jlink is building lean Docker images; however, this explicit SQL dependency adds dead weight to the final image (a 20% size increase for my project). Intentionally removing the java.sql module results in a runtime error of java.lang.NoClassDefFoundError: java/sql/Time, despite it not being necessary for execution.

The indirect/transitive module dependencies that end up being added are listed here: https://docs.oracle.com/javase/9/docs/api/java.sql-summary.html

I would really appreciate seeing the java.sql dependency being optional. Thank you for your time.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 25, 2021

I ran this against all of Google's internal tests that depend (directly or indirectly) on Gson, and didn't see any problems. I think this is good to go.

@eamonnmcmanus eamonnmcmanus merged commit 1023f0f into google:master Aug 25, 2021
3 checks passed
@Marcono1234 Marcono1234 deleted the optional-sql branch Aug 25, 2021
@Marcono1234 Marcono1234 mentioned this pull request Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants