Skip to content
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.

Sign up for GitHub

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

Make Object and JsonElement deserialization iterative #1912

Merged
merged 5 commits into from
Jun 23, 2022
Merged

Make Object and JsonElement deserialization iterative #1912

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
539952e
Make Object and JsonElement deserialization iterative
Marcono1234 Jun 18, 2021
e35f674
Merge branch 'master' into marcono1234/iterative-deserialization
Marcono1234 Feb 5, 2022
dce0d06
Improve imports grouping
Marcono1234 Feb 5, 2022
e1f5a59
Merge branch 'master' into marcono1234/iterative-deserialization
Marcono1234 Apr 22, 2022
f7d3c18
Address review feedback
Marcono1234 May 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 79 additions & 23 deletions gson/src/main/java/com/google/gson/internal/bind/ObjectTypeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -70,42 +71,97 @@ public static TypeAdapterFactory getFactory(ToNumberStrategy toNumberStrategy) {
}
}

@Override public Object read(JsonReader in) throws IOException {
JsonToken token = in.peek();
switch (token) {
case BEGIN_ARRAY:
List<Object> list = new ArrayList<>();
/**
* Tries to begin reading a JSON array or JSON object, returning {@code null} if
* the next element is neither of those.
*/
private Object tryBeginNesting(JsonReader in, JsonToken peeked) throws IOException {
if (peeked == JsonToken.BEGIN_ARRAY) {
Marcono1234 marked this conversation as resolved.
Show resolved Hide resolved
in.beginArray();
while (in.hasNext()) {
list.add(read(in));
}
in.endArray();
return list;

case BEGIN_OBJECT:
Map<String, Object> map = new LinkedTreeMap<>();
return new ArrayList<>();
} else if (peeked == JsonToken.BEGIN_OBJECT) {
in.beginObject();
while (in.hasNext()) {
map.put(in.nextName(), read(in));
}
in.endObject();
return map;
return new LinkedTreeMap<String, Object>();
} else {
return null;
}
}

/** Reads an {@code Object} which cannot have any nested elements */
private Object readTerminal(JsonReader in, JsonToken peeked) throws IOException {
switch (peeked) {
case STRING:
return in.nextString();

case NUMBER:
return toNumberStrategy.readNumber(in);

case BOOLEAN:
return in.nextBoolean();

case NULL:
in.nextNull();
return null;

default:
throw new IllegalStateException();
// When read(JsonReader) is called with JsonReader in invalid state
throw new IllegalStateException("Unexpected token: " + peeked);
}
}

@Override public Object read(JsonReader in) throws IOException {
// Either List or Map
Object current;
JsonToken peeked = in.peek();

current = tryBeginNesting(in, peeked);
if (current == null) {
return readTerminal(in, peeked);
}

LinkedList<Object> stack = new LinkedList<>();
Marcono1234 marked this conversation as resolved.
Show resolved Hide resolved

while (true) {
while (in.hasNext()) {
String name = null;
// Name is only used for JSON object members
if (current instanceof Map) {
name = in.nextName();
}

peeked = in.peek();
Object value = tryBeginNesting(in, peeked);
boolean isNesting = value != null;

if (value == null) {
value = readTerminal(in, peeked);
}

if (current instanceof List) {
@SuppressWarnings("unchecked")
List<Object> list = (List<Object>) current;
list.add(value);
} else {
@SuppressWarnings("unchecked")
Map<String, Object> map = (Map<String, Object>) current;
map.put(name, value);
}

if (isNesting) {
stack.addLast(current);
current = value;
}
}

// End current element
if (current instanceof List) {
in.endArray();
} else {
in.endObject();
}

if (stack.isEmpty()) {
return current;
} else {
// Continue with enclosing element
current = stack.removeLast();
}
}
}

Expand Down
101 changes: 78 additions & 23 deletions gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Currency;
import java.util.GregorianCalendar;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -695,12 +696,25 @@ public void write(JsonWriter out, Locale value) throws IOException {
public static final TypeAdapterFactory LOCALE_FACTORY = newFactory(Locale.class, LOCALE);

public static final TypeAdapter<JsonElement> JSON_ELEMENT = new TypeAdapter<JsonElement>() {
@Override public JsonElement read(JsonReader in) throws IOException {
if (in instanceof JsonTreeReader) {
return ((JsonTreeReader) in).nextJsonElement();
/**
* Tries to begin reading a JSON array or JSON object, returning {@code null} if
* the next element is neither of those.
*/
private JsonElement tryBeginNesting(JsonReader in, JsonToken peeked) throws IOException {
if (peeked == JsonToken.BEGIN_ARRAY) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about switch here, and about LinkedList below. Is it possible to reduce the amount of code duplication between this class and ObjectTypeAdapter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reduce the amount of code duplication between this class and ObjectTypeAdapter?

The code indeed looks pretty similar. Maybe this can be done by having a separate internal abstract IterativeDeserializer or similar with subclasses for Object and JsonElement deserialization, but I am not sure if this wouldn't be more complex (and possibly more inefficient). Do you have a specific implementation in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to do anything about this right away, unless there is some way that people could switch between the two to circumvent any depth (etc) limitations we are trying to introduce here.

in.beginArray();
return new JsonArray();
} else if (peeked == JsonToken.BEGIN_OBJECT) {
in.beginObject();
return new JsonObject();
} else {
return null;
}
}

switch (in.peek()) {
/** Reads a {@link JsonElement} which cannot have any nested elements */
private JsonElement readTerminal(JsonReader in, JsonToken peeked) throws IOException {
switch (peeked) {
case STRING:
return new JsonPrimitive(in.nextString());
case NUMBER:
Expand All @@ -711,28 +725,69 @@ public void write(JsonWriter out, Locale value) throws IOException {
case NULL:
in.nextNull();
return JsonNull.INSTANCE;
case BEGIN_ARRAY:
JsonArray array = new JsonArray();
in.beginArray();
default:
// When read(JsonReader) is called with JsonReader in invalid state
throw new IllegalStateException("Unexpected token: " + peeked);
}
}

@Override public JsonElement read(JsonReader in) throws IOException {
if (in instanceof JsonTreeReader) {
return ((JsonTreeReader) in).nextJsonElement();
}

// Either JsonArray or JsonObject
JsonElement current;
JsonToken peeked = in.peek();

current = tryBeginNesting(in, peeked);
if (current == null) {
return readTerminal(in, peeked);
}

LinkedList<JsonElement> stack = new LinkedList<>();

while (true) {
while (in.hasNext()) {
array.add(read(in));
String name = null;
// Name is only used for JSON object members
if (current instanceof JsonObject) {
name = in.nextName();
}

peeked = in.peek();
JsonElement value = tryBeginNesting(in, peeked);
boolean isNesting = value != null;

if (value == null) {
value = readTerminal(in, peeked);
}

if (current instanceof JsonArray) {
((JsonArray) current).add(value);
} else {
((JsonObject) current).add(name, value);
}

if (isNesting) {
stack.addLast(current);
current = value;
}
}
in.endArray();
return array;
case BEGIN_OBJECT:
JsonObject object = new JsonObject();
in.beginObject();
while (in.hasNext()) {
object.add(in.nextName(), read(in));

// End current element
if (current instanceof JsonArray) {
in.endArray();
} else {
in.endObject();
}

if (stack.isEmpty()) {
return current;
} else {
// Continue with enclosing element
current = stack.removeLast();
}
in.endObject();
return object;
case END_DOCUMENT:
case NAME:
case END_OBJECT:
case END_ARRAY:
default:
throw new IllegalArgumentException();
}
}

Expand Down
41 changes: 41 additions & 0 deletions gson/src/test/java/com/google/gson/JsonParserParameterizedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package com.google.gson;

import static org.junit.Assert.assertEquals;

import java.io.IOException;
import java.util.Arrays;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class JsonParserParameterizedTest {
@Parameters
public static Iterable<String> data() {
return Arrays.asList(
"[]",
"{}",
"null",
"1.0",
"true",
"\"string\"",
"[true,1.0,null,{},2.0,{\"a\":[false]},[3.0,\"test\"],4.0]",
"{\"\":1.0,\"a\":true,\"b\":null,\"c\":[],\"d\":{\"a1\":2.0,\"b2\":[true,{\"a3\":3.0}]},\"e\":[{\"f\":4.0},\"test\"]}"
);
}

private final TypeAdapter<JsonElement> adapter = new Gson().getAdapter(JsonElement.class);
@Parameter
public String json;

@Test
public void testParse() throws IOException {
JsonElement deserialized = JsonParser.parseString(json);
String actualSerialized = adapter.toJson(deserialized);

// Serialized JsonElement should be the same as original JSON
assertEquals(json, actualSerialized);
}
}
50 changes: 49 additions & 1 deletion gson/src/test/java/com/google/gson/JsonParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

import java.io.CharArrayReader;
import java.io.CharArrayWriter;
import java.io.IOException;
import java.io.StringReader;

import junit.framework.TestCase;

import com.google.gson.common.TestTypes.BagOfPrimitives;
Expand Down Expand Up @@ -90,6 +90,54 @@ public void testParseMixedArray() {
assertEquals("stringValue", array.get(2).getAsString());
}

private static String repeat(String s, int times) {
StringBuilder stringBuilder = new StringBuilder(s.length() * times);
for (int i = 0; i < times; i++) {
stringBuilder.append(s);
}
return stringBuilder.toString();
}

/** Deeply nested JSON arrays should not cause {@link StackOverflowError} */
public void testParseDeeplyNestedArrays() throws IOException {
int times = 10000;
// [[[ ... ]]]
String json = repeat("[", times) + repeat("]", times);

int actualTimes = 0;
JsonArray current = JsonParser.parseString(json).getAsJsonArray();
while (true) {
actualTimes++;
if (current.isEmpty()) {
break;
}
assertEquals(1, current.size());
current = current.get(0).getAsJsonArray();
}
assertEquals(times, actualTimes);
}

/** Deeply nested JSON objects should not cause {@link StackOverflowError} */
public void testParseDeeplyNestedObjects() throws IOException {
int times = 10000;
// {"a":{"a": ... {"a":null} ... }}
String json = repeat("{\"a\":", times) + "null" + repeat("}", times);

int actualTimes = 0;
JsonObject current = JsonParser.parseString(json).getAsJsonObject();
while (true) {
assertEquals(1, current.size());
actualTimes++;
JsonElement next = current.get("a");
if (next.isJsonNull()) {
break;
} else {
current = next.getAsJsonObject();
}
}
assertEquals(times, actualTimes);
}

public void testParseReader() {
StringReader reader = new StringReader("{a:10,b:'c'}");
JsonElement e = JsonParser.parseReader(reader);
Expand Down
Loading