Improve JsonReader.skipValue() (#2062)

* 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>

gson/src/main/java/com/google/gson/internal/bind/JsonTreeReader.java

@@ -93,6 +93,7 @@ public final class JsonTreeReader extends JsonReader {
 
   @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) {
@@ -165,16 +166,20 @@ public final class JsonTreeReader extends JsonReader {
     }
   }
 
-  @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) {
@@ -269,17 +274,26 @@ public final class JsonTreeReader extends JsonReader {
   }
 
   @Override public void skipValue() throws IOException {
-    if (peek() == JsonToken.NAME) {
+    JsonToken peeked = peek();
-      nextName();
+    switch (peeked) {
-      pathNames[stackSize - 2] = "null";
+      case NAME:
-    } else {
+        nextName(true);
-      popStack();
+        break;
-      if (stackSize > 0) {
+      case END_ARRAY:
-        pathNames[stackSize - 1] = "null";
+        endArray();
-      }
+        break;
-    }
+      case END_OBJECT:
-    if (stackSize > 0) {
+        endObject();
-      pathIndices[stackSize - 1]++;
+        break;
+      case END_DOCUMENT:
+        // Do nothing
+        break;
+      default:
+        popStack();
+        if (stackSize > 0) {
+          pathIndices[stackSize - 1]++;
+        }
+        break;
     }
   }
 

gson/src/main/java/com/google/gson/stream/JsonReader.java

@@ -777,10 +777,9 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * Returns the next token, a {@link com.google.gson.stream.JsonToken#NAME property name}, and
+   * Returns the next token, a {@link JsonToken#NAME property name}, and consumes it.
-   * 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 {
@@ -804,7 +803,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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.
    *
@@ -840,7 +839,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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
@@ -884,7 +883,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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)}.
    *
@@ -930,7 +929,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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.
@@ -1163,7 +1162,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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.
@@ -1223,7 +1222,7 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * 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;
@@ -1233,9 +1232,19 @@ public class JsonReader implements Closeable {
   }
 
   /**
-   * Skips the next value recursively. If it is an object or array, all nested
+   * Skips the next value recursively. This method is intended for use when
-   * elements are skipped. This method is intended for use when the JSON token
+   * the JSON token stream contains unrecognized or unhandled values.
-   * 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;
@@ -1245,32 +1254,69 @@ public class JsonReader implements Closeable {
         p = doPeek();
       }
 
-      if (p == PEEKED_BEGIN_ARRAY) {
+      switch (p) {
-        push(JsonScope.EMPTY_ARRAY);
+        case PEEKED_BEGIN_ARRAY:
-        count++;
+          push(JsonScope.EMPTY_ARRAY);
-      } else if (p == PEEKED_BEGIN_OBJECT) {
+          count++;
-        push(JsonScope.EMPTY_OBJECT);
+          break;
-        count++;
+        case PEEKED_BEGIN_OBJECT:
-      } else if (p == PEEKED_END_ARRAY) {
+          push(JsonScope.EMPTY_OBJECT);
-        stackSize--;
+          count++;
-        count--;
+          break;
-      } else if (p == PEEKED_END_OBJECT) {
+        case PEEKED_END_ARRAY:
-        stackSize--;
+          stackSize--;
-        count--;
+          count--;
-      } else if (p == PEEKED_UNQUOTED_NAME || p == PEEKED_UNQUOTED) {
+          break;
-        skipUnquotedValue();
+        case PEEKED_END_OBJECT:
-      } else if (p == PEEKED_SINGLE_QUOTED || p == PEEKED_SINGLE_QUOTED_NAME) {
+          // Only update when object end is explicitly skipped, otherwise stack is not updated anyways
-        skipQuotedValue('\'');
+          if (count == 0) {
-      } else if (p == PEEKED_DOUBLE_QUOTED || p == PEEKED_DOUBLE_QUOTED_NAME) {
+            pathNames[stackSize - 1] = null; // Free the last path name so that it can be garbage collected
-        skipQuotedValue('"');
+          }
-      } else if (p == PEEKED_NUMBER) {
+          stackSize--;
-        pos += peekedNumberLength;
+          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) {
@@ -1505,7 +1551,7 @@ public class JsonReader implements Closeable {
    *   <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
@@ -1522,7 +1568,7 @@ public class JsonReader implements Closeable {
    *   <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

gson/src/test/java/com/google/gson/internal/bind/JsonTreeReaderTest.java

@@ -35,6 +35,7 @@ public class JsonTreeReaderTest extends TestCase {
     JsonTreeReader in = new JsonTreeReader(new JsonObject());
     in.skipValue();
     assertEquals(JsonToken.END_DOCUMENT, in.peek());
+    assertEquals("$", in.getPath());
   }
 
   public void testSkipValue_filledJsonObject() throws IOException {
@@ -53,6 +54,46 @@ public class JsonTreeReaderTest extends TestCase {
     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 {

gson/src/test/java/com/google/gson/stream/JsonReaderPathTest.java

@@ -16,6 +16,9 @@
 
 package com.google.gson.stream;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assume.assumeTrue;
+
 import com.google.gson.JsonElement;
 import com.google.gson.internal.Streams;
 import com.google.gson.internal.bind.JsonTreeReader;
@@ -27,9 +30,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assume.assumeTrue;
-
 @SuppressWarnings("resource")
 @RunWith(Parameterized.class)
 public class JsonReaderPathTest {
@@ -221,12 +221,27 @@ public class JsonReaderPathTest {
     assertEquals("$[2]", reader.getPath());
   }
 
+  @Test public void skipArrayEnd() throws IOException {
+    JsonReader reader = factory.create("[[],1]");
+    reader.beginArray();
+    reader.beginArray();
+    assertEquals("$[0][0]", reader.getPreviousPath());
+    assertEquals("$[0][0]", reader.getPath());
+    reader.skipValue(); // skip end of array
+    assertEquals("$[0]", reader.getPreviousPath());
+    assertEquals("$[1]", reader.getPath());
+  }
+
   @Test public void skipObjectNames() throws IOException {
-    JsonReader reader = factory.create("{\"a\":1}");
+    JsonReader reader = factory.create("{\"a\":[]}");
     reader.beginObject();
     reader.skipValue();
-    assertEquals("$.null", reader.getPreviousPath());
+    assertEquals("$.<skipped>", reader.getPreviousPath());
-    assertEquals("$.null", reader.getPath());
+    assertEquals("$.<skipped>", reader.getPath());
+
+    reader.beginArray();
+    assertEquals("$.<skipped>[0]", reader.getPreviousPath());
+    assertEquals("$.<skipped>[0]", reader.getPath());
   }
 
   @Test public void skipObjectValues() throws IOException {
@@ -236,13 +251,25 @@ public class JsonReaderPathTest {
     assertEquals("$.", reader.getPath());
     reader.nextName();
     reader.skipValue();
-    assertEquals("$.null", reader.getPreviousPath());
+    assertEquals("$.a", reader.getPreviousPath());
-    assertEquals("$.null", reader.getPath());
+    assertEquals("$.a", reader.getPath());
     reader.nextName();
     assertEquals("$.b", reader.getPreviousPath());
     assertEquals("$.b", reader.getPath());
   }
 
+  @Test public void skipObjectEnd() throws IOException {
+    JsonReader reader = factory.create("{\"a\":{},\"b\":2}");
+    reader.beginObject();
+    reader.nextName();
+    reader.beginObject();
+    assertEquals("$.a.", reader.getPreviousPath());
+    assertEquals("$.a.", reader.getPath());
+    reader.skipValue(); // skip end of object
+    assertEquals("$.a", reader.getPreviousPath());
+    assertEquals("$.a", reader.getPath());
+  }
+
   @Test public void skipNestedStructures() throws IOException {
     JsonReader reader = factory.create("[[1,2,3],4]");
     reader.beginArray();
@@ -251,6 +278,20 @@ public class JsonReaderPathTest {
     assertEquals("$[1]", reader.getPath());
   }
 
+  @Test public void skipEndOfDocument() throws IOException {
+    JsonReader reader = factory.create("[]");
+    reader.beginArray();
+    reader.endArray();
+    assertEquals("$", reader.getPreviousPath());
+    assertEquals("$", reader.getPath());
+    reader.skipValue();
+    assertEquals("$", reader.getPreviousPath());
+    assertEquals("$", reader.getPath());
+    reader.skipValue();
+    assertEquals("$", reader.getPreviousPath());
+    assertEquals("$", reader.getPath());
+  }
+
   @Test public void arrayOfObjects() throws IOException {
     JsonReader reader = factory.create("[{},{},{}]");
     reader.beginArray();
@@ -307,6 +348,52 @@ public class JsonReaderPathTest {
     assertEquals("$", reader.getPath());
   }
 
+  @Test public void objectOfObjects() throws IOException {
+    JsonReader reader = factory.create("{\"a\":{\"a1\":1,\"a2\":2},\"b\":{\"b1\":1}}");
+    reader.beginObject();
+    assertEquals("$.", reader.getPreviousPath());
+    assertEquals("$.", reader.getPath());
+    reader.nextName();
+    assertEquals("$.a", reader.getPreviousPath());
+    assertEquals("$.a", reader.getPath());
+    reader.beginObject();
+    assertEquals("$.a.", reader.getPreviousPath());
+    assertEquals("$.a.", reader.getPath());
+    reader.nextName();
+    assertEquals("$.a.a1", reader.getPreviousPath());
+    assertEquals("$.a.a1", reader.getPath());
+    reader.nextInt();
+    assertEquals("$.a.a1", reader.getPreviousPath());
+    assertEquals("$.a.a1", reader.getPath());
+    reader.nextName();
+    assertEquals("$.a.a2", reader.getPreviousPath());
+    assertEquals("$.a.a2", reader.getPath());
+    reader.nextInt();
+    assertEquals("$.a.a2", reader.getPreviousPath());
+    assertEquals("$.a.a2", reader.getPath());
+    reader.endObject();
+    assertEquals("$.a", reader.getPreviousPath());
+    assertEquals("$.a", reader.getPath());
+    reader.nextName();
+    assertEquals("$.b", reader.getPreviousPath());
+    assertEquals("$.b", reader.getPath());
+    reader.beginObject();
+    assertEquals("$.b.", reader.getPreviousPath());
+    assertEquals("$.b.", reader.getPath());
+    reader.nextName();
+    assertEquals("$.b.b1", reader.getPreviousPath());
+    assertEquals("$.b.b1", reader.getPath());
+    reader.nextInt();
+    assertEquals("$.b.b1", reader.getPreviousPath());
+    assertEquals("$.b.b1", reader.getPath());
+    reader.endObject();
+    assertEquals("$.b", reader.getPreviousPath());
+    assertEquals("$.b", reader.getPath());
+    reader.endObject();
+    assertEquals("$", reader.getPreviousPath());
+    assertEquals("$", reader.getPath());
+  }
+
   public enum Factory {
     STRING_READER {
       @Override public JsonReader create(String data) {

gson/src/test/java/com/google/gson/stream/JsonReaderTest.java

@@ -16,13 +16,6 @@
 
 package com.google.gson.stream;
 
-import java.io.EOFException;
-import java.io.IOException;
-import java.io.Reader;
-import java.io.StringReader;
-import java.util.Arrays;
-import junit.framework.TestCase;
-
 import static com.google.gson.stream.JsonToken.BEGIN_ARRAY;
 import static com.google.gson.stream.JsonToken.BEGIN_OBJECT;
 import static com.google.gson.stream.JsonToken.BOOLEAN;
@@ -33,6 +26,13 @@ import static com.google.gson.stream.JsonToken.NULL;
 import static com.google.gson.stream.JsonToken.NUMBER;
 import static com.google.gson.stream.JsonToken.STRING;
 
+import java.io.EOFException;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.Arrays;
+import junit.framework.TestCase;
+
 @SuppressWarnings("resource")
 public final class JsonReaderTest extends TestCase {
   public void testReadArray() throws IOException {
@@ -140,6 +140,35 @@ public final class JsonReaderTest extends TestCase {
     assertEquals(JsonToken.END_DOCUMENT, reader.peek());
   }
 
+  public void testSkipObjectName() throws IOException {
+    JsonReader reader = new JsonReader(reader("{\"a\": 1}"));
+    reader.beginObject();
+    reader.skipValue();
+    assertEquals(JsonToken.NUMBER, reader.peek());
+    assertEquals("$.<skipped>", reader.getPath());
+    assertEquals(1, reader.nextInt());
+  }
+
+  public void testSkipObjectNameSingleQuoted() throws IOException {
+    JsonReader reader = new JsonReader(reader("{'a': 1}"));
+    reader.setLenient(true);
+    reader.beginObject();
+    reader.skipValue();
+    assertEquals(JsonToken.NUMBER, reader.peek());
+    assertEquals("$.<skipped>", reader.getPath());
+    assertEquals(1, reader.nextInt());
+  }
+
+  public void testSkipObjectNameUnquoted() throws IOException {
+    JsonReader reader = new JsonReader(reader("{a: 1}"));
+    reader.setLenient(true);
+    reader.beginObject();
+    reader.skipValue();
+    assertEquals(JsonToken.NUMBER, reader.peek());
+    assertEquals("$.<skipped>", reader.getPath());
+    assertEquals(1, reader.nextInt());
+  }
+
   public void testSkipInteger() throws IOException {
     JsonReader reader = new JsonReader(reader(
         "{\"a\":123456789,\"b\":-123456789}"));
@@ -164,6 +193,34 @@ public final class JsonReaderTest extends TestCase {
     assertEquals(JsonToken.END_DOCUMENT, reader.peek());
   }
 
+  public void testSkipValueAfterEndOfDocument() throws IOException {
+    JsonReader reader = new JsonReader(reader("{}"));
+    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 testSkipValueAtArrayEnd() throws IOException {
+    JsonReader reader = new JsonReader(reader("[]"));
+    reader.beginArray();
+    reader.skipValue();
+    assertEquals(JsonToken.END_DOCUMENT, reader.peek());
+    assertEquals("$", reader.getPath());
+  }
+
+  public void testSkipValueAtObjectEnd() throws IOException {
+    JsonReader reader = new JsonReader(reader("{}"));
+    reader.beginObject();
+    reader.skipValue();
+    assertEquals(JsonToken.END_DOCUMENT, reader.peek());
+    assertEquals("$", reader.getPath());
+  }
+
   public void testHelloWorld() throws IOException {
     String json = "{\n" +
         "   \"hello\": true,\n" +