-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Error reflection JDK 17 and gson #1979
Comments
Similar to #1963 and other existing reports. Apparently you are deserializing a JDK class ( The reason why this is causing an exception for JDK 17 is because JDK internals are now strongly encapsulated (see JEP 403). Note that I am not a maintainer of this project. |
We are seeing another example of consequences from "JEP 403: Strongly Encapsulate JDK Internals" in Gson code. We are returning array type from the API:
The code in GerritCodeReview can return private Collection<String> reviewed(RevisionResource resource) throws AuthException {
CurrentUser user = self.get();
if (!user.isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
Account.Id userId = user.getAccountId();
PatchSet patchSetId = resource.getPatchSet();
Optional<PatchSetWithReviewedFiles> o;
o = accountPatchReviewStore.call(s -> s.findReviewed(patchSetId.id(), userId));
if (o.isPresent()) {
PatchSetWithReviewedFiles res = o.get();
if (res.patchSetId().equals(patchSetId.id())) {
return res.files();
}
try {
return copy(res.files(), res.patchSetId(), resource, userId);
} catch (IOException | DiffNotAvailableException e) {
logger.atWarning().withCause(e).log("Cannot copy patch review flags");
}
}
return Collections.emptyList();
} Now, the Gson serialization started to fail on JDK 17:
This patch fixed it: diff --git a/java/com/google/gerrit/server/restapi/change/Files.java b/java/com/google/gerrit/server/restapi/change/Files.java
index e9961695c3..ab57052fa5 100644
--- a/java/com/google/gerrit/server/restapi/change/Files.java
+++ b/java/com/google/gerrit/server/restapi/change/Files.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.change;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
@@ -58,7 +59,6 @@ import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -260,7 +260,7 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
}
}
- return Collections.emptyList();
+ return ImmutableList.of();
}
private List<String> copy(
I wonder this is the right thing to fix all Gson clients to abandon the usage of Update Next failure is in
|
@Marcono1234 Thanks for the pointer! I have built However, I still see complication with
I would have expected that See this upstream change: [1]. [1] https://gerrit-review.googlesource.com/c/gerrit/+/327739 |
That is great to hear!
Unfortunately Gson still has Java 6 (soon Java 7) as minimum supported Java version. It does not provide a default adapter for Note that I am not a member of this project. |
Are there any plans to add an option to not register the |
@arouel, I have created a pull request a while ago which would allow filtering for which classes the default reflection based adapter should be used, so it should also be possible to implement a filter which blocks all reflection based access. But I do not know what the maintainers think about this feature. |
I'm definitely sympathetic to making it possible to restrict the use of reflection. It's a major source of unpredictable behaviour and I think we should probably recommend that people restrict it by default, or at least restrict the use of platform classes (via #1905). |
I added --add-opens java.base/java.lang=ALL-UNNAMED but still have this exception
|
@a364176773, as mentioned in #1979 (comment), Gson has no built-in type adapter for @eamonnmcmanus, do you think we can close this issue? As pointed out in the previous comments there is nothing Gson can / should do to solve this issue in my opinion. Users have to write a custom type adapter.
Do you think we should in the future configure Gson to block access to platform classes by default? (users could still overwrite it by specifying a custom reflection access filter) |
alibaba/fastjson#4077 |
If I understand it correctly, that GitHub issue is mainly about how the exception is handled, but it does not allow you either to access JDK internals. The main difference is that fastjson seems to provide built-in codecs for Either way, this is probably getting off-topic. My point was that Gson (and most likely any other object mapper) cannot access internal JDK fields using reflection (JEP 403), which is the topic of this GitHub issue. Whether Gson should provide built-in adapters for more JDK types is a different topic and already tracked in separate GitHub issues. |
I got it. thx |
It may be a good idea to include java.time adapters in the library by default. I find myself having to copy the same adapters across multiple projects and use a static utility to construct the Gson object. I understand you guys want to also support Java 7, but this really is a pain point. |
It might make sense for us to have a separate artifact that includes |
So, how to deal with the startup failure caused by gson? Is there a solution |
Caused by: java.lang.ExceptionInInitializerError: null |
@yexiaobin909090 The specific error that you are running into is the one that is and fixed here: googleapis/java-spanner#1426 Upgrading your Spanner client to at least version 6.12.4 should fix this problem. |
Thank you very much. It's effective to upgrade to 6.12.5 |
Hi.
I'm using gson (version 2.8.8), and only with JDK 17 this code:
throws this exception:
The same code works with previous JDK's.
The text was updated successfully, but these errors were encountered: