From 88fd6d13905b520531f79cd3caf9fff30f3617e9 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 23 Aug 2023 16:09:32 +0200 Subject: [PATCH] Improve `JsonAdapter` documentation and tests (#2442) * Document how `JsonAdapter` creates adapter instances & add tests * Extend `JsonAdapter.nullSafe()` documentation * Improve test for JsonAdapter factory returning null Existing test `JsonAdapterNullSafeTest` had misleading comments; while it did in the end detect if null had not been handled correctly, that only worked because the field `JsonAdapterFactory.recursiveCall` is static and one test method therefore affected the state of the other test method. If the test methods were run separately in different test runs, they would not have detected if null was handled correctly, because the factory would not have returned null. * Extend JsonAdapter nullSafe test * Extend test --- .../java/com/google/gson/InstanceCreator.java | 2 + .../google/gson/annotations/JsonAdapter.java | 44 ++++-- ...onAdapterAnnotationTypeAdapterFactory.java | 1 + .../JsonAdapterAnnotationOnClassesTest.java | 140 +++++++++++++++++- ...JsonAdapterSerializerDeserializerTest.java | 61 ++++++-- .../regression/JsonAdapterNullSafeTest.java | 64 -------- 6 files changed, 223 insertions(+), 89 deletions(-) delete mode 100644 gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java diff --git a/gson/src/main/java/com/google/gson/InstanceCreator.java b/gson/src/main/java/com/google/gson/InstanceCreator.java index b973da07..486f2504 100644 --- a/gson/src/main/java/com/google/gson/InstanceCreator.java +++ b/gson/src/main/java/com/google/gson/InstanceCreator.java @@ -73,6 +73,8 @@ import java.lang.reflect.Type; * * @param the type of object that will be created by this implementation. * + * @see GsonBuilder#registerTypeAdapter(Type, Object) + * * @author Inderjeet Singh * @author Joel Leitch */ diff --git a/gson/src/main/java/com/google/gson/annotations/JsonAdapter.java b/gson/src/main/java/com/google/gson/annotations/JsonAdapter.java index d1685759..5a5da72e 100644 --- a/gson/src/main/java/com/google/gson/annotations/JsonAdapter.java +++ b/gson/src/main/java/com/google/gson/annotations/JsonAdapter.java @@ -17,6 +17,8 @@ package com.google.gson.annotations; import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.InstanceCreator; import com.google.gson.JsonDeserializer; import com.google.gson.JsonSerializer; import com.google.gson.TypeAdapter; @@ -35,11 +37,13 @@ import java.lang.annotation.Target; * @JsonAdapter(UserJsonAdapter.class) * public class User { * public final String firstName, lastName; + * * private User(String firstName, String lastName) { * this.firstName = firstName; * this.lastName = lastName; * } * } + * * public class UserJsonAdapter extends TypeAdapter<User> { * @Override public void write(JsonWriter out, User user) throws IOException { * // implement write: combine firstName and lastName into name @@ -47,8 +51,8 @@ import java.lang.annotation.Target; * out.name("name"); * out.value(user.firstName + " " + user.lastName); * out.endObject(); - * // implement the write method * } + * * @Override public User read(JsonReader in) throws IOException { * // implement read: split name into firstName and lastName * in.beginObject(); @@ -60,14 +64,15 @@ import java.lang.annotation.Target; * } * * - * Since User class specified UserJsonAdapter.class in @JsonAdapter annotation, it - * will automatically be invoked to serialize/deserialize User instances. + * Since {@code User} class specified {@code UserJsonAdapter.class} in {@code @JsonAdapter} + * annotation, it will automatically be invoked to serialize/deserialize {@code User} instances. * - *

Here is an example of how to apply this annotation to a field. + *

Here is an example of how to apply this annotation to a field. * 

  * private static final class Gadget {
- *   @JsonAdapter(UserJsonAdapter2.class)
+ *   @JsonAdapter(UserJsonAdapter.class)
  *   final User user;
+ *
  *   Gadget(User user) {
  *     this.user = user;
  *   }
@@ -75,15 +80,30 @@ import java.lang.annotation.Target;
  *
* * It's possible to specify different type adapters on a field, that - * field's type, and in the {@link com.google.gson.GsonBuilder}. Field - * annotations take precedence over {@code GsonBuilder}-registered type + * field's type, and in the {@link GsonBuilder}. Field annotations + * take precedence over {@code GsonBuilder}-registered type * adapters, which in turn take precedence over annotated types. * *

The class referenced by this annotation must be either a {@link * TypeAdapter} or a {@link TypeAdapterFactory}, or must implement one * or both of {@link JsonDeserializer} or {@link JsonSerializer}. * Using {@link TypeAdapterFactory} makes it possible to delegate - * to the enclosing {@link Gson} instance. + * to the enclosing {@link Gson} instance. By default the specified + * adapter will not be called for {@code null} values; set {@link #nullSafe()} + * to {@code false} to let the adapter handle {@code null} values itself. + * + *

The type adapter is created in the same way Gson creates instances of + * custom classes during deserialization, that means: + *

    + *
  1. If a custom {@link InstanceCreator} has been registered for the + * adapter class, it will be used to create the instance + *
  2. Otherwise, if the adapter class has a no-args constructor + * (regardless of which visibility), it will be invoked to create + * the instance + *
  3. Otherwise, JDK {@code Unsafe} will be used to create the instance; + * see {@link GsonBuilder#disableJdkUnsafe()} for the unexpected + * side-effects this might have + *
* *

{@code Gson} instances might cache the adapter they create for * a {@code @JsonAdapter} annotation. It is not guaranteed that a new @@ -104,7 +124,13 @@ public @interface JsonAdapter { /** Either a {@link TypeAdapter} or {@link TypeAdapterFactory}, or one or both of {@link JsonDeserializer} or {@link JsonSerializer}. */ Class value(); - /** false, to be able to handle {@code null} values within the adapter, default value is true. */ + /** + * Whether the adapter referenced by {@link #value()} should be made {@linkplain TypeAdapter#nullSafe() null-safe}. + * + *

If {@code true} (the default), it will be made null-safe and Gson will handle {@code null} Java objects + * on serialization and JSON {@code null} on deserialization without calling the adapter. If {@code false}, + * the adapter will have to handle the {@code null} values. + */ boolean nullSafe() default true; } diff --git a/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java b/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java index b444a4bd..8b528b7f 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java +++ b/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java @@ -134,6 +134,7 @@ public final class JsonAdapterAnnotationTypeAdapterFactory implements TypeAdapte TypeAdapter tempAdapter = new TreeTypeAdapter(serializer, deserializer, gson, type, skipPast, nullSafe); typeAdapter = tempAdapter; + // TreeTypeAdapter handles nullSafe; don't additionally call `nullSafe()` nullSafe = false; } else { throw new IllegalArgumentException("Invalid attempt to bind an instance of " diff --git a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java index d540f6e7..b2b8dc0a 100644 --- a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java +++ b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java @@ -37,6 +37,7 @@ import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonWriter; import java.io.IOException; +import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.List; import java.util.Locale; @@ -147,10 +148,84 @@ public final class JsonAdapterAnnotationOnClassesTest { } @Test - public void testNullSafeObjectFromJson() { + public void testNullSafeObject() { Gson gson = new Gson(); NullableClass fromJson = gson.fromJson("null", NullableClass.class); assertThat(fromJson).isNull(); + + fromJson = gson.fromJson("\"ignored\"", NullableClass.class); + assertThat(fromJson).isNotNull(); + + String json = gson.toJson(null, NullableClass.class); + assertThat(json).isEqualTo("null"); + + json = gson.toJson(new NullableClass()); + assertThat(json).isEqualTo("\"nullable\""); + } + + /** + * Tests behavior when a {@link TypeAdapterFactory} registered with {@code @JsonAdapter} returns + * {@code null}, indicating that it cannot handle the type and Gson should try a different factory + * instead. + */ + @Test + public void testFactoryReturningNull() { + Gson gson = new Gson(); + + assertThat(gson.fromJson("null", WithNullReturningFactory.class)).isNull(); + assertThat(gson.toJson(null, WithNullReturningFactory.class)).isEqualTo("null"); + + TypeToken> stringTypeArg = new TypeToken>() {}; + WithNullReturningFactory deserialized = gson.fromJson("\"a\"", stringTypeArg); + assertThat(deserialized.t).isEqualTo("custom-read:a"); + assertThat(gson.fromJson("null", stringTypeArg)).isNull(); + assertThat(gson.toJson(new WithNullReturningFactory<>("b"), stringTypeArg.getType())).isEqualTo("\"custom-write:b\""); + assertThat(gson.toJson(null, stringTypeArg.getType())).isEqualTo("null"); + + // Factory should return `null` for this type and Gson should fall back to reflection-based adapter + TypeToken> numberTypeArg = new TypeToken>() {}; + deserialized = gson.fromJson("{\"t\":1}", numberTypeArg); + assertThat(deserialized.t).isEqualTo(1); + assertThat(gson.toJson(new WithNullReturningFactory<>(2), numberTypeArg.getType())).isEqualTo("{\"t\":2}"); + } + // Also set `nullSafe = true` to verify that this does not cause a NullPointerException if the + // factory would accidentally call `nullSafe()` on null adapter + @JsonAdapter(value = WithNullReturningFactory.NullReturningFactory.class, nullSafe = true) + private static class WithNullReturningFactory { + T t; + + public WithNullReturningFactory(T t) { + this.t = t; + } + + static class NullReturningFactory implements TypeAdapterFactory { + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + // Don't handle raw (non-parameterized) type + if (type.getType() instanceof Class) { + return null; + } + ParameterizedType parameterizedType = (ParameterizedType) type.getType(); + // Makes this test a bit more realistic by only conditionally returning null (instead of always) + if (parameterizedType.getActualTypeArguments()[0] != String.class) { + return null; + } + + @SuppressWarnings("unchecked") + TypeAdapter adapter = (TypeAdapter) new TypeAdapter>() { + @Override + public void write(JsonWriter out, WithNullReturningFactory value) throws IOException { + out.value("custom-write:" + value.t); + } + + @Override + public WithNullReturningFactory read(JsonReader in) throws IOException { + return new WithNullReturningFactory<>("custom-read:" + in.nextString()); + } + }; + return adapter; + } + } } @JsonAdapter(A.JsonAdapter.class) @@ -223,7 +298,6 @@ public final class JsonAdapterAnnotationOnClassesTest { out.name("name"); out.value(user.firstName + " " + user.lastName); out.endObject(); - // implement the write method } @Override public User read(JsonReader in) throws IOException { // implement read: split name into firstName and lastName @@ -235,6 +309,7 @@ public final class JsonAdapterAnnotationOnClassesTest { } } + // Implicit `nullSafe=true` @JsonAdapter(value = NullableClassJsonAdapter.class) private static class NullableClass { } @@ -606,4 +681,65 @@ public final class JsonAdapterAnnotationOnClassesTest { } } } + + /** + * Tests creation of the adapter referenced by {@code @JsonAdapter} using an {@link InstanceCreator}. + */ + @Test + public void testAdapterCreatedByInstanceCreator() { + CreatedByInstanceCreator.Serializer serializer = new CreatedByInstanceCreator.Serializer("custom"); + Gson gson = new GsonBuilder() + .registerTypeAdapter(CreatedByInstanceCreator.Serializer.class, (InstanceCreator) t -> serializer) + .create(); + + String json = gson.toJson(new CreatedByInstanceCreator()); + assertThat(json).isEqualTo("\"custom\""); + } + @JsonAdapter(CreatedByInstanceCreator.Serializer.class) + private static class CreatedByInstanceCreator { + static class Serializer implements JsonSerializer { + private final String value; + + @SuppressWarnings("unused") + public Serializer() { + throw new AssertionError("should not be called"); + } + + public Serializer(String value) { + this.value = value; + } + + @Override + public JsonElement serialize(CreatedByInstanceCreator src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive(value); + } + } + } + + /** + * Tests creation of the adapter referenced by {@code @JsonAdapter} using JDK Unsafe. + */ + @Test + public void testAdapterCreatedByJdkUnsafe() { + String json = new Gson().toJson(new CreatedByJdkUnsafe()); + assertThat(json).isEqualTo("false"); + } + @JsonAdapter(CreatedByJdkUnsafe.Serializer.class) + private static class CreatedByJdkUnsafe { + static class Serializer implements JsonSerializer { + // JDK Unsafe leaves this at default value `false` + private boolean wasInitialized = true; + + // Explicit constructor with args to remove implicit no-args constructor + @SuppressWarnings("unused") + public Serializer(int i) { + throw new AssertionError("should not be called"); + } + + @Override + public JsonElement serialize(CreatedByJdkUnsafe src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive(wasInitialized); + } + } + } } diff --git a/gson/src/test/java/com/google/gson/functional/JsonAdapterSerializerDeserializerTest.java b/gson/src/test/java/com/google/gson/functional/JsonAdapterSerializerDeserializerTest.java index 20081f68..1e4af6a0 100644 --- a/gson/src/test/java/com/google/gson/functional/JsonAdapterSerializerDeserializerTest.java +++ b/gson/src/test/java/com/google/gson/functional/JsonAdapterSerializerDeserializerTest.java @@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.errorprone.annotations.Keep; import com.google.gson.Gson; +import com.google.gson.GsonBuilder; import com.google.gson.JsonDeserializationContext; import com.google.gson.JsonDeserializer; import com.google.gson.JsonElement; @@ -27,7 +28,11 @@ import com.google.gson.JsonParseException; import com.google.gson.JsonPrimitive; import com.google.gson.JsonSerializationContext; import com.google.gson.JsonSerializer; +import com.google.gson.TypeAdapter; import com.google.gson.annotations.JsonAdapter; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonWriter; +import java.io.IOException; import java.lang.reflect.Type; import org.junit.Test; @@ -43,7 +48,7 @@ public final class JsonAdapterSerializerDeserializerTest { String json = gson.toJson(new Computer(new User("Inderjeet Singh"), null, new User("Jesse Wilson"))); assertThat(json).isEqualTo("{\"user1\":\"UserSerializer\",\"user3\":\"UserSerializerDeserializer\"}"); Computer computer = gson.fromJson("{'user2':'Jesse Wilson','user3':'Jake Wharton'}", Computer.class); - assertThat(computer.user2.name).isEqualTo("UserSerializer"); + assertThat(computer.user2.name).isEqualTo("UserDeserializer"); assertThat(computer.user3.name).isEqualTo("UserSerializerDeserializer"); } @@ -82,7 +87,7 @@ public final class JsonAdapterSerializerDeserializerTest { @Override public User deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { - return new User("UserSerializer"); + return new User("UserDeserializer"); } } @@ -178,20 +183,48 @@ public final class JsonAdapterSerializerDeserializerTest { @Test public void testJsonAdapterNullSafe() { - Gson gson = new Gson(); - String json = gson.toJson(new Computer3(null, null)); - assertThat(json).isEqualTo("{\"user1\":\"UserSerializerDeserializer\"}"); - Computer3 computer3 = gson.fromJson("{\"user1\":null, \"user2\":null}", Computer3.class); - assertThat(computer3.user1.name).isEqualTo("UserSerializerDeserializer"); - assertThat(computer3.user2).isNull(); + Gson gson = new GsonBuilder() + .registerTypeAdapter(User.class, new TypeAdapter() { + @Override + public User read(JsonReader in) throws IOException { + in.nextNull(); + return new User("fallback-read"); + } + @Override + public void write(JsonWriter out, User value) throws IOException { + assertThat(value).isNull(); + out.value("fallback-write"); + } + }) + .serializeNulls() + .create(); + + String json = gson.toJson(new WithNullSafe(null, null, null, null)); + // Only nullSafe=true serializer writes null; for @JsonAdapter with deserializer nullSafe is ignored when serializing + assertThat(json).isEqualTo("{\"userS\":\"UserSerializer\",\"userSN\":null,\"userD\":\"fallback-write\",\"userDN\":\"fallback-write\"}"); + + WithNullSafe deserialized = gson.fromJson("{\"userS\":null,\"userSN\":null,\"userD\":null,\"userDN\":null}", WithNullSafe.class); + // For @JsonAdapter with serializer nullSafe is ignored when deserializing + assertThat(deserialized.userS.name).isEqualTo("fallback-read"); + assertThat(deserialized.userSN.name).isEqualTo("fallback-read"); + assertThat(deserialized.userD.name).isEqualTo("UserDeserializer"); + assertThat(deserialized.userDN).isNull(); } - private static final class Computer3 { - @JsonAdapter(value = UserSerializerDeserializer.class, nullSafe = false) final User user1; - @JsonAdapter(value = UserSerializerDeserializer.class) final User user2; - Computer3(User user1, User user2) { - this.user1 = user1; - this.user2 = user2; + private static final class WithNullSafe { + // "userS..." uses JsonSerializer + @JsonAdapter(value = UserSerializer.class, nullSafe = false) final User userS; + @JsonAdapter(value = UserSerializer.class, nullSafe = true) final User userSN; + + // "userD..." uses JsonDeserializer + @JsonAdapter(value = UserDeserializer.class, nullSafe = false) final User userD; + @JsonAdapter(value = UserDeserializer.class, nullSafe = true) final User userDN; + + WithNullSafe(User userS, User userSN, User userD, User userDN) { + this.userS = userS; + this.userSN = userSN; + this.userD = userD; + this.userDN = userDN; } } } diff --git a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java deleted file mode 100644 index 10c9a9a5..00000000 --- a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Copyright (C) 2016 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.regression; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.gson.Gson; -import com.google.gson.TypeAdapter; -import com.google.gson.TypeAdapterFactory; -import com.google.gson.annotations.JsonAdapter; -import com.google.gson.reflect.TypeToken; -import org.junit.Test; - -public class JsonAdapterNullSafeTest { - private final Gson gson = new Gson(); - - @Test - public void testNullSafeBugSerialize() { - Device device = new Device("ec57803e"); - String unused = gson.toJson(device); - } - - @Test - public void testNullSafeBugDeserialize() { - Device device = gson.fromJson("{'id':'ec57803e2'}", Device.class); - assertThat(device.id).isEqualTo("ec57803e2"); - } - - @JsonAdapter(Device.JsonAdapterFactory.class) - private static final class Device { - String id; - Device(String id) { - this.id = id; - } - - static final class JsonAdapterFactory implements TypeAdapterFactory { - // The recursiveCall in {@link Device.JsonAdapterFactory} is the source of this bug - // because we use it to return a null type adapter on a recursive call. - private static final ThreadLocal recursiveCall = new ThreadLocal<>(); - - @Override public TypeAdapter create(final Gson gson, TypeToken type) { - if (type.getRawType() != Device.class || recursiveCall.get() != null) { - recursiveCall.set(null); // clear for subsequent use - return null; - } - recursiveCall.set(Boolean.TRUE); - return gson.getDelegateAdapter(this, type); - } - } - } -}