diff --git a/java/src/org/openqa/selenium/json/JsonInput.java b/java/src/org/openqa/selenium/json/JsonInput.java index 10e891e87eb17..d4b23c9760b47 100644 --- a/java/src/org/openqa/selenium/json/JsonInput.java +++ b/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 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 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); } } diff --git a/java/test/org/openqa/selenium/json/JsonInputTest.java b/java/test/org/openqa/selenium/json/JsonInputTest.java index 037c5714c5ca7..23ba6734759f8 100644 --- a/java/test/org/openqa/selenium/json/JsonInputTest.java +++ b/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]);