Skip to content

Commit

Permalink
Improve JsonReader.skipValue() (#2062)
Browse files Browse the repository at this point in the history 
* Fix JsonReader.skipValue() not behaving properly at end of document

JsonReader implementation erroneously reset `peeked` to PEEKED_NONE;
JsonTreeReader threw ArrayIndexOutOfBoundsException.

* Fix JsonReader.skipValue() not behaving properly at end of array and object

For JsonReader this caused an IllegalStateException (in the past it caused
JsonReader to get stuck in an infinite loop); for JsonTreeReader it only
popped the empty iterator but not the JsonArray or JsonObject, which caused
peek() to again report END_ARRAY or END_OBJECT.

* Only have JsonReader.skipValue() overwrite path name when name was skipped

This improves the JSON path when the value for a property was skipped and
before the subsequent property (or the end of the object) getPath() is called.

* Address feedback; improve test coverage

Co-authored-by: Éamonn McManus <emcmanus@google.com>
  • Loading branch information
@Marcono1234 @eamonnmcmanus
Marcono1234 and eamonnmcmanus authored Oct 10, 2022
1 parent e614e71 commit 5269701
Show file tree
Hide file tree
Showing 5 changed files with 309 additions and 64 deletions.
40 changes: 27 additions & 13 deletions gson/src/main/java/com/google/gson/internal/bind/JsonTreeReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public JsonTreeReader(JsonElement element) {

@Override public void endObject() throws IOException {
expect(JsonToken.END_OBJECT);
pathNames[stackSize - 1] = null; // Free the last path name so that it can be garbage collected
popStack(); // empty iterator
popStack(); // object
if (stackSize > 0) {
Expand Down Expand Up @@ -165,16 +166,20 @@ private void expect(JsonToken expected) throws IOException {
}
}

@Override public String nextName() throws IOException {
private String nextName(boolean skipName) throws IOException {
expect(JsonToken.NAME);
Iterator<?> i = (Iterator<?>) peekStack();
Map.Entry<?, ?> entry = (Map.Entry<?, ?>) i.next();
String result = (String) entry.getKey();
pathNames[stackSize - 1] = result;
pathNames[stackSize - 1] = skipName ? "<skipped>" : result;
push(entry.getValue());
return result;
}

@Override public String nextName() throws IOException {
return nextName(false);
}

@Override public String nextString() throws IOException {
JsonToken token = peek();
if (token != JsonToken.STRING && token != JsonToken.NUMBER) {
Expand Down Expand Up @@ -269,17 +274,26 @@ JsonElement nextJsonElement() throws IOException {
}

@Override public void skipValue() throws IOException {
if (peek() == JsonToken.NAME) {
nextName();
pathNames[stackSize - 2] = "null";
} else {
popStack();
if (stackSize > 0) {
pathNames[stackSize - 1] = "null";
}
}
if (stackSize > 0) {
pathIndices[stackSize - 1]++;
JsonToken peeked = peek();
switch (peeked) {
case NAME:
nextName(true);
break;
case END_ARRAY:
endArray();
break;
case END_OBJECT:
endObject();
break;
case END_DOCUMENT:
// Do nothing
break;
default:
popStack();
if (stackSize > 0) {
pathIndices[stackSize - 1]++;
}
break;
}
}

Expand Down
118 changes: 82 additions & 36 deletions gson/src/main/java/com/google/gson/stream/JsonReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,10 +777,9 @@ private boolean isLiteral(char c) throws IOException {
}

/**
* Returns the next token, a {@link com.google.gson.stream.JsonToken#NAME property name}, and
* consumes it.
* Returns the next token, a {@link JsonToken#NAME property name}, and consumes it.
*
* @throws java.io.IOException if the next token in the stream is not a property
* @throws IOException if the next token in the stream is not a property
* name.
*/
public String nextName() throws IOException {
Expand All @@ -804,7 +803,7 @@ public String nextName() throws IOException {
}

/**
* Returns the {@link com.google.gson.stream.JsonToken#STRING string} value of the next token,
* Returns the {@link JsonToken#STRING string} value of the next token,
* consuming it. If the next token is a number, this method will return its
* string form.
*
Expand Down Expand Up @@ -840,7 +839,7 @@ public String nextString() throws IOException {
}

/**
* Returns the {@link com.google.gson.stream.JsonToken#BOOLEAN boolean} value of the next token,
* Returns the {@link JsonToken#BOOLEAN boolean} value of the next token,
* consuming it.
*
* @throws IllegalStateException if the next token is not a boolean or if
Expand Down Expand Up @@ -884,7 +883,7 @@ public void nextNull() throws IOException {
}

/**
* Returns the {@link com.google.gson.stream.JsonToken#NUMBER double} value of the next token,
* Returns the {@link JsonToken#NUMBER double} value of the next token,
* consuming it. If the next token is a string, this method will attempt to
* parse it as a double using {@link Double#parseDouble(String)}.
*
Expand Down Expand Up @@ -930,7 +929,7 @@ public double nextDouble() throws IOException {
}

/**
* Returns the {@link com.google.gson.stream.JsonToken#NUMBER long} value of the next token,
* Returns the {@link JsonToken#NUMBER long} value of the next token,
* consuming it. If the next token is a string, this method will attempt to
* parse it as a long. If the next token's numeric value cannot be exactly
* represented by a Java {@code long}, this method throws.
Expand Down Expand Up @@ -1163,7 +1162,7 @@ private void skipUnquotedValue() throws IOException {
}

/**
* Returns the {@link com.google.gson.stream.JsonToken#NUMBER int} value of the next token,
* Returns the {@link JsonToken#NUMBER int} value of the next token,
* consuming it. If the next token is a string, this method will attempt to
* parse it as an int. If the next token's numeric value cannot be exactly
* represented by a Java {@code int}, this method throws.
Expand Down Expand Up @@ -1223,7 +1222,7 @@ public int nextInt() throws IOException {
}

/**
* Closes this JSON reader and the underlying {@link java.io.Reader}.
* Closes this JSON reader and the underlying {@link Reader}.
*/
@Override public void close() throws IOException {
peeked = PEEKED_NONE;
Expand All @@ -1233,9 +1232,19 @@ public int nextInt() throws IOException {
}

/**
* Skips the next value recursively. If it is an object or array, all nested
* elements are skipped. This method is intended for use when the JSON token
* stream contains unrecognized or unhandled values.
* Skips the next value recursively. This method is intended for use when
* the JSON token stream contains unrecognized or unhandled values.
*
* <p>The behavior depends on the type of the next JSON token:
* <ul>
* <li>Start of a JSON array or object: It and all of its nested values are skipped.</li>
* <li>Primitive value (for example a JSON number): The primitive value is skipped.</li>
* <li>Property name: Only the name but not the value of the property is skipped.
* {@code skipValue()} has to be called again to skip the property value as well.</li>
* <li>End of a JSON array or object: Only this end token is skipped.</li>
* <li>End of JSON document: Skipping has no effect, the next token continues to be the
* end of the document.</li>
* </ul>
*/
public void skipValue() throws IOException {
int count = 0;
Expand All @@ -1245,32 +1254,69 @@ public void skipValue() throws IOException {
p = doPeek();
}

if (p == PEEKED_BEGIN_ARRAY) {
push(JsonScope.EMPTY_ARRAY);
count++;
} else if (p == PEEKED_BEGIN_OBJECT) {
push(JsonScope.EMPTY_OBJECT);
count++;
} else if (p == PEEKED_END_ARRAY) {
stackSize--;
count--;
} else if (p == PEEKED_END_OBJECT) {
stackSize--;
count--;
} else if (p == PEEKED_UNQUOTED_NAME || p == PEEKED_UNQUOTED) {
skipUnquotedValue();
} else if (p == PEEKED_SINGLE_QUOTED || p == PEEKED_SINGLE_QUOTED_NAME) {
skipQuotedValue('\'');
} else if (p == PEEKED_DOUBLE_QUOTED || p == PEEKED_DOUBLE_QUOTED_NAME) {
skipQuotedValue('"');
} else if (p == PEEKED_NUMBER) {
pos += peekedNumberLength;
switch (p) {
case PEEKED_BEGIN_ARRAY:
push(JsonScope.EMPTY_ARRAY);
count++;
break;
case PEEKED_BEGIN_OBJECT:
push(JsonScope.EMPTY_OBJECT);
count++;
break;
case PEEKED_END_ARRAY:
stackSize--;
count--;
break;
case PEEKED_END_OBJECT:
// Only update when object end is explicitly skipped, otherwise stack is not updated anyways
if (count == 0) {
pathNames[stackSize - 1] = null; // Free the last path name so that it can be garbage collected
}
stackSize--;
count--;
break;
case PEEKED_UNQUOTED:
skipUnquotedValue();
break;
case PEEKED_SINGLE_QUOTED:
skipQuotedValue('\'');
break;
case PEEKED_DOUBLE_QUOTED:
skipQuotedValue('"');
break;
case PEEKED_UNQUOTED_NAME:
skipUnquotedValue();
// Only update when name is explicitly skipped, otherwise stack is not updated anyways
if (count == 0) {
pathNames[stackSize - 1] = "<skipped>";
}
break;
case PEEKED_SINGLE_QUOTED_NAME:
skipQuotedValue('\'');
// Only update when name is explicitly skipped, otherwise stack is not updated anyways
if (count == 0) {
pathNames[stackSize - 1] = "<skipped>";
}
break;
case PEEKED_DOUBLE_QUOTED_NAME:
skipQuotedValue('"');
// Only update when name is explicitly skipped, otherwise stack is not updated anyways
if (count == 0) {
pathNames[stackSize - 1] = "<skipped>";
}
break;
case PEEKED_NUMBER:
pos += peekedNumberLength;
break;
case PEEKED_EOF:
// Do nothing
return;
// For all other tokens there is nothing to do; token has already been consumed from underlying reader
}
peeked = PEEKED_NONE;
} while (count != 0);
} while (count > 0);

pathIndices[stackSize - 1]++;
pathNames[stackSize - 1] = "null";
}

private void push(int newTop) {
Expand Down Expand Up @@ -1505,7 +1551,7 @@ private String getPath(boolean usePreviousPath) {
* <li>For JSON arrays the path points to the index of the previous element.<br>
* If no element has been consumed yet it uses the index 0 (even if there are no elements).</li>
* <li>For JSON objects the path points to the last property, or to the current
* property if its value has not been consumed yet.</li>
* property if its name has already been consumed.</li>
* </ul>
*
* <p>This method can be useful to add additional context to exception messages
Expand All @@ -1522,7 +1568,7 @@ public String getPreviousPath() {
* <li>For JSON arrays the path points to the index of the next element (even
* if there are no further elements).</li>
* <li>For JSON objects the path points to the last property, or to the current
* property if its value has not been consumed yet.</li>
* property if its name has already been consumed.</li>
* </ul>
*
* <p>This method can be useful to add additional context to exception messages
Expand Down
41 changes: 41 additions & 0 deletions gson/src/test/java/com/google/gson/internal/bind/JsonTreeReaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public void testSkipValue_emptyJsonObject() throws IOException {
JsonTreeReader in = new JsonTreeReader(new JsonObject());
in.skipValue();
assertEquals(JsonToken.END_DOCUMENT, in.peek());
assertEquals("$", in.getPath());
}

public void testSkipValue_filledJsonObject() throws IOException {
Expand All @@ -53,6 +54,46 @@ public void testSkipValue_filledJsonObject() throws IOException {
JsonTreeReader in = new JsonTreeReader(jsonObject);
in.skipValue();
assertEquals(JsonToken.END_DOCUMENT, in.peek());
assertEquals("$", in.getPath());
}

public void testSkipValue_name() throws IOException {
JsonObject jsonObject = new JsonObject();
jsonObject.addProperty("a", "value");
JsonTreeReader in = new JsonTreeReader(jsonObject);
in.beginObject();
in.skipValue();
assertEquals(JsonToken.STRING, in.peek());
assertEquals("$.<skipped>", in.getPath());
assertEquals("value", in.nextString());
}

public void testSkipValue_afterEndOfDocument() throws IOException {
JsonTreeReader reader = new JsonTreeReader(new JsonObject());
reader.beginObject();
reader.endObject();
assertEquals(JsonToken.END_DOCUMENT, reader.peek());

assertEquals("$", reader.getPath());
reader.skipValue();
assertEquals(JsonToken.END_DOCUMENT, reader.peek());
assertEquals("$", reader.getPath());
}

public void testSkipValue_atArrayEnd() throws IOException {
JsonTreeReader reader = new JsonTreeReader(new JsonArray());
reader.beginArray();
reader.skipValue();
assertEquals(JsonToken.END_DOCUMENT, reader.peek());
assertEquals("$", reader.getPath());
}

public void testSkipValue_atObjectEnd() throws IOException {
JsonTreeReader reader = new JsonTreeReader(new JsonObject());
reader.beginObject();
reader.skipValue();
assertEquals(JsonToken.END_DOCUMENT, reader.peek());
assertEquals("$", reader.getPath());
}

public void testHasNext_endOfDocument() throws IOException {
Expand Down
Loading

0 comments on commit 5269701

Please sign in to comment.