diff --git a/core/src/main/java/org/elasticsearch/common/settings/loader/JsonSettingsLoader.java b/core/src/main/java/org/elasticsearch/common/settings/loader/JsonSettingsLoader.java index f6f77192c75..02f7a5c37a0 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/loader/JsonSettingsLoader.java +++ b/core/src/main/java/org/elasticsearch/common/settings/loader/JsonSettingsLoader.java @@ -27,6 +27,10 @@ import org.elasticsearch.common.xcontent.XContentType; */ public class JsonSettingsLoader extends XContentSettingsLoader { + public JsonSettingsLoader(boolean allowNullValues) { + super(allowNullValues); + } + @Override public XContentType contentType() { return XContentType.JSON; diff --git a/core/src/main/java/org/elasticsearch/common/settings/loader/SettingsLoaderFactory.java b/core/src/main/java/org/elasticsearch/common/settings/loader/SettingsLoaderFactory.java index e55cb1092f2..5bf9916ee0e 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/loader/SettingsLoaderFactory.java +++ b/core/src/main/java/org/elasticsearch/common/settings/loader/SettingsLoaderFactory.java @@ -20,43 +20,63 @@ package org.elasticsearch.common.settings.loader; /** - * A settings loader factory automatically trying to identify what type of - * {@link SettingsLoader} to use. - * - * + * A class holding factory methods for settings loaders that attempts + * to infer the type of the underlying settings content. */ public final class SettingsLoaderFactory { private SettingsLoaderFactory() { - } /** - * Returns a {@link SettingsLoader} based on the resource name. + * Returns a {@link SettingsLoader} based on the source resource + * name. This factory method assumes that if the resource name ends + * with ".json" then the content should be parsed as JSON, else if + * the resource name ends with ".yml" or ".yaml" then the content + * should be parsed as YAML, else if the resource name ends with + * ".properties" then the content should be parsed as properties, + * otherwise default to attempting to parse as JSON. Note that the + * parsers returned by this method will not accept null-valued + * keys. + * + * @param resourceName The resource name containing the settings + * content. + * @return A settings loader. */ public static SettingsLoader loaderFromResource(String resourceName) { if (resourceName.endsWith(".json")) { - return new JsonSettingsLoader(); + return new JsonSettingsLoader(false); } else if (resourceName.endsWith(".yml") || resourceName.endsWith(".yaml")) { - return new YamlSettingsLoader(); + return new YamlSettingsLoader(false); } else if (resourceName.endsWith(".properties")) { return new PropertiesSettingsLoader(); } else { // lets default to the json one - return new JsonSettingsLoader(); + return new JsonSettingsLoader(false); } } /** - * Returns a {@link SettingsLoader} based on the actual settings source. + * Returns a {@link SettingsLoader} based on the source content. + * This factory method assumes that if the underlying content + * contains an opening and closing brace ('{' and '}') then the + * content should be parsed as JSON, else if the underlying content + * fails this condition but contains a ':' then the content should + * be parsed as YAML, and otherwise should be parsed as properties. + * Note that the JSON and YAML parsers returned by this method will + * accept null-valued keys. + * + * @param source The underlying settings content. + * @return A settings loader. */ public static SettingsLoader loaderFromSource(String source) { if (source.indexOf('{') != -1 && source.indexOf('}') != -1) { - return new JsonSettingsLoader(); + return new JsonSettingsLoader(true); } if (source.indexOf(':') != -1) { - return new YamlSettingsLoader(); + return new YamlSettingsLoader(true); } return new PropertiesSettingsLoader(); } + } diff --git a/core/src/main/java/org/elasticsearch/common/settings/loader/XContentSettingsLoader.java b/core/src/main/java/org/elasticsearch/common/settings/loader/XContentSettingsLoader.java index 9c2f973b96e..3875c1ef85a 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/loader/XContentSettingsLoader.java +++ b/core/src/main/java/org/elasticsearch/common/settings/loader/XContentSettingsLoader.java @@ -38,6 +38,12 @@ public abstract class XContentSettingsLoader implements SettingsLoader { public abstract XContentType contentType(); + private final boolean allowNullValues; + + XContentSettingsLoader(boolean allowNullValues) { + this.allowNullValues = allowNullValues; + } + @Override public Map load(String source) throws IOException { try (XContentParser parser = XContentFactory.xContent(contentType()).createParser(source)) { @@ -153,6 +159,16 @@ public abstract class XContentSettingsLoader implements SettingsLoader { currentValue ); } + + if (currentValue == null && !allowNullValues) { + throw new ElasticsearchParseException( + "null-valued setting found for key [{}] found at line number [{}], column number [{}]", + key, + parser.getTokenLocation().lineNumber, + parser.getTokenLocation().columnNumber + ); + } + settings.put(key, currentValue); } } diff --git a/core/src/main/java/org/elasticsearch/common/settings/loader/YamlSettingsLoader.java b/core/src/main/java/org/elasticsearch/common/settings/loader/YamlSettingsLoader.java index 248fe090b5d..12cde976691 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/loader/YamlSettingsLoader.java +++ b/core/src/main/java/org/elasticsearch/common/settings/loader/YamlSettingsLoader.java @@ -30,6 +30,10 @@ import java.util.Map; */ public class YamlSettingsLoader extends XContentSettingsLoader { + public YamlSettingsLoader(boolean allowNullValues) { + super(allowNullValues); + } + @Override public XContentType contentType() { return XContentType.YAML; diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index e3f2bc1bb2b..eb6cc56816f 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -201,8 +201,8 @@ public class SettingsTests extends ESTestCase { assertThat(settings.getAsArray("value"), arrayContaining("2", "3")); settings = settingsBuilder() - .put(new YamlSettingsLoader().load("value: 1")) - .put(new YamlSettingsLoader().load("value: [ 2, 3 ]")) + .put(new YamlSettingsLoader(false).load("value: 1")) + .put(new YamlSettingsLoader(false).load("value: [ 2, 3 ]")) .build(); assertThat(settings.getAsArray("value"), arrayContaining("2", "3")); 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 d7f10891f28..9750205145e 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 @@ -25,6 +25,7 @@ import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.test.ESTestCase; import static org.elasticsearch.common.settings.Settings.settingsBuilder; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.equalTo; /** @@ -61,4 +62,10 @@ public class JsonSettingsLoaderTests extends ESTestCase { assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [1], column number [20], previous value [bar], current value [baz]")); } } + + public void testNullValuedSettingThrowsException() { + final String json = "{\"foo\":null}"; + final ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> new JsonSettingsLoader(false).load(json)); + assertThat(e.toString(), containsString("null-valued setting found for key [foo] found at line number [1], column number [8]")); + } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/loader/PropertiesSettingsLoaderTests.java b/core/src/test/java/org/elasticsearch/common/settings/loader/PropertiesSettingsLoaderTests.java index 7a1897fbaf9..3a438945802 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/loader/PropertiesSettingsLoaderTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/loader/PropertiesSettingsLoaderTests.java @@ -44,4 +44,10 @@ public class PropertiesSettingsLoaderTests extends ESTestCase { assertEquals(e.getMessage(), "duplicate settings key [foo] found, previous value [bar], current value [baz]"); } } + + public void testThatNoDuplicatesPropertiesDoesNotAcceptNullValues() { + final PropertiesSettingsLoader loader = new PropertiesSettingsLoader(); + final PropertiesSettingsLoader.NoDuplicatesProperties properties = loader.new NoDuplicatesProperties(); + expectThrows(NullPointerException.class, () -> properties.put("key", null)); + } } 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 48703044ecd..a4536ec21b9 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 @@ -86,4 +86,10 @@ public class YamlSettingsLoaderTests extends ESTestCase { assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [2], column number [6], previous value [bar], current value [baz]")); } } + + public void testNullValuedSettingThrowsException() { + final String yaml = "foo:"; + final ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> new YamlSettingsLoader(false).load(yaml)); + assertThat(e.toString(), containsString("null-valued setting found for key [foo] found at line number [1], column number [5]")); + } }