[
https://issues.apache.org/jira/browse/GROOVY-12064?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18086209#comment-18086209
]
ASF GitHub Bot commented on GROOVY-12064:
-----------------------------------------
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.
> Provide a maximum depth threshold for parsing deeply nested JSON documents
> --------------------------------------------------------------------------
>
> Key: GROOVY-12064
> URL: https://issues.apache.org/jira/browse/GROOVY-12064
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
