Do not fail on duplicated content field filters (#85382)

`Set.of()` throws a runtime exception if it encounters a duplicated element, `Set.copyOf`, on the contrary, doesn't do that. We shouldn't fail if the user configure multiple include or exclude filters for _source fields. See https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-source-field.html#include-exclude
elastic · Mar 29, 2022 · 50d1b4f · 50d1b4f
1 parent 9071f59
commit 50d1b4f
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 9 deletions.
diff --git a/docs/changelog/85382.yaml b/docs/changelog/85382.yaml
@@ -0,0 +1,5 @@
+pr: 85382
+summary: Do not fail on duplicated content field filters
+area: Mapping
+type: bug
+issues: []
diff --git a/.../java/org/elasticsearch/xcontent/support/filtering/AbstractXContentFilteringTestCase.java b/.../java/org/elasticsearch/xcontent/support/filtering/AbstractXContentFilteringTestCase.java
@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Set;
 import java.util.stream.IntStream;
 
@@ -324,8 +325,9 @@ public void testDotsAndDoubleWildcardInExcludedFieldName() throws IOException {
     }
 
     @Override
-    protected final void testFilter(Builder expected, Builder sample, Set<String> includes, Set<String> excludes) throws IOException {
-        testFilter(expected, sample, includes, excludes, false);
+    protected final void testFilter(Builder expected, Builder sample, Collection<String> includes, Collection<String> excludes)
+        throws IOException {
+        testFilter(expected, sample, Set.copyOf(includes), Set.copyOf(excludes), false);
     }
 
     private void testFilter(Builder expected, Builder sample, Set<String> includes, Set<String> excludes, boolean matchFieldNamesWithDots)

diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java
@@ -73,8 +73,8 @@ static XContentFieldFilter newFieldFilter(String[] includes, String[] excludes)
             };
         } else {
             final XContentParserConfiguration parserConfig = XContentParserConfiguration.EMPTY.withFiltering(
-                Set.of(includes),
-                Set.of(excludes),
+                Set.copyOf(Arrays.asList(includes)),
+                Set.copyOf(Arrays.asList(excludes)),
                 true
             );
             return (originalSource, contentType) -> {

diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentFieldFilterTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentFieldFilterTests.java
@@ -23,7 +23,9 @@
 import org.elasticsearch.xcontent.XContentType;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -36,7 +38,8 @@
 
 public class XContentFieldFilterTests extends AbstractFilteringTestCase {
     @Override
-    protected void testFilter(Builder expected, Builder actual, Set<String> includes, Set<String> excludes) throws IOException {
+    protected void testFilter(Builder expected, Builder actual, Collection<String> includes, Collection<String> excludes)
+        throws IOException {
         final XContentType xContentType = randomFrom(XContentType.values());
         final boolean humanReadable = randomBoolean();
         String[] sourceIncludes;
@@ -56,7 +59,8 @@ protected void testFilter(Builder expected, Builder actual, Set<String> includes
         assertMap(XContentHelper.convertToMap(ref, true, xContentType).v2(), matchesMap(toMap(expected, xContentType, humanReadable)));
     }
 
-    private void testFilter(String expectedJson, String actualJson, Set<String> includes, Set<String> excludes) throws IOException {
+    private void testFilter(String expectedJson, String actualJson, Collection<String> includes, Collection<String> excludes)
+        throws IOException {
         CheckedFunction<String, Builder, IOException> toBuilder = json -> {
             XContentParser parser = XContentHelper.createParser(XContentParserConfiguration.EMPTY, new BytesArray(json), XContentType.JSON);
             if ((parser.currentToken() == null) && (parser.nextToken() == null)) {
@@ -82,6 +86,36 @@ public void testPrefixedNamesFilteringTest() throws IOException {
         testFilter(expected, actual, singleton("obj_name"), emptySet());
     }
 
+    public void testDuplicatedIncludes() throws IOException {
+        String actual = """
+            {
+                "obj": "value",
+                "obj_name": "value_name"
+            }
+            """;
+        String expected = """
+            {
+                "obj_name": "value_name"
+            }
+            """;
+        testFilter(expected, actual, List.of("obj_name", "obj_name"), emptySet());
+    }
+
+    public void testDuplicatedExcludes() throws IOException {
+        String actual = """
+            {
+                "obj": "value",
+                "obj_name": "value_name"
+            }
+            """;
+        String expected = """
+            {
+                "obj": "value"
+            }
+            """;
+        testFilter(expected, actual, emptySet(), List.of("obj_name", "obj_name"));
+    }
+
     public void testNestedFiltering() throws IOException {
         String actual = """
             {

diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java
@@ -22,11 +22,11 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 import static org.elasticsearch.common.xcontent.XContentHelper.convertToMap;
 import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
@@ -44,7 +44,8 @@
 public class XContentMapValuesTests extends AbstractFilteringTestCase {
 
     @Override
-    protected void testFilter(Builder expected, Builder actual, Set<String> includes, Set<String> excludes) throws IOException {
+    protected void testFilter(Builder expected, Builder actual, Collection<String> includes, Collection<String> excludes)
+        throws IOException {
         final XContentType xContentType = randomFrom(XContentType.values());
         final boolean humanReadable = randomBoolean();
 

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java
@@ -89,6 +89,22 @@ public void testIncludes() throws Exception {
         assertThat(sourceAsMap.containsKey("path2"), equalTo(false));
     }
 
+    public void testDuplicatedIncludes() throws Exception {
+        DocumentMapper documentMapper = createDocumentMapper(
+            topMapping(b -> b.startObject("_source").array("includes", "path1", "path1").endObject())
+        );
+
+        ParsedDocument doc = documentMapper.parse(source(b -> {
+            b.startObject("path1").field("field1", "value1").endObject();
+            b.startObject("path2").field("field2", "value2").endObject();
+        }));
+
+        IndexableField sourceField = doc.rootDoc().getField("_source");
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, new BytesArray(sourceField.binaryValue()))) {
+            assertEquals(Map.of("path1", Map.of("field1", "value1")), parser.map());
+        }
+    }
+
     public void testExcludes() throws Exception {
         DocumentMapper documentMapper = createDocumentMapper(
             topMapping(b -> b.startObject("_source").array("excludes", "path1*").endObject())
@@ -108,6 +124,22 @@ public void testExcludes() throws Exception {
         assertThat(sourceAsMap.containsKey("path2"), equalTo(true));
     }
 
+    public void testDuplicatedExcludes() throws Exception {
+        DocumentMapper documentMapper = createDocumentMapper(
+            topMapping(b -> b.startObject("_source").array("excludes", "path1", "path1").endObject())
+        );
+
+        ParsedDocument doc = documentMapper.parse(source(b -> {
+            b.startObject("path1").field("field1", "value1").endObject();
+            b.startObject("path2").field("field2", "value2").endObject();
+        }));
+
+        IndexableField sourceField = doc.rootDoc().getField("_source");
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, new BytesArray(sourceField.binaryValue()))) {
+            assertEquals(Map.of("path2", Map.of("field2", "value2")), parser.map());
+        }
+    }
+
     public void testComplete() throws Exception {
 
         assertTrue(createDocumentMapper(topMapping(b -> {})).sourceMapper().isComplete());

diff --git a/...rk/src/main/java/org/elasticsearch/common/xcontent/support/AbstractFilteringTestCase.java b/...rk/src/main/java/org/elasticsearch/common/xcontent/support/AbstractFilteringTestCase.java
@@ -26,6 +26,7 @@
 import java.io.InputStreamReader;
 import java.net.URISyntaxException;
 import java.nio.charset.StandardCharsets;
+import java.util.Collection;
 import java.util.Set;
 
 import static java.util.Collections.emptySet;
@@ -41,7 +42,8 @@ public abstract class AbstractFilteringTestCase extends ESTestCase {
     @FunctionalInterface
     protected interface Builder extends CheckedFunction<XContentBuilder, XContentBuilder, IOException> {}
 
-    protected abstract void testFilter(Builder expected, Builder actual, Set<String> includes, Set<String> excludes) throws IOException;
+    protected abstract void testFilter(Builder expected, Builder actual, Collection<String> includes, Collection<String> excludes)
+        throws IOException;
 
     /** Sample test case */
     protected static final Builder SAMPLE = builderFor("sample.json");