-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Follow-up for java.time adapter changes
#2972
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,29 @@ | ||
| /* | ||
| * Copyright (C) 2026 Google Inc. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I should have caught that. :-) |
||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.google.gson.internal.bind; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| /** | ||
| * Used for animal-sniffer-maven-plugin to suppress warnings about API being unavailable for the | ||
| * target Android API Level. | ||
| */ | ||
| @Retention(RetentionPolicy.CLASS) | ||
| @Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE, ElementType.FIELD}) | ||
| @SuppressWarnings("IdentifierName") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,18 @@ | ||
| /* | ||
| * Copyright (C) 2026 Google Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.google.gson.internal.bind; | ||
|
|
||
| import static java.lang.Math.toIntExact; | ||
|
|
@@ -9,7 +24,6 @@ | |
| import com.google.gson.internal.bind.TypeAdapters.IntegerFieldsTypeAdapter; | ||
| import com.google.gson.reflect.TypeToken; | ||
| import com.google.gson.stream.JsonReader; | ||
| import com.google.gson.stream.JsonToken; | ||
| import com.google.gson.stream.JsonWriter; | ||
| import java.io.IOException; | ||
| import java.time.Duration; | ||
|
|
@@ -39,8 +53,11 @@ | |
| * is obviously fragile, and it also needs special {@code --add-opens} configuration with more | ||
| * recent JDK versions. So here we freeze the representation that was current with JDK 21, in a way | ||
| * that does not use reflection. | ||
| * | ||
| * <p>This class should not directly be used, instead the type adapter factory should be obtained | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fair, since apparently the presence of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main concern was actually usage from within Gson (since |
||
| * from {@link TypeAdapters#javaTimeTypeAdapterFactory()}. | ||
| */ | ||
| @IgnoreJRERequirement // Protected by a reflective check | ||
| @IgnoreJRERequirement // Protected by a reflective check in `TypeAdapters` | ||
| final class JavaTimeTypeAdapters implements TypeAdapters.FactorySupplier { | ||
|
|
||
| @Override | ||
|
|
@@ -91,7 +108,7 @@ long[] integerValues(LocalDate localDate) { | |
| } | ||
| }; | ||
|
|
||
| public static final TypeAdapter<LocalTime> LOCAL_TIME = | ||
| private static final TypeAdapter<LocalTime> LOCAL_TIME = | ||
| new IntegerFieldsTypeAdapter<LocalTime>("hour", "minute", "second", "nano") { | ||
| @Override | ||
| LocalTime create(long[] values) { | ||
|
|
@@ -119,7 +136,7 @@ public LocalDateTime read(JsonReader in) throws IOException { | |
| LocalDate localDate = null; | ||
| LocalTime localTime = null; | ||
| in.beginObject(); | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
eamonnmcmanus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| String name = in.nextName(); | ||
| switch (name) { | ||
| case "date": | ||
|
|
@@ -172,7 +189,7 @@ public OffsetDateTime read(JsonReader in) throws IOException { | |
| in.beginObject(); | ||
| LocalDateTime localDateTime = null; | ||
| ZoneOffset zoneOffset = null; | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
| String name = in.nextName(); | ||
| switch (name) { | ||
| case "dateTime": | ||
|
|
@@ -213,7 +230,7 @@ public OffsetTime read(JsonReader in) throws IOException { | |
| in.beginObject(); | ||
| LocalTime localTime = null; | ||
| ZoneOffset zoneOffset = null; | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
| String name = in.nextName(); | ||
| switch (name) { | ||
| case "time": | ||
|
|
@@ -287,7 +304,7 @@ long[] integerValues(YearMonth yearMonth) { | |
| // A ZoneId is either a ZoneOffset or a ZoneRegion, where ZoneOffset is public and ZoneRegion is | ||
| // not. For compatibility with reflection-based serialization, we need to write the "id" field of | ||
| // ZoneRegion if we have a ZoneRegion, and we need to write the "totalSeconds" field of ZoneOffset | ||
| // if we have a ZoneOffset. When reading, we need to construct the the appropriate thing depending | ||
| // if we have a ZoneOffset. When reading, we need to construct the appropriate thing depending | ||
| // on which of those two fields we see. | ||
| private static final TypeAdapter<ZoneId> ZONE_ID = | ||
| new TypeAdapter<ZoneId>() { | ||
|
|
@@ -296,7 +313,7 @@ public ZoneId read(JsonReader in) throws IOException { | |
| in.beginObject(); | ||
| String id = null; | ||
| Integer totalSeconds = null; | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
| String name = in.nextName(); | ||
| switch (name) { | ||
| case "id": | ||
|
|
@@ -348,7 +365,7 @@ public ZonedDateTime read(JsonReader in) throws IOException { | |
| LocalDateTime localDateTime = null; | ||
| ZoneOffset zoneOffset = null; | ||
| ZoneId zoneId = null; | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
| String name = in.nextName(); | ||
| switch (name) { | ||
| case "dateTime": | ||
|
|
@@ -390,7 +407,7 @@ public void write(JsonWriter out, ZonedDateTime value) throws IOException { | |
| }.nullSafe(); | ||
| } | ||
|
|
||
| static final TypeAdapterFactory JAVA_TIME_FACTORY = | ||
| private static final TypeAdapterFactory JAVA_TIME_FACTORY = | ||
| new TypeAdapterFactory() { | ||
| @Override | ||
| public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -807,6 +807,8 @@ public void write(JsonWriter out, Currency value) throws IOException { | |
| * An abstract {@link TypeAdapter} for classes whose JSON serialization consists of a fixed set of | ||
| * integer fields. That is the case for {@link Calendar} and the legacy serialization of various | ||
| * {@code java.time} types. | ||
| * | ||
| * <p>This class handles {@code null}; subclasses don't have to use {@link #nullSafe()}. | ||
| */ | ||
| abstract static class IntegerFieldsTypeAdapter<T> extends TypeAdapter<T> { | ||
| private final List<String> fields; | ||
|
|
@@ -815,8 +817,21 @@ abstract static class IntegerFieldsTypeAdapter<T> extends TypeAdapter<T> { | |
| this.fields = Arrays.asList(fields); | ||
| } | ||
|
|
||
| /** | ||
| * On deserialization: Creates an object from the integer values. Subclasses should use {@link | ||
| * Math#toIntExact(long)} and similar if necessary to prevent silent truncation. | ||
| * | ||
| * <p>Values have the same order as the field names provided to the {@linkplain | ||
| * #IntegerFieldsTypeAdapter(String[]) constructor}. | ||
| */ | ||
| abstract T create(long[] values); | ||
|
|
||
| /** | ||
| * On serialization: Extracts the integer values from the object. | ||
| * | ||
| * <p>Values must have the same order as the field names provided to the {@linkplain | ||
| * #IntegerFieldsTypeAdapter(String[]) constructor}. | ||
| */ | ||
| abstract long[] integerValues(T t); | ||
|
|
||
| @Override | ||
|
|
@@ -827,7 +842,7 @@ public T read(JsonReader in) throws IOException { | |
| } | ||
| in.beginObject(); | ||
| long[] values = new long[fields.size()]; | ||
| while (in.peek() != JsonToken.END_OBJECT) { | ||
| while (in.hasNext()) { | ||
| String name = in.nextName(); | ||
| int index = fields.indexOf(name); | ||
| if (index >= 0) { | ||
|
|
@@ -884,7 +899,7 @@ long[] integerValues(Calendar calendar) { | |
| } | ||
| }; | ||
|
|
||
| // TODO: update this when we are on at least Android API Level 24. | ||
| // TODO: switch to `Math#toIntExact` when we are on at least Android API Level 24. | ||
|
Comment on lines
-887
to
+902
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, maybe we should throw |
||
| private static int toIntExact(long x) { | ||
| int i = (int) x; | ||
| if (i != x) { | ||
|
|
@@ -946,6 +961,10 @@ interface FactorySupplier { | |
| TypeAdapterFactory get(); | ||
| } | ||
|
|
||
| /** | ||
| * Adapter factory for {@code java.time} classes. Returns {@code null} if not supported by the | ||
| * current environment (e.g. too old Android version, without desugaring). | ||
| */ | ||
|
Comment on lines
+964
to
+967
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, maybe this comment I added is incorrect? When Do you have an internal test which covers this, and know how this behaves? |
||
| public static TypeAdapterFactory javaTimeTypeAdapterFactory() { | ||
| try { | ||
| Class<?> javaTimeTypeAdapterFactoryClass = | ||
|
|
||
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.
Only added this because the animal-sniffer-maven-plugin config had been changed to use the API Level 23 signatures.
But it seems we don't actually use any Android 23 API yet? So alternatively we could revert this here and used the API Level 21 signatures again?
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.
Yes, I think that would make sense. Do you want to add it to this PR?
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.
You mean revert to 21 API Level signatures again, right?