[java] Enforce commas between JSON elements; accept trailing commas

java/src/org/openqa/selenium/json/JsonInput.java

@@ -49,6 +49,10 @@ public class JsonInput implements Closeable {
   // Used when reading maps and collections so that we handle de-nesting and
   // figuring out whether we're expecting a NAME properly.
   private final Deque<Container> stack = new ArrayDeque<>();
+  // Parallel stack tracking whether the current container has seen at least
+  // one element. Used by hasNext() to enforce comma separators between
+  // elements while remaining lenient about a single trailing comma.
+  private final Deque<Boolean> containerHasElement = new ArrayDeque<>();
 
   JsonInput(Reader source, JsonTypeCoercer coercer, PropertySetting setter) {
 
@@ -353,13 +357,30 @@ public boolean hasNext() {
     }
 
     skipWhitespace(input);
+    boolean seenElement = Boolean.TRUE.equals(containerHasElement.peekFirst());
+
     if (input.peek() == ',') {
+      if (!seenElement) {
+        throw new JsonException("Unexpected ',' before first element of container. " + input);
+      }
       input.read();
-      return true;
+      // We've moved past the separator, so we're once again expecting an element rather than
+      // another comma. Clear the flag so a repeat hasNext() before reading is a no-op.
+      clearSeenElement();
+      skipWhitespace(input);
+      JsonType afterComma = peek();
+      // Trailing comma leniency: '[1,]' and '{"a":1,}' are accepted.
+      return afterComma != JsonType.END_COLLECTION && afterComma != JsonType.END_MAP;
     }
 
     JsonType type = peek();
-    return type != JsonType.END_COLLECTION && type != JsonType.END_MAP;
+    if (type == JsonType.END_COLLECTION || type == JsonType.END_MAP) {
+      return false;
+    }
+    if (seenElement) {
+      throw new JsonException("Expected ',' or end of container but saw " + type + ". " + input);
+    }
+    return true;
   }
 
   /**
@@ -370,6 +391,7 @@ public boolean hasNext() {
   public void beginArray() {
     expect(JsonType.START_COLLECTION);
     stack.addFirst(Container.COLLECTION);
+    containerHasElement.addFirst(false);
     input.read();
   }
 
@@ -380,12 +402,13 @@ public void beginArray() {
    */
   public void endArray() {
     expect(JsonType.END_COLLECTION);
-    Container expectation = stack.removeFirst();
-    if (expectation != Container.COLLECTION) {
+    if (stack.peekFirst() != Container.COLLECTION) {
       // The only other thing we could be closing is a map
       throw new JsonException(
           "Attempt to close a JSON List, but a JSON Object was expected. " + input);
     }
+    stack.removeFirst();
+    containerHasElement.removeFirst();
     input.read();
   }
 
@@ -397,6 +420,7 @@ public void endArray() {
   public void beginObject() {
     expect(JsonType.START_MAP);
     stack.addFirst(Container.MAP_NAME);
+    containerHasElement.addFirst(false);
     input.read();
   }
 
@@ -407,11 +431,11 @@ public void beginObject() {
    */
   public void endObject() {
     expect(JsonType.END_MAP);
-    Container expectation = stack.removeFirst();
-    if (expectation != Container.MAP_NAME) {
-      // The only other thing we could be closing is a map
+    if (stack.peekFirst() != Container.MAP_NAME) {
       throw new JsonException("Attempt to close a JSON Map, but not ready to. " + input);
     }
+    stack.removeFirst();
+    containerHasElement.removeFirst();
     input.read();
   }
 
@@ -565,10 +589,31 @@ private void expect(JsonType type) {
       return; // End of Name handling
     }
 
-    // Handle the case where we're reading a value
+    // Handle the case where we're reading a value.
+    if (type == JsonType.END_COLLECTION || type == JsonType.END_MAP) {
+      // Closing the container - don't treat as a new element in it.
+      return;
+    }
     if (top == Container.MAP_VALUE) {
       stack.removeFirst();
       stack.addFirst(Container.MAP_NAME);
+      markElementRead();
+    } else if (top == Container.COLLECTION) {
+      markElementRead();
+    }
+  }
+
+  private void markElementRead() {
+    if (!containerHasElement.isEmpty()) {
+      containerHasElement.removeFirst();
+      containerHasElement.addFirst(true);
+    }
+  }
+
+  private void clearSeenElement() {
+    if (!containerHasElement.isEmpty()) {
+      containerHasElement.removeFirst();
+      containerHasElement.addFirst(false);
     }
   }
 

java/test/org/openqa/selenium/json/JsonInputTest.java

@@ -384,6 +384,119 @@ void shouldReportEndOfInputAsEofNotAsAUnicodeReplacement() {
     }
   }
 
+  @Test
+  void shouldAcceptTrailingCommaInArray() {
+    try (JsonInput input = newInput("[1,2,3,]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(3L);
+      assertThat(input.hasNext()).isFalse();
+      input.endArray();
+    }
+  }
+
+  @Test
+  void shouldAcceptTrailingCommaInObject() {
+    try (JsonInput input = newInput("{\"a\":1,}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isFalse();
+      input.endObject();
+    }
+  }
+
+  @Test
+  void shouldRejectMissingCommaBetweenArrayElements() {
+    try (JsonInput input = newInput("[1 2]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectLeadingCommaInArray() {
+    try (JsonInput input = newInput("[,1]")) {
+      input.beginArray();
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectMissingCommaBetweenObjectEntries() {
+    try (JsonInput input = newInput("{\"a\":1 \"b\":2}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectLeadingCommaInObject() {
+    try (JsonInput input = newInput("{,\"a\":1}")) {
+      input.beginObject();
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void mismatchedCloseLeavesParserStateIntact() {
+    // endObject() while inside an array must fail without popping the container stack,
+    // so the parser still knows it is inside the array afterwards.
+    try (JsonInput input = newInput("[1}")) {
+      input.beginArray();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class)
+          .isThrownBy(input::endObject)
+          .withMessageStartingWith("Attempt to close a JSON Map");
+      // Before the fix this threw "not in a container type" because the array's
+      // stack entry had already been popped.
+      assertThat(input.hasNext()).isFalse();
+    }
+  }
+
+  @Test
+  void hasNextIsIdempotentBetweenElementsInArray() {
+    // Iterator-style probing (peek/hasNext repeatedly before reading) must not falsely fail
+    // once hasNext() has already consumed the comma before the next element.
+    try (JsonInput input = newInput("[1,2]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isFalse();
+      assertThat(input.hasNext()).isFalse();
+      input.endArray();
+    }
+  }
+
+  @Test
+  void hasNextIsIdempotentBetweenEntriesInObject() {
+    try (JsonInput input = newInput("{\"a\":1,\"b\":2}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("b");
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isFalse();
+      input.endObject();
+    }
+  }
+
   @Test
   void nullInputsShouldCoerceAsNullValues() throws IOException {
     try (InputStream is = new ByteArrayInputStream(new byte[0]);