Remove hand-coded XContent duplicate checks

With this commit we cleanup hand-coded duplicate checks in XContent
parsing. They were necessary previously but since we reconfigured the
underlying parser in #22073 and #22225, these checks are obsolete and
were also ineffective unless an undocumented system property has been
set. As we also remove this escape hatch, we can remove the additional
checks as well.

Closes #22253
Relates #34588

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java

@@ -216,7 +216,7 @@ public <T> void declareField(BiConsumer<Value, T> consumer, ContextParser<Contex
             int position = constructorArgInfos.size();
             boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
             constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
-            objectParser.declareField((target, v) -> target.constructorArg(position, parseField, v), parser, parseField, type);
+            objectParser.declareField((target, v) -> target.constructorArg(position, v), parser, parseField, type);
         } else {
             numberOfFields += 1;
             objectParser.declareField(queueingConsumer(consumer, parseField), parser, parseField, type);
@@ -249,7 +249,7 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
             int position = constructorArgInfos.size();
             boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
             constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
-            objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, parseField, v), namedObjectParser, parseField);
+            objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, v), namedObjectParser, parseField);
         } else {
             numberOfFields += 1;
             objectParser.declareNamedObjects(queueingConsumer(consumer, parseField), namedObjectParser, parseField);
@@ -285,7 +285,7 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
             int position = constructorArgInfos.size();
             boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
             constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
-            objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, parseField, v), namedObjectParser,
+            objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, v), namedObjectParser,
                     wrapOrderedModeCallBack(orderedModeCallback), parseField);
         } else {
             numberOfFields += 1;
@@ -395,10 +395,7 @@ private class Target {
         /**
          * Set a constructor argument and build the target object if all constructor arguments have arrived.
          */
-        private void constructorArg(int position, ParseField parseField, Object value) {
-            if (constructorArgs[position] != null) {
-                throw new IllegalArgumentException("Can't repeat param [" + parseField + "]");
-            }
+        private void constructorArg(int position, Object value) {
             constructorArgs[position] = value;
             constructorArgsCollected++;
             if (constructorArgsCollected == constructorArgInfos.size()) {

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContent.java

@@ -19,8 +19,6 @@
 
 package org.elasticsearch.common.xcontent;
 
-import org.elasticsearch.common.Booleans;
-
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -32,28 +30,6 @@
  * A generic abstraction on top of handling content, inspired by JSON and pull parsing.
  */
 public interface XContent {
-
-    /*
-     * NOTE: This comment is only meant for maintainers of the Elasticsearch code base and is intentionally not a Javadoc comment as it
-     *       describes an undocumented system property.
-     *
-     *
-     * Determines whether the XContent parser will always check for duplicate keys. This behavior is enabled by default but
-     * can be disabled by setting the otherwise undocumented system property "es.xcontent.strict_duplicate_detection to "false".
-     *
-     * Before we've enabled this mode, we had custom duplicate checks in various parts of the code base. As the user can still disable this
-     * mode and fall back to the legacy duplicate checks, we still need to keep the custom duplicate checks around and we also need to keep
-     * the tests around.
-     *
-     * If this fallback via system property is removed one day in the future you can remove all tests that call this method and also remove
-     * the corresponding custom duplicate check code.
-     *
-     */
-    static boolean isStrictDuplicateDetectionEnabled() {
-        // Don't allow duplicate keys in JSON content by default but let the user opt out
-        return Booleans.parseBoolean(System.getProperty("es.xcontent.strict_duplicate_detection", "true"), true);
-    }
-
     /**
      * The type this content handles and produces.
      */

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java

@@ -56,7 +56,7 @@ public static XContentBuilder contentBuilder() throws IOException {
         cborFactory.configure(CBORFactory.Feature.FAIL_ON_SYMBOL_HASH_OVERFLOW, false); // this trips on many mappings now...
         // Do not automatically close unclosed objects/arrays in com.fasterxml.jackson.dataformat.cbor.CBORGenerator#close() method
         cborFactory.configure(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT, false);
-        cborFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
+        cborFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, true);
         cborXContent = new CborXContent();
     }
 

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java

@@ -57,7 +57,7 @@ public static XContentBuilder contentBuilder() throws IOException {
         jsonFactory.configure(JsonFactory.Feature.FAIL_ON_SYMBOL_HASH_OVERFLOW, false); // this trips on many mappings now...
         // Do not automatically close unclosed objects/arrays in com.fasterxml.jackson.core.json.UTF8JsonGenerator#close() method
         jsonFactory.configure(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT, false);
-        jsonFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
+        jsonFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, true);
         jsonXContent = new JsonXContent();
     }
 

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java

@@ -58,7 +58,7 @@ public static XContentBuilder contentBuilder() throws IOException {
         smileFactory.configure(SmileFactory.Feature.FAIL_ON_SYMBOL_HASH_OVERFLOW, false); // this trips on many mappings now...
         // Do not automatically close unclosed objects/arrays in com.fasterxml.jackson.dataformat.smile.SmileGenerator#close() method
         smileFactory.configure(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT, false);
-        smileFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
+        smileFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, true);
         smileXContent = new SmileXContent();
     }
 

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContent.java

@@ -51,7 +51,7 @@ public static XContentBuilder contentBuilder() throws IOException {
 
     static {
         yamlFactory = new YAMLFactory();
-        yamlFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
+        yamlFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, true);
         yamlXContent = new YamlXContent();
     }
 

libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java

@@ -39,7 +39,6 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
-import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.nullValue;
 
 public class ConstructingObjectParserTests extends ESTestCase {
@@ -164,21 +163,6 @@ public void testMissingFirstConstructorArgButNotRequired() throws IOException {
         assertEquals((Integer) 2, parsed.vegetable);
     }
 
-    public void testRepeatedConstructorParam() throws IOException {
-        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-        XContentParser parser = createParser(JsonXContent.jsonXContent,
-                  "{\n"
-                + "  \"vegetable\": 1,\n"
-                + "  \"vegetable\": 2\n"
-                + "}");
-        Throwable e = expectThrows(XContentParseException.class, () -> randomFrom(HasCtorArguments.ALL_PARSERS).apply(parser, null));
-        assertEquals("[has_required_arguments] failed to parse field [vegetable]", e.getMessage());
-        e = e.getCause();
-        assertThat(e, instanceOf(IllegalArgumentException.class));
-        assertEquals("Can't repeat param [vegetable]", e.getMessage());
-    }
-
     public void testBadParam() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent,
                   "{\n"

server/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -712,41 +712,29 @@ private static void fromXContent(XContentParser parser, StringBuilder keyBuilder
                     }
                 }
                 String key = keyBuilder.toString();
-                validateValue(key, list, builder, parser, allowNullValues);
+                validateValue(key, list, parser, allowNullValues);
                 builder.putList(key, list);
             } else if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
                 String key = keyBuilder.toString();
-                validateValue(key, null, builder, parser, allowNullValues);
+                validateValue(key, null, parser, allowNullValues);
                 builder.putNull(key);
             } else if (parser.currentToken() == XContentParser.Token.VALUE_STRING
                 || parser.currentToken() == XContentParser.Token.VALUE_NUMBER) {
                 String key = keyBuilder.toString();
                 String value = parser.text();
-                validateValue(key, value, builder, parser, allowNullValues);
+                validateValue(key, value, parser, allowNullValues);
                 builder.put(key, value);
             } else if (parser.currentToken() == XContentParser.Token.VALUE_BOOLEAN) {
                 String key = keyBuilder.toString();
-                validateValue(key, parser.text(), builder, parser, allowNullValues);
+                validateValue(key, parser.text(), parser, allowNullValues);
                 builder.put(key, parser.booleanValue());
             } else {
                 XContentParserUtils.throwUnknownToken(parser.currentToken(), parser.getTokenLocation());
             }
         }
     }
 
