Add type adapters for java.time classes
#2948
Conversation
These adapters essentially freeze the JSON representation that `ReflectiveTypeAdapterFactory` established by default, based on the private fields of `java.time` classes. That's not a great representation, but it is understandable. Changing it to anything else would break compatibility with systems that are expecting the current format. If Gson had been updated with `java.time` adapters at the time the `java.time` API was added, the representation would surely have been something else, probably ISO standard formats. We can still supply non-default adapters for that, but we'll still need to have these legacy adapters by default. I've been meaning to make this change for a while, but the need to do so becomes more urgent with [this JDK commit](openjdk/jdk@cc05530) which makes a number of `java.time` fields `transient`. That means that the reflective-based adapter will no longer work with the classes that were changed.
I think it is time to do this, but obviously it should be a separate PR.
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("JavaDurationGetSecondsGetNano") |
There was a problem hiding this comment.
Huh, I would have hoped that the getSeconds and getNano calls below were close enough to avoid this warning :(
There was a problem hiding this comment.
I'm not sure what you mean? The warning is supposed to prevent you from calling getSeconds() when you meant toSeconds().
There was a problem hiding this comment.
https://errorprone.info/bugpattern/JavaDurationGetSecondsGetNano:
If you call duration.getNano(), you must also call duration.getSeconds() in ‘nearby’ code.
I get the impression that the check would be happy with foo(duration.getSeconds(), duration.getNano()) but not the array case. But I see no other examples in my depot search for ([{]|new.*\[)[^\042]*getSeconds.*getNano lang:java, so I don't think I'll bother to file a feature request.
(I forget where we landed with getSeconds vs. toSeconds. I just know that I got sad about the various get* and to* methods, including the to*Part methods, in b/201676318.)
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
| <groupId>net.sf.androidscents.signature</groupId> | ||
| <artifactId>android-api-level-21</artifactId> | ||
| <version>5.0.1_r2</version> | ||
| <artifactId>android-api-level-26</artifactId> |
There was a problem hiding this comment.
I saw that your commit description mentions that you were thinking about putting this in a separate PR.
Alternatively, you can introduce your own annotation for Animal Sniffer suppressions (likely copying the standard name, "IgnoreJRERequirement"), configure your build to respect it, and then suppress at the narrow scope so that we continue to test that we're compatible with 21 (or 23, since that's the typical Google default nowadays) except in code that we're careful to run only under newer versions.
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
* Move the new `TypeAdapter` implementations into a new class `JavaTimeTypeAdapters`. This allows us to omit that class from Android builds where it otherwise triggers warnings about SDK levels. * Ensure that every `java.time` class that needs a `TypeAdapter` has one. This doesn't include "subpackages" `java.time.*` like `java.time.chrono`, where it doesn't appear that there are any classes that are likely to be serialized to JSON. * Fix the handling of `ZoneId` and its subclasses.
* refactor: slightly optimize ConstructorConstructor * Update comments Co-authored-by: Marcono1234 <[email protected]> * spotless apply Co-authored-by: Marcono1234 <[email protected]> --------- Co-authored-by: Marcono1234 <[email protected]>
I copied the internal version that was running on Google's infrastructure, and some adjustments were needed.
This still doesn't pass with a local build, and I may need to debug an OSGi problem.
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("JavaDurationGetSecondsGetNano") |
There was a problem hiding this comment.
https://errorprone.info/bugpattern/JavaDurationGetSecondsGetNano:
If you call duration.getNano(), you must also call duration.getSeconds() in ‘nearby’ code.
I get the impression that the check would be happy with foo(duration.getSeconds(), duration.getNano()) but not the array case. But I see no other examples in my depot search for ([{]|new.*\[)[^\042]*getSeconds.*getNano lang:java, so I don't think I'll bother to file a feature request.
(I forget where we landed with getSeconds vs. toSeconds. I just know that I got sad about the various get* and to* methods, including the to*Part methods, in b/201676318.)
gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| // This is Math.toIntExact, but works on Android level 23. |
There was a problem hiding this comment.
toIntExact should work under any Android version, even if the user doesn't opt in to library desugaring, because it's one of the methods that is "backported" instead of just handled by opt-in desugaring.
https://github.com/open-toast/gummy-bears, already mentioned in your pom.xml, gets this right. Not that there is any harm in having our own helper method here; this sort of thing just may come up again in time.
| FactorySupplier supplier = | ||
| (FactorySupplier) javaTimeTypeAdapterFactoryClass.getDeclaredConstructor().newInstance(); | ||
| return supplier.get(); | ||
| } catch (ReflectiveOperationException e) { |
There was a problem hiding this comment.
After openjdk/jdk#28018 (comment), I have again lost faith in my ability to determine when and how a JVM will report missing classes :) Notably, even if tests pass in the various environments we test in, we have seen failures in other environments.
So: I wouldn't 100% rule out that Class.forName and newInstance could succeed (so there's no ReflectiveOperationException) but that get could fail. Options include "Try what we have and see what happens" and "catch (Throwable t)," or maybe you have something else we could try. Maybe the constructor for JavaTimeTypeAdapters could instantiate a Duration instance for the sake of (presumably) triggering a failure that would still result in ReflectiveOperationException? Or, again, we could hope for the best with what we have now.
| // Assuming we have reflective access to the fields of java.time classes, check that | ||
| // ReflectiveTypeAdapterFactory would produce the same JSON. This ensures that we are preserving | ||
| // a compatible JSON format for those classes even though we no longer use reflection. | ||
| private void checkReflectiveTypeAdapterFactory(Object value, String expectedJson) { |
There was a problem hiding this comment.
Do you want to skip the tests yet under Java 26(?)+, or is it too early to worry about that?
4 participants
Add type adapters for
java.timeclasses.These adapters essentially freeze the JSON representation that
ReflectiveTypeAdapterFactoryestablished by default, based on the private fields ofjava.timeclasses. That's not a great representation, but it is understandable. Changing it to anything else would break compatibility with systems that are expecting the current format.If Gson had been updated with
java.timeadapters at the time thejava.timeAPI was added, the representation would surely have been something else, probably ISO standard formats. We can potentially supply non-default adapters for that, but we'll still need to have these legacy adapters by default.I've been meaning to make this change for a while, but the need to do so becomes more urgent with this JDK
commit which makes a number of
java.timefieldstransient. That means that, in JDK 26, the reflective-based adapter will no longer work with the classes that were changed.This addresses #1059 and #739. All relevant classes in the
java.timepackage are covered.(Google internal bug reference: b/463237567.)