Enable strict duplicate checks for all XContent types

With this commit we enable the Jackson feature 'STRICT_DUPLICATE_DETECTION'
by default for all XContent types (not only JSON).

We have also changed the name of the system property to disable this feature
from `es.json.strict_duplicate_detection` to the now more appropriate name
`es.xcontent.strict_duplicate_detection`.

Relates elastic#19614
Relates elastic#22073

Author: danielmitterdorfer

core/src/main/java/org/elasticsearch/common/xcontent/XContent.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.common.xcontent;
 
+import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.bytes.BytesReference;
 
 import java.io.IOException;
@@ -33,6 +34,27 @@
  */
 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.parseBooleanExact(System.getProperty("es.xcontent.strict_duplicate_detection", "true"));
+    }
+
     /**
      * The type this content handles and produces.
      */

core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java

@@ -21,6 +21,7 @@
 
 import com.fasterxml.jackson.core.JsonEncoding;
 import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.dataformat.cbor.CBORFactory;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.bytes.BytesReference;
@@ -54,6 +55,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());
         cborXContent = new CborXContent();
     }
 

core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java

@@ -23,7 +23,6 @@
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.JsonParser;
-import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.FastStringReader;
 import org.elasticsearch.common.xcontent.XContent;
@@ -50,35 +49,14 @@ public static XContentBuilder contentBuilder() throws IOException {
 
     public static final JsonXContent jsonXContent;
 
-    /*
-     * 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 JSON parser will always check for duplicate keys in JSON content. This behavior is enabled by default but
-     * can be disabled by setting the otherwise undocumented system property "es.json.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.
-     *
-     */
-    public static boolean isStrictDuplicateDetectionEnabled() {
-        // Don't allow duplicate keys in JSON content by default but let the user opt out
-        return Booleans.parseBooleanExact(System.getProperty("es.json.strict_duplicate_detection", "true"));
-    }
-
     static {
         jsonFactory = new JsonFactory();
         jsonFactory.configure(JsonGenerator.Feature.QUOTE_FIELD_NAMES, true);
         jsonFactory.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
         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, isStrictDuplicateDetectionEnabled());
+        jsonFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
         jsonXContent = new JsonXContent();
     }
 

core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java

@@ -21,6 +21,7 @@
 
 import com.fasterxml.jackson.core.JsonEncoding;
 import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.dataformat.smile.SmileFactory;
 import com.fasterxml.jackson.dataformat.smile.SmileGenerator;
 import org.elasticsearch.common.bytes.BytesReference;
@@ -55,6 +56,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());
         smileXContent = new SmileXContent();
     }
 

core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContent.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.common.xcontent.yaml;
 
 import com.fasterxml.jackson.core.JsonEncoding;
+import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.bytes.BytesReference;
@@ -50,6 +51,7 @@ public static XContentBuilder contentBuilder() throws IOException {
 
     static {
         yamlFactory = new YAMLFactory();
+        yamlFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled());
         yamlXContent = new YamlXContent();
     }
 

core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java

@@ -22,7 +22,7 @@
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.settings.SettingsException;
-import org.elasticsearch.common.xcontent.json.JsonXContent;
+import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.test.ESTestCase;
 
 import static org.hamcrest.CoreMatchers.containsString;
@@ -49,8 +49,8 @@ public void testSimpleJsonSettings() throws Exception {
     }
 
     public void testDuplicateKeysThrowsException() {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        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).build());
         assertEquals(e.getCause().getClass(), ElasticsearchParseException.class);

core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.settings.SettingsException;
+import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.test.ESTestCase;
 
 import java.nio.charset.StandardCharsets;
