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

JSON-serialization causes "Illegal reflective access" warning on JDK 9 #1216

Closed
emulov opened this Issue Dec 29, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@emulov

emulov commented Dec 29, 2017

Hi,

Since upgrading to JDK 9 (Gson v2.8.1), we keep getting warning messages like the following when doing JSON-serialization using Gson:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.gson.internal.ConstructorConstructor (./lib/gson-2.8.1.jar) to constructor java.text.NumberFormat()
WARNING: Please consider reporting this to the maintainers of com.google.gson.internal.ConstructorConstructor
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

It seems that with the introduction of modularization in Java 9, reflective access is only permitted to classes within the same module. However, Gson apparently accesses classes in the module java.base (in this example java.text.NumberFormat) which causes above warning message (by default, it is only displayed once on the first illegal reflective access). Since illegal access of this form will be prohibited in future releases (probably in Java 10), and because of this annoying warning message, this should be fixed as soon as possible.

Thanks!

@emulov emulov changed the title from JSON-serialization causes "Illegal reflective access" warning on JDK 9: to JSON-serialization causes "Illegal reflective access" warning on JDK 9 Dec 29, 2017

@inder123

This comment has been minimized.

Collaborator

inder123 commented Dec 29, 2017

Can you propose a solution, or may be send a PR? Thanks.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Dec 29, 2017

@inder123

This comment has been minimized.

Collaborator

inder123 commented Dec 29, 2017

@JakeWharton Do we do that somewhere in Gson? If so, we should fix that.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Dec 29, 2017

@inder123

This comment has been minimized.

Collaborator

inder123 commented Dec 29, 2017

I see. One solution will be to explicitly add type-adapters for each of the java.* classes in Gson.
But we will only be able to do that for classes up to JDK 6 (since we can't include classes for JDK 7+ given JDK 6 being the minimally supported version).

Your suggestion to @emulov is right: don't serialize java.* classes without writing an explicit type-adapter.

@emulov if you do write type adapters for java.* classes (especially JDK 9), feel free to contribute them back in the extras package for the benefit of others.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Dec 29, 2017

@amogilev

This comment has been minimized.

Contributor

amogilev commented Dec 30, 2017

The current trend for avoiding "illegal reflective access" warnings and errors is using Unsafe for setting "accessible" flag. See https://github.com/amogilev/j9-reflection-utils for details.
If you are interested, I can make a pull request with changes similar to amogilev/yagson@4713522

@inder123

This comment has been minimized.

Collaborator

inder123 commented Dec 30, 2017

@amogilev Yes, that would work.

  1. Please don't add a maven dependency to j9-reflection-utils, just copy over the minimal set of classes.
  2. Please ensure compatibility with JDk 6 onwards
    Thanks
@emulov

This comment has been minimized.

emulov commented Jan 1, 2018

Thanks for all replies.

@JakeWharton @inder123 Your are right when you stated that developers should not serialize internal java.* classes. However, I don't think it is a good idea to prevent such serialization in the future for the following reasons:

  • This policy should have been enforced right away from the beginning, when introducing Gson. I am sure that now, many developers already have code serializing internal java.* classes and strictly preventing this in a future version of Gson might break their code (even though you would do this for the greater good). It is the responsibility of the developers who have this practice to keep an eye on the compatibility of their serialized java.* classes and adjustments must be made on their side when java.* classes do in fact change.

  • Also, we use Gson many times to quickly serialize an object in memory to perform some analysis on the data (e. g. using JSON path), or converting it to other formats (e. g. to XML using org.json.XML) without persisting or deserializing the data at all. In such cases, I think it very much makes sense to perform the serialization of java.* classes, just as it was done till now. Strictly preventing in the current state would be counter-productive imo.

@amogilev @inder123 That seems be a good fix and also guarantees backwards-compatibility and the same semantics Gson had till now. Please let us know when it has been implemented.

Thanks a lot for your effort!

@amogilev

This comment has been minimized.

Contributor

amogilev commented Jan 1, 2018

@emulov The pull request with the fix is ready, so there is a chance that it will be in the next Gson release.
However, I am not quite sure that you example would work with it. At first, NumberFormat is an abstract class, so one cannot deserialize to it. Secondly, if your actual instance is DecimalFormat, that class cannot be serialized because of multiple fields named maximumIntegerDigits (and my fix which supported multiple fields with the same name was rejected recently)

@emulov

This comment has been minimized.

emulov commented Jan 1, 2018

@amogilev Thanks for the fix, looking forward to use the new version!

I understand what you're saying and the mentioned case with NumberFormat actually corresponds to the second use case I mentioned in my last post, so deserialization does never take place. It probably wasn't the best example for triggering the warning message. The object just needs to be serialized to analyze some actual data fields of the class and this worked well without any warning message till now. In general, I was rather referring before to classes like LocalDate, LocalDateTime, etc., which might change in the future (but probably won't) and should be allowed to be serialized and deserialized, at the risk of the developer. In case the classes are changed, deserialization should take an eager approach by eagerly deserializing the matching fields and not initializing non-matching fields. I do think that this is how Gson currently works anyways, but correct me if I'm wrong.

Thanks again!

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Jan 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment