Avoid doing redundant work when checking for self references. (#26927)

Currently we test all maps, arrays or iterables. However, in the case that maps
contain sub maps for instance, we will test the sub maps again even though the
work has already been done for the top-level map.

Relates #26907

Author: jpountz

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -773,32 +773,23 @@ public XContentBuilder field(String name, Object value) throws IOException {
     }
 
     public XContentBuilder array(String name, Object... values) throws IOException {
-        return field(name).values(values);
+        return field(name).values(values, true);
     }
 
-    XContentBuilder values(Object[] values) throws IOException {
+    private XContentBuilder values(Object[] values, boolean ensureNoSelfReferences) throws IOException {
         if (values == null) {
             return nullValue();
         }
 
-        // checks that the array of object does not contain references to itself because
-        // iterating over entries will cause a stackoverflow error
-        ensureNoSelfReferences(values);
-
-        startArray();
-        for (Object o : values) {
-            value(o);
-        }
-        endArray();
-        return this;
+        return value(Arrays.asList(values), ensureNoSelfReferences);
     }
 
     public XContentBuilder value(Object value) throws IOException {
-        unknownValue(value);
+        unknownValue(value, true);
         return this;
     }
 
-    private void unknownValue(Object value) throws IOException {
+    private void unknownValue(Object value, boolean ensureNoSelfReferences) throws IOException {
         if (value == null) {
             nullValue();
             return;
@@ -810,11 +801,11 @@ private void unknownValue(Object value) throws IOException {
             //Path implements Iterable<Path> and causes endless recursion and a StackOverFlow if treated as an Iterable here
             value((Path) value);
         } else if (value instanceof Map) {
-            map((Map) value);
+            map((Map<String,?>) value, ensureNoSelfReferences);
         } else if (value instanceof Iterable) {
-            value((Iterable<?>) value);
+            value((Iterable<?>) value, ensureNoSelfReferences);
         } else if (value instanceof Object[]) {
-            values((Object[]) value);
+            values((Object[]) value, ensureNoSelfReferences);
         } else if (value instanceof Calendar) {
             value((Calendar) value);
         } else if (value instanceof ReadableInstant) {
@@ -863,18 +854,25 @@ public XContentBuilder field(String name, Map<String, Object> values) throws IOE
     }
 
     public XContentBuilder map(Map<String, ?> values) throws IOException {
+        return map(values, true);
+    }
+
+    private XContentBuilder map(Map<String, ?> values, boolean ensureNoSelfReferences) throws IOException {
         if (values == null) {
             return nullValue();
         }
 
         // checks that the map does not contain references to itself because
         // iterating over map entries will cause a stackoverflow error
-        ensureNoSelfReferences(values);
+        if (ensureNoSelfReferences) {
+            ensureNoSelfReferences(values);
+        }
 
         startObject();
         for (Map.Entry<String, ?> value : values.entrySet()) {
             field(value.getKey());
-            unknownValue(value.getValue());
+            // pass ensureNoSelfReferences=false as we already performed the check at a higher level
+            unknownValue(value.getValue(), false);
         }
         endObject();
         return this;
@@ -884,7 +882,7 @@ public XContentBuilder field(String name, Iterable<?> values) throws IOException
         return field(name).value(values);
     }
 
-    private XContentBuilder value(Iterable<?> values) throws IOException {
+    private XContentBuilder value(Iterable<?> values, boolean ensureNoSelfReferences) throws IOException {
         if (values == null) {
             return nullValue();
         }
@@ -895,11 +893,14 @@ private XContentBuilder value(Iterable<?> values) throws IOException {
         } else {
             // checks that the iterable does not contain references to itself because
             // iterating over entries will cause a stackoverflow error
-            ensureNoSelfReferences(values);
+            if (ensureNoSelfReferences) {
+                ensureNoSelfReferences(values);
+            }
 
             startArray();
             for (Object value : values) {
-                unknownValue(value);
+                // pass ensureNoSelfReferences=false as we already performed the check at a higher level
+                unknownValue(value, false);
             }
             endArray();
         }
@@ -1076,9 +1077,9 @@ private static void ensureNoSelfReferences(final Object value, final Set<Object>
 
             Iterable<?> it;
             if (value instanceof Map) {
-                it = ((Map) value).values();
+                it = ((Map<?,?>) value).values();
             } else if ((value instanceof Iterable) && (value instanceof Path == false)) {
-                it = (Iterable) value;
+                it = (Iterable<?>) value;
             } else if (value instanceof Object[]) {
                 it = Arrays.asList((Object[]) value);
             } else {

server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java

@@ -534,7 +534,6 @@ public void testObjects() throws Exception {
             final String expected = o.getKey();
             assertResult(expected, () -> builder().startObject().field("objects", o.getValue()).endObject());
             assertResult(expected, () -> builder().startObject().field("objects").value(o.getValue()).endObject());
-            assertResult(expected, () -> builder().startObject().field("objects").values(o.getValue()).endObject());
             assertResult(expected, () -> builder().startObject().array("objects", o.getValue()).endObject());
         }
     }