Copilot commented on code in PR #2589:
URL: https://github.com/apache/groovy/pull/2589#discussion_r3359131325
##########
subprojects/groovy-json/src/main/java/groovy/json/JsonSlurper.java:
##########
@@ -94,6 +95,7 @@ public class JsonSlurper {
private boolean chop = false;
private boolean lazyChop = true;
private boolean checkDates = true;
+ private int maxNestingDepth = BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH;
Review Comment:
`JsonSlurper` claims the default max nesting depth can be overridden via the
`groovy.json.maxNestingDepth` system property, but this field is initialized to
the constant and then always pushed into newly created parsers, effectively
ignoring the system property unless callers explicitly call
`setMaxNestingDepth(...)`.
##########
subprojects/groovy-json/src/test/groovy/groovy/json/JsonSlurperNestingDepthTest.groovy:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.json
+
+import org.apache.groovy.json.internal.BaseJsonParser
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+
+/**
+ * Verifies that {@link JsonSlurper} (all parser types) and {@link
JsonSlurperClassic} bound the
+ * nesting depth of arrays/objects, turning a small but deeply-nested document
into a clean
+ * {@link JsonException} rather than a {@link StackOverflowError}.
+ */
+class JsonSlurperNestingDepthTest {
+
+ private static String nestedArray(int depth) {
+ '[' * depth + ']' * depth
+ }
+
+ private static String nestedObject(int depth) {
+ '{"a":' * depth + '1' + '}' * depth
+ }
+
+ @Test
+ void testDefaultMaxNestingDepth() {
+ assertEquals(BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH, new
JsonSlurper().maxNestingDepth)
+ assertEquals(1000, BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH)
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedArrays() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedArray(50)), "depth 50 should
parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(51))
+ }
+ }
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedObjects() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedObject(50)), "depth 50
should parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedObject(51))
+ }
+ }
+ }
+
+ @Test
+ void testHugeDepthThrowsJsonExceptionNotStackOverflow() {
+ // With the default cap, a pathological document fails fast with
JsonException and never
+ // reaches a StackOverflowError.
+ JsonParserType.values().each { type ->
+ def slurper = new JsonSlurper().setType(type)
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(100000))
+ }
+ }
+ }
+
+ @Test
+ void testCheckCanBeDisabled() {
+ // A value <= 0 disables the bound; a depth above the default must
then parse.
+ def slurper = new JsonSlurper().setMaxNestingDepth(0)
+ assertNotNull(slurper.parseText(nestedArray(1500)))
Review Comment:
This test disables the nesting-depth guard and then parses depth 1500, which
reintroduces deep recursion and could be flaky on environments with smaller
thread stacks. Using the minimum depth just above the default (e.g.
`DEFAULT_MAX_NESTING_DEPTH + 1`) still proves the check is disabled while
reducing StackOverflowError risk in the test itself.
##########
subprojects/groovy-json/src/test/groovy/groovy/json/JsonSlurperNestingDepthTest.groovy:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.json
+
+import org.apache.groovy.json.internal.BaseJsonParser
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+
+/**
+ * Verifies that {@link JsonSlurper} (all parser types) and {@link
JsonSlurperClassic} bound the
+ * nesting depth of arrays/objects, turning a small but deeply-nested document
into a clean
+ * {@link JsonException} rather than a {@link StackOverflowError}.
+ */
+class JsonSlurperNestingDepthTest {
+
+ private static String nestedArray(int depth) {
+ '[' * depth + ']' * depth
+ }
+
+ private static String nestedObject(int depth) {
+ '{"a":' * depth + '1' + '}' * depth
+ }
+
+ @Test
+ void testDefaultMaxNestingDepth() {
+ assertEquals(BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH, new
JsonSlurper().maxNestingDepth)
+ assertEquals(1000, BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH)
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedArrays() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedArray(50)), "depth 50 should
parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(51))
+ }
+ }
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedObjects() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedObject(50)), "depth 50
should parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedObject(51))
+ }
+ }
+ }
+
+ @Test
+ void testHugeDepthThrowsJsonExceptionNotStackOverflow() {
+ // With the default cap, a pathological document fails fast with
JsonException and never
+ // reaches a StackOverflowError.
+ JsonParserType.values().each { type ->
+ def slurper = new JsonSlurper().setType(type)
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(100000))
Review Comment:
Using a depth of 100000 makes the test allocate and concatenate a much
larger String than necessary. Since the parser should fail as soon as it
crosses the configured max nesting depth, testing with
`DEFAULT_MAX_NESTING_DEPTH + 1` keeps the intent while reducing test time and
memory.
##########
subprojects/groovy-json/src/test/groovy/groovy/json/JsonSlurperNestingDepthTest.groovy:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.json
+
+import org.apache.groovy.json.internal.BaseJsonParser
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+
+/**
+ * Verifies that {@link JsonSlurper} (all parser types) and {@link
JsonSlurperClassic} bound the
+ * nesting depth of arrays/objects, turning a small but deeply-nested document
into a clean
+ * {@link JsonException} rather than a {@link StackOverflowError}.
+ */
+class JsonSlurperNestingDepthTest {
+
+ private static String nestedArray(int depth) {
+ '[' * depth + ']' * depth
+ }
+
+ private static String nestedObject(int depth) {
+ '{"a":' * depth + '1' + '}' * depth
+ }
+
+ @Test
+ void testDefaultMaxNestingDepth() {
+ assertEquals(BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH, new
JsonSlurper().maxNestingDepth)
+ assertEquals(1000, BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH)
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedArrays() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedArray(50)), "depth 50 should
parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(51))
+ }
+ }
+ }
+
+ @Test
+ void testAllParserTypesRejectDeeplyNestedObjects() {
+ JsonParserType.values().each { type ->
+ def slurper = new
JsonSlurper().setType(type).setMaxNestingDepth(50)
+ assertNotNull(slurper.parseText(nestedObject(50)), "depth 50
should parse for $type")
+ shouldFail(JsonException) {
+ slurper.parseText(nestedObject(51))
+ }
+ }
+ }
+
+ @Test
+ void testHugeDepthThrowsJsonExceptionNotStackOverflow() {
+ // With the default cap, a pathological document fails fast with
JsonException and never
+ // reaches a StackOverflowError.
+ JsonParserType.values().each { type ->
+ def slurper = new JsonSlurper().setType(type)
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(100000))
+ }
+ }
+ }
+
+ @Test
+ void testCheckCanBeDisabled() {
+ // A value <= 0 disables the bound; a depth above the default must
then parse.
+ def slurper = new JsonSlurper().setMaxNestingDepth(0)
+ assertNotNull(slurper.parseText(nestedArray(1500)))
+ }
+
+ @Test
+ void testClassicRejectsDeeplyNested() {
+ def slurper = new JsonSlurperClassic()
+ slurper.maxNestingDepth = 50
+ assertNotNull(slurper.parseText(nestedArray(50)))
+ assertNotNull(slurper.parseText(nestedObject(50)))
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(51))
+ }
+ shouldFail(JsonException) {
+ slurper.parseText(nestedObject(51))
+ }
+ }
+
+ @Test
+ void testClassicHugeDepthThrowsJsonExceptionNotStackOverflow() {
+ def slurper = new JsonSlurperClassic()
+ shouldFail(JsonException) {
+ slurper.parseText(nestedArray(100000))
Review Comment:
As above, depth 100000 allocates a much larger String than needed to verify
the default max nesting depth guard. Using `DEFAULT_MAX_NESTING_DEPTH + 1`
keeps the same assertion while reducing runtime and memory for the test suite.
##########
subprojects/groovy-json/src/test/groovy/groovy/json/JsonSlurperNestingDepthTest.groovy:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.json
+
+import org.apache.groovy.json.internal.BaseJsonParser
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.shouldFail
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+
+/**
+ * Verifies that {@link JsonSlurper} (all parser types) and {@link
JsonSlurperClassic} bound the
+ * nesting depth of arrays/objects, turning a small but deeply-nested document
into a clean
+ * {@link JsonException} rather than a {@link StackOverflowError}.
+ */
+class JsonSlurperNestingDepthTest {
+
+ private static String nestedArray(int depth) {
+ '[' * depth + ']' * depth
+ }
+
+ private static String nestedObject(int depth) {
+ '{"a":' * depth + '1' + '}' * depth
+ }
+
+ @Test
+ void testDefaultMaxNestingDepth() {
+ assertEquals(BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH, new
JsonSlurper().maxNestingDepth)
+ assertEquals(1000, BaseJsonParser.DEFAULT_MAX_NESTING_DEPTH)
+ }
Review Comment:
This test assumes `groovy.json.maxNestingDepth` is not set externally. Since
the new API explicitly supports global override via system property, the test
should isolate itself from ambient JVM properties (save/clear/restore) to avoid
failures in builds that set this property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
