From c731abb293e79b26e53db3264306768fb86ef4c4 Mon Sep 17 00:00:00 2001 From: Inderjeet Singh Date: Thu, 25 Feb 2016 13:56:42 -0800 Subject: [PATCH 1/4] Fixed a regression in Gson 2.6 where Gson caused NPE if the TypeAdapterFactory.create() returned null. --- ...onAdapterAnnotationTypeAdapterFactory.java | 8 +- .../regression/JsonAdapterNullSafeTest.java | 186 ++++++++++++++++++ 2 files changed, 191 insertions(+), 3 deletions(-) create mode 100644 gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java 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 3801cfd4..b52e1573 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 @@ -51,7 +51,7 @@ public final class JsonAdapterAnnotationTypeAdapterFactory implements TypeAdapte static TypeAdapter getTypeAdapter(ConstructorConstructor constructorConstructor, Gson gson, TypeToken fieldType, JsonAdapter annotation) { Class value = annotation.value(); - final TypeAdapter typeAdapter; + TypeAdapter typeAdapter; if (TypeAdapter.class.isAssignableFrom(value)) { Class> typeAdapterClass = (Class>) value; typeAdapter = constructorConstructor.get(TypeToken.get(typeAdapterClass)).construct(); @@ -64,7 +64,9 @@ public final class JsonAdapterAnnotationTypeAdapterFactory implements TypeAdapte throw new IllegalArgumentException( "@JsonAdapter value must be TypeAdapter or TypeAdapterFactory reference."); } - - return typeAdapter.nullSafe(); + if (typeAdapter != null) { + typeAdapter = typeAdapter.nullSafe(); + } + return typeAdapter; } } diff --git a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java new file mode 100644 index 00000000..236a4bc9 --- /dev/null +++ b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java @@ -0,0 +1,186 @@ +/* + * 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 java.io.IOException; +import java.util.Objects; + +import com.google.gson.Gson; +import com.google.gson.JsonElement; +import com.google.gson.JsonParser; +import com.google.gson.JsonPrimitive; +import com.google.gson.JsonSyntaxException; +import com.google.gson.TypeAdapter; +import com.google.gson.TypeAdapterFactory; +import com.google.gson.annotations.JsonAdapter; +import com.google.gson.annotations.SerializedName; +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 junit.framework.TestCase; + +public class JsonAdapterNullSafeTest extends TestCase { + private final Gson gson = new Gson(); + + /** + * The recursiveCall in {@link ControlData.JsonAdapterFactory} is the source of this bug + * because it returns a null type adapter. + */ + public void testTypeAdapterFactoryNullSafeBug() throws IOException { + ControlData control = new ControlData("ec57803e", 2, true, 11211); + Device device = new Device("device1", control); + String json = gson.toJson(device); + + json = "\"{\\\"id\\\":\\\"ec57803e2\\\",\\\"category\\\":2,\\\"alwaysOn\\\":true,\\\"codeset_id\\\":11211}\""; + control = gson.fromJson(json, ControlData.class); + assertEquals("ec57803e2", control.id); + assertTrue(control.alwaysOn); + assertEquals(11211, control.codesetId); + assertEquals(2, control.category); + + String deviceJson = "{'id':'device1','controlData':null}"; + device = gson.fromJson(deviceJson, Device.class); + assertNull(device.controlData); + + deviceJson = "{'id':'device1','controlData':{'id':'ec57803e2','category':2,'alwaysOn':true,'codeset_id':12221}}"; + device = gson.fromJson(deviceJson, Device.class); + assertEquals(12221, device.controlData.codesetId); + + deviceJson = "{'id':'device1','controlData':'\\\"{}\\\"'}"; + device = gson.fromJson(deviceJson, Device.class); + + try { + deviceJson = "{'id':'device1','controlData':'a'}"; + device = gson.fromJson(deviceJson, Device.class); + assertNotNull(device.controlData); + fail(); + } catch (JsonSyntaxException expected) {} + + deviceJson = "{'id':'device1','controlData':' '}"; + device = gson.fromJson(deviceJson, Device.class); + assertNull(device.controlData); + } + + private static final class Device { + @SuppressWarnings("unused") + String id; + ControlData controlData; + + public Device(String id, ControlData controlData) { + this.id = id; + this.controlData = controlData; + } + } + + @JsonAdapter(ControlData.JsonAdapterFactory.class) + private static final class ControlData { + String id; + int category; + boolean alwaysOn; + @SerializedName("codeset_id") int codesetId; + ControlData(String id, int category, boolean alwaysOn, int codesetId) { + this.id = id; + this.category = category; + this.alwaysOn = alwaysOn; + this.codesetId = codesetId; + } + + /** + * DeviceControlData is received as String in JSON instead of proper JSON. + * So, we need to write a special type adapter. + */ + static final class JsonAdapterFactory extends StringifiedJsonAdapterFactory { + private static final ThreadLocal recursiveCall = new ThreadLocal(); + public JsonAdapterFactory() { + super(recursiveCall, ControlData.class, true); + } + } + } + + /** + * Converts an object to Stringified JSON for saving in a JSON field as a string type. + */ + private static class StringifiedJsonAdapterFactory implements TypeAdapterFactory { + private final Class targetType; + private final ThreadLocal recursiveCall; + private final boolean writeAsJson; + + /** + * @param recursiveCall provide a static ThreadLocal to workaround a Gson bug where + * annotation-based type adapter factories can't be skipped over. + * @param targetType The class whose instances needs to be written in stringified form. + * @param writeAsJson Set this to true to write the output as JSON not string. + */ + public StringifiedJsonAdapterFactory(ThreadLocal recursiveCall, Class targetType, + boolean writeAsJson) { + this.recursiveCall = recursiveCall; + this.targetType = targetType; + this.writeAsJson = writeAsJson; + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + @Override public TypeAdapter create(final Gson gson, TypeToken type) { + if (type.getRawType() != targetType || recursiveCall.get() != null) { + recursiveCall.set(null); // clear for subsequent use + return null; + } + recursiveCall.set(Boolean.TRUE); + final TypeAdapter delegate = (TypeAdapter) gson.getDelegateAdapter(this, type); + return (TypeAdapter) new TypeAdapter() { + @Override public void write(JsonWriter out, R value) throws IOException { + if (writeAsJson) { + delegate.write(out, value); + } else { + // delegate.toJson(value) will write nulls. avoid that by using gson.toJson() + String json = gson.toJson(delegate.toJsonTree(value)); + out.value(json); + } + } + @Override public R read(JsonReader in) throws IOException { + JsonToken token = in.peek(); + JsonElement root; + if (token == JsonToken.BEGIN_OBJECT) { + return delegate.read(in); + } else { // assume to be string + String json = in.nextString(); + JsonParser parser = new JsonParser(); + root = parseString(parser, json, null); + return root == null ? null : delegate.fromJsonTree(root); + } + } + + private JsonElement parseString(JsonParser parser, String json, String prevJson) + throws IOException { + if (json == null || json.trim().isEmpty()) { + return null; + } + JsonElement root = parser.parse(json); + if (root instanceof JsonPrimitive) { + prevJson = json; + json = root.getAsString(); + if (Objects.equals(json, prevJson)) { + throw new JsonSyntaxException("Unexpected Json: " + json); + } + return parseString(parser, json, prevJson); + } + return root; + } + }.nullSafe(); + } + } +} From 79a00cd90695c9ab72f301ca30fe2cbbc1fe8e36 Mon Sep 17 00:00:00 2001 From: Inderjeet Singh Date: Thu, 25 Feb 2016 17:38:48 -0800 Subject: [PATCH 2/4] incorporated code review feedback. Simplified the code, merged Device and Control and removed unnecessary fields. --- .../regression/JsonAdapterNullSafeTest.java | 172 +++++------------- 1 file changed, 46 insertions(+), 126 deletions(-) diff --git a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java index 236a4bc9..c888b4ee 100644 --- a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java +++ b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java @@ -26,161 +26,81 @@ import com.google.gson.JsonSyntaxException; import com.google.gson.TypeAdapter; import com.google.gson.TypeAdapterFactory; import com.google.gson.annotations.JsonAdapter; -import com.google.gson.annotations.SerializedName; 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 junit.framework.TestCase; public class JsonAdapterNullSafeTest extends TestCase { + // The recursiveCall in {@link Device.JsonAdapterFactory} is the source of this bug + // because it returns a null type adapter. + private final Gson gson = new Gson(); - /** - * The recursiveCall in {@link ControlData.JsonAdapterFactory} is the source of this bug - * because it returns a null type adapter. - */ - public void testTypeAdapterFactoryNullSafeBug() throws IOException { - ControlData control = new ControlData("ec57803e", 2, true, 11211); - Device device = new Device("device1", control); - String json = gson.toJson(device); - - json = "\"{\\\"id\\\":\\\"ec57803e2\\\",\\\"category\\\":2,\\\"alwaysOn\\\":true,\\\"codeset_id\\\":11211}\""; - control = gson.fromJson(json, ControlData.class); - assertEquals("ec57803e2", control.id); - assertTrue(control.alwaysOn); - assertEquals(11211, control.codesetId); - assertEquals(2, control.category); - - String deviceJson = "{'id':'device1','controlData':null}"; - device = gson.fromJson(deviceJson, Device.class); - assertNull(device.controlData); - - deviceJson = "{'id':'device1','controlData':{'id':'ec57803e2','category':2,'alwaysOn':true,'codeset_id':12221}}"; - device = gson.fromJson(deviceJson, Device.class); - assertEquals(12221, device.controlData.codesetId); - - deviceJson = "{'id':'device1','controlData':'\\\"{}\\\"'}"; - device = gson.fromJson(deviceJson, Device.class); - - try { - deviceJson = "{'id':'device1','controlData':'a'}"; - device = gson.fromJson(deviceJson, Device.class); - assertNotNull(device.controlData); - fail(); - } catch (JsonSyntaxException expected) {} - - deviceJson = "{'id':'device1','controlData':' '}"; - device = gson.fromJson(deviceJson, Device.class); - assertNull(device.controlData); + public void testNullSafeBugSerialize() throws Exception { + Device device = new Device("ec57803e", 2); + gson.toJson(device); } + public void testNullSafeBugDeserialize() throws Exception { + String json = "\"{\\\"id\\\":\\\"ec57803e2\\\",\\\"category\\\":2}\""; + Device device = gson.fromJson(json, Device.class); + assertEquals("ec57803e2", device.id); + assertEquals(2, device.category); + } + + @JsonAdapter(Device.JsonAdapterFactory.class) private static final class Device { - @SuppressWarnings("unused") - String id; - ControlData controlData; - - public Device(String id, ControlData controlData) { - this.id = id; - this.controlData = controlData; - } - } - - @JsonAdapter(ControlData.JsonAdapterFactory.class) - private static final class ControlData { String id; int category; - boolean alwaysOn; - @SerializedName("codeset_id") int codesetId; - ControlData(String id, int category, boolean alwaysOn, int codesetId) { + Device(String id, int category) { this.id = id; this.category = category; - this.alwaysOn = alwaysOn; - this.codesetId = codesetId; } /** - * DeviceControlData is received as String in JSON instead of proper JSON. - * So, we need to write a special type adapter. + * Write the value as a String, not JSON. */ - static final class JsonAdapterFactory extends StringifiedJsonAdapterFactory { + static final class JsonAdapterFactory implements TypeAdapterFactory { private static final ThreadLocal recursiveCall = new ThreadLocal(); - public JsonAdapterFactory() { - super(recursiveCall, ControlData.class, true); - } - } - } - /** - * Converts an object to Stringified JSON for saving in a JSON field as a string type. - */ - private static class StringifiedJsonAdapterFactory implements TypeAdapterFactory { - private final Class targetType; - private final ThreadLocal recursiveCall; - private final boolean writeAsJson; - - /** - * @param recursiveCall provide a static ThreadLocal to workaround a Gson bug where - * annotation-based type adapter factories can't be skipped over. - * @param targetType The class whose instances needs to be written in stringified form. - * @param writeAsJson Set this to true to write the output as JSON not string. - */ - public StringifiedJsonAdapterFactory(ThreadLocal recursiveCall, Class targetType, - boolean writeAsJson) { - this.recursiveCall = recursiveCall; - this.targetType = targetType; - this.writeAsJson = writeAsJson; - } - - @SuppressWarnings({"unchecked", "rawtypes"}) - @Override public TypeAdapter create(final Gson gson, TypeToken type) { - if (type.getRawType() != targetType || recursiveCall.get() != null) { - recursiveCall.set(null); // clear for subsequent use - return null; - } - recursiveCall.set(Boolean.TRUE); - final TypeAdapter delegate = (TypeAdapter) gson.getDelegateAdapter(this, type); - return (TypeAdapter) new TypeAdapter() { - @Override public void write(JsonWriter out, R value) throws IOException { - if (writeAsJson) { - delegate.write(out, value); - } else { - // delegate.toJson(value) will write nulls. avoid that by using gson.toJson() - String json = gson.toJson(delegate.toJsonTree(value)); - out.value(json); - } + @SuppressWarnings({"unchecked", "rawtypes"}) + @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; } - @Override public R read(JsonReader in) throws IOException { - JsonToken token = in.peek(); - JsonElement root; - if (token == JsonToken.BEGIN_OBJECT) { - return delegate.read(in); - } else { // assume to be string + recursiveCall.set(Boolean.TRUE); + final TypeAdapter delegate = (TypeAdapter) gson.getDelegateAdapter(this, type); + return (TypeAdapter) new TypeAdapter() { + @Override public void write(JsonWriter out, Device value) throws IOException { + delegate.write(out, value); + } + @Override public Device read(JsonReader in) throws IOException { String json = in.nextString(); JsonParser parser = new JsonParser(); - root = parseString(parser, json, null); + JsonElement root = parseString(parser, json, null); return root == null ? null : delegate.fromJsonTree(root); } - } - - private JsonElement parseString(JsonParser parser, String json, String prevJson) - throws IOException { - if (json == null || json.trim().isEmpty()) { - return null; - } - JsonElement root = parser.parse(json); - if (root instanceof JsonPrimitive) { - prevJson = json; - json = root.getAsString(); - if (Objects.equals(json, prevJson)) { - throw new JsonSyntaxException("Unexpected Json: " + json); + private JsonElement parseString(JsonParser parser, String json, String prevJson) + throws IOException { // called recursively + if (json == null || json.trim().isEmpty()) { + return null; } - return parseString(parser, json, prevJson); + JsonElement root = parser.parse(json); + if (root instanceof JsonPrimitive) { + prevJson = json; + json = root.getAsString(); + if (Objects.equals(json, prevJson)) { + throw new JsonSyntaxException("Unexpected Json: " + json); + } + return parseString(parser, json, prevJson); + } + return root; } - return root; - } - }.nullSafe(); + }; + } } } } From 1fa43821e829c455db89f9b75737d109c01dfd2c Mon Sep 17 00:00:00 2001 From: Inderjeet Singh Date: Thu, 25 Feb 2016 19:37:07 -0800 Subject: [PATCH 3/4] removed unneeded null check. --- .../com/google/gson/regression/JsonAdapterNullSafeTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java index c888b4ee..3b332940 100644 --- a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java +++ b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java @@ -85,9 +85,6 @@ public class JsonAdapterNullSafeTest extends TestCase { } private JsonElement parseString(JsonParser parser, String json, String prevJson) throws IOException { // called recursively - if (json == null || json.trim().isEmpty()) { - return null; - } JsonElement root = parser.parse(json); if (root instanceof JsonPrimitive) { prevJson = json; From 1ab73ffd21d8c08bbe734154921a936e4a8cb091 Mon Sep 17 00:00:00 2001 From: Inderjeet Singh Date: Fri, 26 Feb 2016 09:25:23 -0800 Subject: [PATCH 4/4] incorporated code review feedback by eliminating the stringified type adapter. --- .../regression/JsonAdapterNullSafeTest.java | 53 +++---------------- 1 file changed, 6 insertions(+), 47 deletions(-) diff --git a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java index 3b332940..30a6775c 100644 --- a/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java +++ b/gson/src/test/java/com/google/gson/regression/JsonAdapterNullSafeTest.java @@ -15,54 +15,37 @@ */ package com.google.gson.regression; -import java.io.IOException; -import java.util.Objects; - import com.google.gson.Gson; -import com.google.gson.JsonElement; -import com.google.gson.JsonParser; -import com.google.gson.JsonPrimitive; -import com.google.gson.JsonSyntaxException; import com.google.gson.TypeAdapter; import com.google.gson.TypeAdapterFactory; import com.google.gson.annotations.JsonAdapter; import com.google.gson.reflect.TypeToken; -import com.google.gson.stream.JsonReader; -import com.google.gson.stream.JsonWriter; import junit.framework.TestCase; public class JsonAdapterNullSafeTest extends TestCase { - // The recursiveCall in {@link Device.JsonAdapterFactory} is the source of this bug - // because it returns a null type adapter. - private final Gson gson = new Gson(); public void testNullSafeBugSerialize() throws Exception { - Device device = new Device("ec57803e", 2); + Device device = new Device("ec57803e"); gson.toJson(device); } public void testNullSafeBugDeserialize() throws Exception { - String json = "\"{\\\"id\\\":\\\"ec57803e2\\\",\\\"category\\\":2}\""; - Device device = gson.fromJson(json, Device.class); + Device device = gson.fromJson("{'id':'ec57803e2'}", Device.class); assertEquals("ec57803e2", device.id); - assertEquals(2, device.category); } @JsonAdapter(Device.JsonAdapterFactory.class) private static final class Device { String id; - int category; - Device(String id, int category) { + Device(String id) { this.id = id; - this.category = category; } - /** - * Write the value as a String, not JSON. - */ 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(); @SuppressWarnings({"unchecked", "rawtypes"}) @@ -72,31 +55,7 @@ public class JsonAdapterNullSafeTest extends TestCase { return null; } recursiveCall.set(Boolean.TRUE); - final TypeAdapter delegate = (TypeAdapter) gson.getDelegateAdapter(this, type); - return (TypeAdapter) new TypeAdapter() { - @Override public void write(JsonWriter out, Device value) throws IOException { - delegate.write(out, value); - } - @Override public Device read(JsonReader in) throws IOException { - String json = in.nextString(); - JsonParser parser = new JsonParser(); - JsonElement root = parseString(parser, json, null); - return root == null ? null : delegate.fromJsonTree(root); - } - private JsonElement parseString(JsonParser parser, String json, String prevJson) - throws IOException { // called recursively - JsonElement root = parser.parse(json); - if (root instanceof JsonPrimitive) { - prevJson = json; - json = root.getAsString(); - if (Objects.equals(json, prevJson)) { - throw new JsonSyntaxException("Unexpected Json: " + json); - } - return parseString(parser, json, prevJson); - } - return root; - } - }; + return (TypeAdapter) gson.getDelegateAdapter(this, type); } } }