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
fixed unmarshaling van LocalDateTime error in the DDIExporterTest.java #8517
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't reproduce the issue on Java 11. If the goal is to support Java 17, I'm wondering if upgrading Gson is cleaner (less boilerplate). @ErykKul can you please give this a try? I left a few more comments in my review. Thanks!
@@ -1,6 +1,6 @@ | |||
package edu.harvard.iq.dataverse.export; | |||
|
|||
import com.google.gson.Gson; | |||
import com.google.gson.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a long time we've been discouraging wildcard imports but there was some recent discussion on reconsidering this position. Nothing is written at https://guides.dataverse.org/en/5.10/developers/coding-style.html either way. 😄
private static final Gson gson = new GsonBuilder().registerTypeAdapter(LocalDate.class, (JsonDeserializer<LocalDateTime>) (JsonElement json, Type type, JsonDeserializationContext jsonDeserializationContext) -> { | ||
Instant instant = Instant.ofEpochMilli(json.getAsJsonPrimitive().getAsLong()); | ||
return LocalDateTime.ofInstant(instant, ZoneId.systemDefault()); | ||
}).create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this test on my laptop and it works fine. It seems like a lot of boilerplate code.
I noticed that the InaccessibleObjectException
error was first reported by @poikilotherm in an issue about Java 17:
@ErykKul are you running something newer than Java 11? In that issue, @poikilotherm suggests at upgrading Gson might fix this and what is perhaps a related issued here:
I can't reproduce the problem on Java 11 and haven't tried on anything newer. We aren't actively working on that Java 17 issue above but this pull request seems like a step in the right direction. I'm wondering, however, can you please try upgrading Gson to see if that's also a suitable fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ErykKul I appreciate you checking if upgrading Gson helps. Like I said, the tests still run fine on my laptop with your original change. They also run fine in CI. It's a little more boilerplate but it's confined to one test. If this helps us get to Java 17, great. Thanks for the pull request!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. As an alternative, we tried upgrading Gson but it didn't help.
private static final Gson gson = new GsonBuilder().registerTypeAdapter(LocalDate.class, (JsonDeserializer<LocalDateTime>) (JsonElement json, Type type, JsonDeserializationContext jsonDeserializationContext) -> { | ||
Instant instant = Instant.ofEpochMilli(json.getAsJsonPrimitive().getAsLong()); | ||
return LocalDateTime.ofInstant(instant, ZoneId.systemDefault()); | ||
}).create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ErykKul I appreciate you checking if upgrading Gson helps. Like I said, the tests still run fine on my laptop with your original change. They also run fine in CI. It's a little more boilerplate but it's confined to one test. If this helps us get to Java 17, great. Thanks for the pull request!
Which issue(s) this PR closes: