diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContent.java index c73f5f19d25..72210f09d9b 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/XContent.java +++ b/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 @@ import java.util.Set; */ 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. */ diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java index 4224b5328a7..d79173cfc2b 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContent.java @@ -21,6 +21,7 @@ package org.elasticsearch.common.xcontent.cbor; 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 class CborXContent implements XContent { 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(); } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java index 4657a554099..1b0b351e6ef 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContent.java @@ -23,7 +23,6 @@ import com.fasterxml.jackson.core.JsonEncoding; 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,27 +49,6 @@ public class JsonXContent implements XContent { 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); @@ -78,7 +56,7 @@ public class JsonXContent implements XContent { 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(); } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java index 94ac9b94356..643326cd82f 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContent.java @@ -21,6 +21,7 @@ package org.elasticsearch.common.xcontent.smile; 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 class SmileXContent implements XContent { 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(); } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContent.java index 54da03118d7..7413f05f583 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContent.java +++ b/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 class YamlXContent implements XContent { static { yamlFactory = new YAMLFactory(); + yamlFactory.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, XContent.isStrictDuplicateDetectionEnabled()); yamlXContent = new YamlXContent(); } diff --git a/core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java b/core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java index 70439ef8bf9..d917c0d12c0 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java @@ -22,7 +22,7 @@ package org.elasticsearch.common.settings.loader; 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 class JsonSettingsLoaderTests extends ESTestCase { } 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); diff --git a/core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java b/core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java index 7c956de8f9a..eeb2df7e1d6 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java @@ -22,6 +22,7 @@ package org.elasticsearch.common.settings.loader; 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 class YamlSettingsLoaderTests extends ESTestCase { } 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); diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java index 57e9e0d5216..1c0a92dbd74 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java @@ -22,6 +22,7 @@ package org.elasticsearch.common.xcontent; 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.containsString; 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 abstract class BaseXContentTestCase extends ESTestCase { 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")); diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java b/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java index 387efdb5e50..6cae59391c5 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java @@ -169,8 +169,8 @@ public class ConstructingObjectParserTests extends ESTestCase { } 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" diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/json/JsonXContentTests.java b/core/src/test/java/org/elasticsearch/common/xcontent/json/JsonXContentTests.java index b4549830432..4a79ddb4ec6 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/json/JsonXContentTests.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/json/JsonXContentTests.java @@ -22,7 +22,6 @@ package org.elasticsearch.common.xcontent.json; 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 class JsonXContentTests extends BaseXContentTestCase { 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()); - } } diff --git a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java index 8e04ec7f553..13854ffc1e2 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java @@ -26,6 +26,7 @@ import org.apache.lucene.search.MatchAllDocsQuery; 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 class BoolQueryBuilderTests extends AbstractQueryTestCase