-    private static void validateValue(String key, Object currentValue, Settings.Builder builder, XContentParser parser,
-                                      boolean allowNullValues) {
-        if (builder.map.containsKey(key)) {
-            throw new ElasticsearchParseException(
-                "duplicate settings key [{}] found at line number [{}], column number [{}], previous value [{}], current value [{}]",
-                key,
-                parser.getTokenLocation().lineNumber,
-                parser.getTokenLocation().columnNumber,
-                builder.map.get(key),
-                currentValue
-            );
-        }
-
+    private static void validateValue(String key, Object currentValue, XContentParser parser, boolean allowNullValues) {
         if (currentValue == null && allowNullValues == false) {
             throw new ElasticsearchParseException(
                 "null-valued setting found for key [{}] found at line number [{}], column number [{}]",

server/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java

@@ -98,10 +98,6 @@ public static ConstantScoreQueryBuilder fromXContent(XContentParser parser) thro
                 currentFieldName = parser.currentName();
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if (INNER_QUERY_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                    if (queryFound) {
-                        throw new ParsingException(parser.getTokenLocation(), "[" + ConstantScoreQueryBuilder.NAME + "]"
-                                + " accepts only one 'filter' element.");
-                    }
                     query = parseInnerQueryBuilder(parser);
                     queryFound = true;
                 } else {

server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -26,13 +26,11 @@
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.xcontent.ToXContent;
-import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.VersionUtils;
-import org.hamcrest.CoreMatchers;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -553,32 +551,6 @@ public void testSimpleJsonSettings() throws Exception {
         assertThat(settings.getAsList("test1.test3").get(1), equalTo("test3-2"));
     }
 
-    public void testDuplicateKeysThrowsException() {
-        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-        final String json = "{\"foo\":\"bar\",\"foo\":\"baz\"}";
-        final SettingsException e = expectThrows(SettingsException.class,
-            () -> Settings.builder().loadFromSource(json, XContentType.JSON).build());
-        assertThat(
-            e.toString(),
-            CoreMatchers.containsString("duplicate settings key [foo] " +
-                "found at line number [1], " +
-                "column number [20], " +
-                "previous value [bar], " +
-                "current value [baz]"));
-
-        String yaml = "foo: bar\nfoo: baz";
-        SettingsException e1 = expectThrows(SettingsException.class, () -> {
-            Settings.builder().loadFromSource(yaml, XContentType.YAML);
-        });
-        assertEquals(e1.getCause().getClass(), ElasticsearchParseException.class);
-        String msg = e1.getCause().getMessage();
-        assertTrue(
-            msg,
-            msg.contains("duplicate settings key [foo] found at line number [2], column number [6], " +
-                "previous value [bar], current value [baz]"));
-    }
-
     public void testToXContent() throws IOException {
         // this is just terrible but it's the existing behavior!
         Settings test = Settings.builder().putList("foo.bar", "1", "2", "3").put("foo.bar.baz", "test").build();

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

@@ -1138,9 +1138,6 @@ public void testSelfReferencingIterableTwoLevels() throws IOException {
     }
 
     public void testChecksForDuplicates() throws Exception {
-        assumeTrue("Test only makes sense if XContent parser has strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-
         XContentBuilder builder = builder()
                 .startObject()
                     .field("key", 1)

server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java

@@ -25,7 +25,6 @@
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.ParsingException;
-import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -332,30 +331,6 @@ public void testUnknownQueryName() throws IOException {
         assertEquals("no [query] registered for [unknown_query]", ex.getMessage());
     }
 
-    /**
-     * test that two queries in object throws error
-     */
-    public void testTooManyQueriesInObject() throws IOException {
-        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-        String clauseType = randomFrom("must", "should", "must_not", "filter");
-        // should also throw error if invalid query is preceded by a valid one
-        String query = "{\n" +
-                "  \"bool\": {\n" +
-                "    \"" + clauseType + "\": {\n" +
-                "      \"match\": {\n" +
-                "        \"foo\": \"bar\"\n" +
-                "      },\n" +
-                "      \"match\": {\n" +
-                "        \"baz\": \"buzz\"\n" +
-                "      }\n" +
-                "    }\n" +
-                "  }\n" +
-                "}";
-        ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(query));
-        assertEquals("[match] malformed query, expected [END_OBJECT] but found [FIELD_NAME]", ex.getMessage());
-    }
-
     public void testRewrite() throws IOException {
         BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder();
         boolean mustRewrite = false;

server/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java

@@ -22,7 +22,6 @@
 import org.apache.lucene.search.ConstantScoreQuery;
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.ParsingException;
-import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.search.internal.SearchContext;
 import org.elasticsearch.test.AbstractQueryTestCase;
 
@@ -62,20 +61,6 @@ public void testFilterElement() throws IOException {
         assertThat(e.getMessage(), containsString("requires a 'filter' element"));
     }
 
-    /**
-     * test that multiple "filter" elements causes {@link ParsingException}
-     */
-    public void testMultipleFilterElements() throws IOException {
-        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-        String queryString = "{ \"" + ConstantScoreQueryBuilder.NAME + "\" : {\n" +
-                                    "\"filter\" : { \"term\": { \"foo\": \"a\" } },\n" +
-                                    "\"filter\" : { \"term\": { \"foo\": \"x\" } },\n" +
-                            "} }";
-        ParsingException e = expectThrows(ParsingException.class, () -> parseQuery(queryString));
-        assertThat(e.getMessage(), containsString("accepts only one 'filter' element"));
-    }
-
     /**
      * test that "filter" does not accept an array of queries, throws {@link ParsingException}
      */

server/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java

@@ -35,7 +35,6 @@
 import org.elasticsearch.common.lucene.search.function.FunctionScoreQuery;
 import org.elasticsearch.common.lucene.search.function.WeightFactorFunction;
 import org.elasticsearch.common.unit.DistanceUnit;
-import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
@@ -726,27 +725,6 @@ public void testMalformedQueryMultipleQueryObjects() throws IOException {
         expectParsingException(json, equalTo("[bool] malformed query, expected [END_OBJECT] but found [FIELD_NAME]"));
     }
 
-    public void testMalformedQueryMultipleQueryElements() throws IOException {
-        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
-            XContent.isStrictDuplicateDetectionEnabled());
-        String json = "{\n" +
-                "    \"function_score\":{\n" +
-                "        \"query\":{\n" +
-                "            \"bool\":{\n" +
-                "                \"must\":{\"match\":{\"field\":\"value\"}}" +
-                "             }\n" +
-                "            },\n" +
-                "        \"query\":{\n" +
-                "            \"bool\":{\n" +
-                "                \"must\":{\"match\":{\"field\":\"value\"}}" +
-                "             }\n" +
-                "            }\n" +
-                "        }\n" +
-                "    }\n" +
-                "}";
-        expectParsingException(json, "[query] is already defined.");
-    }
-
     private void expectParsingException(String json, Matcher<String> messageMatcher) {
         ParsingException e = expectThrows(ParsingException.class, () -> parseQuery(json));
         assertThat(e.getMessage(), messageMatcher);