[TEST] fix shuffling of xContent keys (#23929)

ESTestCase has methods to shuffle xContent keys given a builder or a parser. Shuffling wasn't actually doing what was expected but rather reordering the keys in their natural ordering, hence the output was always the same at every run. Corrected that and added tests, also fixed a couple of tests that were affected by this fix.

core/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java

@@ -126,11 +126,8 @@ public class InnerHitBuilderTests extends ESTestCase {
             InnerHitBuilder innerHit = randomInnerHits(true, false);
             XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
             innerHit.toXContent(builder, ToXContent.EMPTY_PARAMS);
-            XContentBuilder shuffled = shuffleXContent(builder);
+            //fields is printed out as an object but parsed into a List where order matters, we disable shuffling
-            if (randomBoolean()) {
+            XContentBuilder shuffled = shuffleXContent(builder, "fields");
-                shuffled.prettyPrint();
-            }
-
             XContentParser parser = createParser(shuffled);
             QueryParseContext context = new QueryParseContext(parser);
             InnerHitBuilder secondInnerHits = InnerHitBuilder.fromXContent(context);
@@ -236,7 +233,7 @@ public class InnerHitBuilderTests extends ESTestCase {
         assertThat(innerHitBuilders.get(leafInnerHits.getName()), notNullValue());
     }
 
-    public void testBuild_ingoreUnmappedNestQuery() throws Exception {
+    public void testBuildIgnoreUnmappedNestQuery() throws Exception {
         QueryShardContext queryShardContext = mock(QueryShardContext.class);
         when(queryShardContext.getObjectMapper("path")).thenReturn(null);
         SearchContext searchContext = mock(SearchContext.class);

core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightBuilderTests.java

@@ -125,8 +125,17 @@ public class HighlightBuilderTests extends ESTestCase {
             if (randomBoolean()) {
                 builder.prettyPrint();
             }
+
+            XContentBuilder shuffled;
+            if (randomBoolean()) {
+                //this way `fields` is printed out as a json array
+                highlightBuilder.useExplicitFieldOrder(true);
                 highlightBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
-            XContentBuilder shuffled = shuffleXContent(builder);
+                shuffled = shuffleXContent(builder);
+            } else {
+                highlightBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
+                shuffled = shuffleXContent(builder, "fields");
+            }
 
             XContentParser parser = createParser(shuffled);
             QueryParseContext context = new QueryParseContext(parser);

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -117,12 +117,12 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Random;
 import java.util.Set;
-import java.util.TreeMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -919,9 +919,11 @@ public abstract class ESTestCase extends LuceneTestCase {
      * recursive shuffling behavior can be made by passing in the names of fields which
      * internally should stay untouched.
      */
-    protected static XContentBuilder shuffleXContent(XContentParser parser, boolean prettyPrint, String... exceptFieldNames) throws IOException {
+    protected static XContentBuilder shuffleXContent(XContentParser parser, boolean prettyPrint, String... exceptFieldNames)
-        //TODO why do we need sorted map if we later sort the keys right before shuffling them? That should be enough?
+            throws IOException {
-        Map<String, Object> shuffledMap = shuffleMap(parser.mapOrdered(), new HashSet<>(Arrays.asList(exceptFieldNames)));
+        //we need a sorted map for reproducibility, as we are going to shuffle its keys and write XContent back
+        Map<String, Object> shuffledMap = shuffleMap((LinkedHashMap<String, Object>)parser.mapOrdered(),
+                new HashSet<>(Arrays.asList(exceptFieldNames)));
         XContentBuilder xContentBuilder = XContentFactory.contentBuilder(parser.contentType());
         if (prettyPrint) {
             xContentBuilder.prettyPrint();
@@ -929,17 +931,15 @@ public abstract class ESTestCase extends LuceneTestCase {
         return xContentBuilder.map(shuffledMap);
     }
 
-    private static Map<String, Object> shuffleMap(Map<String, Object> map, Set<String> exceptFields) {
+    public static LinkedHashMap<String, Object> shuffleMap(LinkedHashMap<String, Object> map, Set<String> exceptFields) {
         List<String> keys = new ArrayList<>(map.keySet());
-        // even though we shuffle later, we need this to make tests reproduce on different jvms
+        LinkedHashMap<String, Object> targetMap = new LinkedHashMap<>();
-        Collections.sort(keys);
-        Map<String, Object> targetMap = new TreeMap<>();
         Collections.shuffle(keys, random());
         for (String key : keys) {
             Object value = map.get(key);
             if (value instanceof Map && exceptFields.contains(key) == false) {
                 @SuppressWarnings("unchecked")
-                Map<String, Object> valueMap = (Map<String, Object>) value;
+                LinkedHashMap<String, Object> valueMap = (LinkedHashMap<String, Object>) value;
                 targetMap.put(key, shuffleMap(valueMap, exceptFields));
             } else {
                 targetMap.put(key, value);

test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java

@@ -157,13 +157,24 @@ public final class RandomObjects {
 
     /**
      * Returns a random source in a given XContentType containing a random number of fields, objects and array, with maximum depth 5.
+     * The minimum number of fields per object is 1.
      *
      * @param random Random generator
      */
     public static BytesReference randomSource(Random random, XContentType xContentType) {
+        return randomSource(random, xContentType, 1);
+    }
+
+    /**
+     * Returns a random source in a given XContentType containing a random number of fields, objects and array, with maximum depth 5.
+     * The minimum number of fields per object is provided as an argument.
+     *
+     * @param random Random generator
+     */
+    public static BytesReference randomSource(Random random, XContentType xContentType, int minNumFields) {
         try (XContentBuilder builder = XContentFactory.contentBuilder(xContentType)) {
             builder.startObject();
-            addFields(random, builder, 0);
+            addFields(random, builder, minNumFields, 0);
             builder.endObject();
             return builder.bytes();
         } catch(IOException e) {
@@ -174,13 +185,13 @@ public final class RandomObjects {
     /**
      * Randomly adds fields, objects, or arrays to the provided builder. The maximum depth is 5.
      */
-    private static void addFields(Random random, XContentBuilder builder, int currentDepth) throws IOException {
+    private static void addFields(Random random, XContentBuilder builder, int minNumFields, int currentDepth) throws IOException {
-        int numFields = randomIntBetween(random, 1, 5);
+        int numFields = randomIntBetween(random, minNumFields, 10);
         for (int i = 0; i < numFields; i++) {
             if (currentDepth < 5 && random.nextBoolean()) {
                 if (random.nextBoolean()) {
                     builder.startObject(RandomStrings.randomAsciiOfLengthBetween(random, 4, 10));
-                    addFields(random, builder, currentDepth + 1);
+                    addFields(random, builder, minNumFields, currentDepth + 1);
                     builder.endObject();
                 } else {
                     builder.startArray(RandomStrings.randomAsciiOfLengthBetween(random, 4, 10));
@@ -193,7 +204,7 @@ public final class RandomObjects {
                     for (int j = 0; j < numElements; j++) {
                         if (object) {
                             builder.startObject();
-                            addFields(random, builder, 5);
+                            addFields(random, builder, minNumFields, 5);
                             builder.endObject();
                         } else {
                             builder.value(randomFieldValue(random, dataType));

test/framework/src/test/java/org/elasticsearch/test/test/ESTestCaseTests.java

@@ -20,18 +20,22 @@
 package org.elasticsearch.test.test;
 
 import junit.framework.AssertionFailedError;
-
+import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.RandomObjects;
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.hamcrest.Matchers.greaterThan;
@@ -65,52 +69,82 @@ public class ESTestCaseTests extends ESTestCase {
         }
     }
 
-    public void testShuffleXContent() throws IOException {
+    public void testShuffleMap() throws IOException {
-        Map<String, Object> randomStringObjectMap = randomStringObjectMap(5);
+        XContentType xContentType = randomFrom(XContentType.values());
-        XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+        BytesReference source = RandomObjects.randomSource(random(), xContentType, 5);
-        builder.map(randomStringObjectMap);
+        try (XContentParser parser = createParser(xContentType.xContent(), source)) {
-        XContentBuilder shuffleXContent = shuffleXContent(builder);
+            LinkedHashMap<String, Object> initialMap = (LinkedHashMap<String, Object>)parser.mapOrdered();
-        XContentParser parser = createParser(shuffleXContent);
+
-        Map<String, Object> resultMap = parser.map();
+            Set<List<String>> distinctKeys = new HashSet<>();
-        assertEquals("both maps should contain the same mappings", randomStringObjectMap, resultMap);
+            for (int i = 0; i < 10; i++) {
-        assertNotEquals("Both builders string representations should be different", builder.bytes(), shuffleXContent.bytes());
+                LinkedHashMap<String, Object> shuffledMap = shuffleMap(initialMap, Collections.emptySet());
+                assertEquals("both maps should contain the same mappings", initialMap, shuffledMap);
+                List<String> shuffledKeys = new ArrayList<>(shuffledMap.keySet());
+                distinctKeys.add(shuffledKeys);
+            }
+            //out of 10 shuffling runs we expect to have at least more than 1 distinct output.
+            //This is to make sure that we actually do the shuffling
+            assertThat(distinctKeys.size(), greaterThan(1));
+        }
     }
 
-    private static Map<String, Object> randomStringObjectMap(int depth) {
+    public void testShuffleXContentExcludeFields() throws IOException {
-        Map<String, Object> result = new HashMap<>();
+        XContentType xContentType = randomFrom(XContentType.values());
-        int entries = randomInt(10);
+        try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) {
-        for (int i = 0; i < entries; i++) {
+            builder.startObject();
-            String key = randomAlphaOfLengthBetween(5, 15);
+            {
-            int suprise = randomIntBetween(0, 4);
+                builder.field("field1", "value1");
-            switch (suprise) {
+                builder.field("field2", "value2");
-            case 0:
+                {
-                result.put(key, randomUnicodeOfCodepointLength(20));
+                    builder.startObject("object1");
-                break;
+                    builder.field("inner1", "value1");
-            case 1:
+                    builder.field("inner2", "value2");
-                result.put(key, randomInt(100));
+                    builder.field("inner3", "value3");
-                break;
+                    builder.endObject();
-            case 2:
-                result.put(key, randomDoubleBetween(-100.0, 100.0, true));
-                break;
-            case 3:
-                result.put(key, randomBoolean());
-                break;
-            case 4:
-                List<String> stringList = new ArrayList<>();
-                int size = randomInt(5);
-                for (int s = 0; s < size; s++) {
-                    stringList.add(randomUnicodeOfCodepointLength(20));
                 }
-                result.put(key, stringList);
+                {
-                break;
+                    builder.startObject("object2");
-            default:
+                    builder.field("inner4", "value4");
-                throw new IllegalArgumentException("unexpected random option: " + suprise);
+                    builder.field("inner5", "value5");
+                    builder.field("inner6", "value6");
+                    builder.endObject();
                 }
             }
-        if (depth > 0) {
+            builder.endObject();
-            result.put(randomAlphaOfLengthBetween(5, 15), randomStringObjectMap(depth - 1));
+            BytesReference bytes = builder.bytes();
+            final LinkedHashMap<String, Object> initialMap;
+            try (XContentParser parser = createParser(xContentType.xContent(), bytes)) {
+                initialMap = (LinkedHashMap<String, Object>)parser.mapOrdered();
+            }
+
+            List<String> expectedInnerKeys1 = Arrays.asList("inner1", "inner2", "inner3");
+            Set<List<String>> distinctTopLevelKeys = new HashSet<>();
+            Set<List<String>> distinctInnerKeys2 = new HashSet<>();
+            for (int i = 0; i < 10; i++) {
+                try (XContentParser parser = createParser(xContentType.xContent(), bytes)) {
+                    try (XContentBuilder shuffledBuilder = shuffleXContent(parser, randomBoolean(), "object1")) {
+                        try (XContentParser shuffledParser = createParser(shuffledBuilder)) {
+                            Map<String, Object> shuffledMap = shuffledParser.mapOrdered();
+                            assertEquals("both maps should contain the same mappings", initialMap, shuffledMap);
+                            List<String> shuffledKeys = new ArrayList<>(shuffledMap.keySet());
+                            distinctTopLevelKeys.add(shuffledKeys);
+                            @SuppressWarnings("unchecked")
+                            Map<String, Object> innerMap1 = (Map<String, Object>)shuffledMap.get("object1");
+                            List<String> actualInnerKeys1 = new ArrayList<>(innerMap1.keySet());
+                            assertEquals("object1 should have been left untouched", expectedInnerKeys1, actualInnerKeys1);
+                            @SuppressWarnings("unchecked")
+                            Map<String, Object> innerMap2 = (Map<String, Object>)shuffledMap.get("object2");
+                            List<String> actualInnerKeys2 = new ArrayList<>(innerMap2.keySet());
+                            distinctInnerKeys2.add(actualInnerKeys2);
+                        }
+                    }
+                }
+            }
+
+            //out of 10 shuffling runs we expect to have at least more than 1 distinct output for both top level keys and inner object2
+            assertThat(distinctTopLevelKeys.size(), greaterThan(1));
+            assertThat(distinctInnerKeys2.size(), greaterThan(1));
         }
-        return result;
     }
 
     public void testRandomUniqueNotUnique() {