@@ -68,6 +69,9 @@ public void testIndentationWithExplicitDocumentStart() throws Exception {
     }
 
     public void testDuplicateKeysThrowsException() {
+        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
+            XContent.isStrictDuplicateDetectionEnabled());
+
         String yaml = "foo: bar\nfoo: baz";
         SettingsException e = expectThrows(SettingsException.class, () -> {
             Settings.builder().loadFromSource(yaml);

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

@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.core.JsonGenerationException;
 import com.fasterxml.jackson.core.JsonGenerator;
 
+import com.fasterxml.jackson.core.JsonParseException;
 import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.Constants;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
@@ -66,6 +67,7 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.startsWith;
 
 public abstract class BaseXContentTestCase extends ESTestCase {
 
@@ -984,6 +986,22 @@ public void testSelfReferencingIterableTwoLevels() throws IOException {
         assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself"));
     }
 
+    public void testChecksForDuplicates() throws Exception {
+        assumeTrue("Test only makes sense if XContent parser has strict duplicate checks enabled",
+            XContent.isStrictDuplicateDetectionEnabled());
+
+        BytesReference bytes = builder()
+                .startObject()
+                    .field("key", 1)
+                    .field("key", 2)
+                .endObject()
+            .bytes();
+
+        JsonParseException pex = expectThrows(JsonParseException.class, () -> createParser(xcontentType().xContent(), bytes).map());
+        assertThat(pex.getMessage(), startsWith("Duplicate field 'key'"));
+    }
+
+
     private static void expectUnclosedException(ThrowingRunnable runnable) {
         IllegalStateException e = expectThrows(IllegalStateException.class, runnable);
         assertThat(e.getMessage(), containsString("Failed to close the XContentBuilder"));

core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java

@@ -169,8 +169,8 @@ public void testMissingFirstConstructorArgButNotRequired() throws IOException {
     }
 
     public void testRepeatedConstructorParam() throws IOException {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        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"

core/src/test/java/org/elasticsearch/common/xcontent/json/JsonXContentTests.java

@@ -22,7 +22,6 @@
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonGenerator;
 
-import com.fasterxml.jackson.core.JsonParseException;
 import org.elasticsearch.common.xcontent.BaseXContentTestCase;
 import org.elasticsearch.common.xcontent.XContentType;
 
@@ -40,13 +39,4 @@ public void testBigInteger() throws Exception {
         JsonGenerator generator = new JsonFactory().createGenerator(os);
         doTestBigInteger(generator, os);
     }
-
-    public void testChecksForDuplicates() throws Exception {
-        assumeTrue("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
-
-        JsonParseException pex = expectThrows(JsonParseException.class,
-            () -> XContentType.JSON.xContent().createParser("{ \"key\": 1, \"key\": 2 }").map());
-        assertEquals("Duplicate field 'key'", pex.getMessage());
-    }
 }

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

@@ -26,6 +26,7 @@
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.ParseFieldMatcher;
 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.XContentType;
@@ -340,8 +341,8 @@ public void testUnknownQueryName() throws IOException {
      * test that two queries in object throws error
      */
     public void testTooManyQueriesInObject() throws IOException {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        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" +

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

@@ -22,6 +22,7 @@
 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.common.xcontent.json.JsonXContent;
 import org.elasticsearch.search.internal.SearchContext;
 import org.elasticsearch.test.AbstractQueryTestCase;
@@ -66,8 +67,8 @@ public void testFilterElement() throws IOException {
      * test that multiple "filter" elements causes {@link ParsingException}
      */
     public void testMultipleFilterElements() throws IOException {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        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" +

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

@@ -36,6 +36,7 @@
 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.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -739,8 +740,8 @@ public void testMalformedQueryMultipleQueryObjects() throws IOException {
     }
 
     public void testMalformedQueryMultipleQueryElements() throws IOException {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        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" +

core/src/test/java/org/elasticsearch/search/aggregations/AggregatorParsingTests.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.common.ParseFieldMatcher;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -104,8 +105,8 @@ public void testTwoTypes() throws Exception {
     }
 
     public void testTwoAggs() throws Exception {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
+            XContent.isStrictDuplicateDetectionEnabled());
         XContentBuilder source = JsonXContent.contentBuilder()
                 .startObject()
                     .startObject("by_date")
@@ -180,8 +181,8 @@ public void testInvalidAggregationName() throws Exception {
     }
 
     public void testSameAggregationName() throws Exception {
-        assumeFalse("Test only makes sense if JSON parser doesn't have strict duplicate checks enabled",
-            JsonXContent.isStrictDuplicateDetectionEnabled());
+        assumeFalse("Test only makes sense if XContent parser doesn't have strict duplicate checks enabled",
+            XContent.isStrictDuplicateDetectionEnabled());
         final String name = randomAsciiOfLengthBetween(1, 10);
         XContentBuilder source = JsonXContent.contentBuilder()
                 .startObject()

core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java

@@ -23,6 +23,7 @@
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -149,7 +150,7 @@ public void testIllegalXContent() throws IOException {
                 "Required [field]");
 
         // test two fieldnames
-        if (JsonXContent.isStrictDuplicateDetectionEnabled()) {
+        if (XContent.isStrictDuplicateDetectionEnabled()) {
             logger.info("Skipping test as it uses a custom duplicate check that is obsolete when strict duplicate checks are enabled.");
         } else {
             directGenerator = "{ \"field\" : \"f1\", \"field\" : \"f2\" }";

docs/reference/migration/migrate_6_0/rest.asciidoc

@@ -8,10 +8,10 @@ This feature was removed in the 5.x series, but a backwards-compatibility layer
 system property `elasticsearch.json.allow_unquoted_field_names`. This backwards-compatibility layer
 has been removed in Elasticsearch 6.0.0.
 
-==== Duplicate Keys in JSON
+==== Duplicate Keys in Content
 
-In previous versions of Elasticsearch, JSON documents were allowed to contain duplicate keys. Elasticsearch 6.0.0
- enforces that all keys are unique.
+In previous versions of Elasticsearch, documents were allowed to contain duplicate keys. Elasticsearch 6.0.0
+ enforces that all keys are unique. This applies to all content types: JSON, CBOR, Yaml and Smile.
 
 ==== Analyze API changes
 

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/parser/AbstractClientYamlTestFragmentParserTestCase.java

@@ -36,8 +36,11 @@ public abstract class AbstractClientYamlTestFragmentParserTestCase extends ESTes
     @After
     public void tearDown() throws Exception {
         super.tearDown();
-        //this is the way to make sure that we consumed the whole yaml
-        assertThat(parser.currentToken(), nullValue());
-        parser.close();
+        // test may be skipped so we did not create a parser instance
+        if (parser != null) {
+            //this is the way to make sure that we consumed the whole yaml
+            assertThat(parser.currentToken(), nullValue());
+            parser.close();
+        }
     }
 }