Merge pull request #13086 from jasontedor/fix/no-duplicate-settings

Detect duplicate settings keys on startup
This commit is contained in:
Jason Tedor 2015-08-24 20:51:22 -04:00
commit 51ae1c4d53
5 changed files with 104 additions and 9 deletions

View File

@ -20,6 +20,7 @@
package org.elasticsearch.common.settings.loader; package org.elasticsearch.common.settings.loader;
import org.apache.lucene.util.IOUtils; import org.apache.lucene.util.IOUtils;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.io.FastStringReader; import org.elasticsearch.common.io.FastStringReader;
import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamInput;
@ -36,7 +37,7 @@ public class PropertiesSettingsLoader implements SettingsLoader {
@Override @Override
public Map<String, String> load(String source) throws IOException { public Map<String, String> load(String source) throws IOException {
Properties props = new Properties(); Properties props = new NoDuplicatesProperties();
FastStringReader reader = new FastStringReader(source); FastStringReader reader = new FastStringReader(source);
try { try {
props.load(reader); props.load(reader);
@ -52,7 +53,7 @@ public class PropertiesSettingsLoader implements SettingsLoader {
@Override @Override
public Map<String, String> load(byte[] source) throws IOException { public Map<String, String> load(byte[] source) throws IOException {
Properties props = new Properties(); Properties props = new NoDuplicatesProperties();
StreamInput stream = StreamInput.wrap(source); StreamInput stream = StreamInput.wrap(source);
try { try {
props.load(stream); props.load(stream);
@ -65,4 +66,15 @@ public class PropertiesSettingsLoader implements SettingsLoader {
IOUtils.closeWhileHandlingException(stream); IOUtils.closeWhileHandlingException(stream);
} }
} }
class NoDuplicatesProperties extends Properties {
@Override
public synchronized Object put(Object key, Object value) {
Object previousValue = super.put(key, value);
if (previousValue != null) {
throw new ElasticsearchParseException("duplicate settings key [{}] found, previous value [{}], current value [{}]", key, previousValue, value);
}
return previousValue;
}
}
} }

View File

@ -20,7 +20,6 @@
package org.elasticsearch.common.settings.loader; package org.elasticsearch.common.settings.loader;
import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.XContentType;
@ -141,7 +140,18 @@ public abstract class XContentSettingsLoader implements SettingsLoader {
sb.append(pathEle).append('.'); sb.append(pathEle).append('.');
} }
sb.append(fieldName); sb.append(fieldName);
settings.put(sb.toString(), parser.text()); String key = sb.toString();
String currentValue = parser.text();
String previousValue = settings.put(key, currentValue);
if (previousValue != null) {
throw new ElasticsearchParseException(
"duplicate settings key [{}] found at line number [{}], column number [{}], previous value [{}], current value [{}]",
key,
parser.getTokenLocation().lineNumber,
parser.getTokenLocation().columnNumber,
previousValue,
currentValue
);
}
} }
} }

View File

@ -19,19 +19,19 @@
package org.elasticsearch.common.settings.loader; package org.elasticsearch.common.settings.loader;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.ESTestCase;
import org.junit.Test; import org.junit.Test;
import static org.elasticsearch.common.settings.Settings.settingsBuilder; import static org.elasticsearch.common.settings.Settings.settingsBuilder;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.equalTo;
/** /**
* *
*/ */
public class JsonSettingsLoaderTests extends ESTestCase { public class JsonSettingsLoaderTests extends ESTestCase {
@Test @Test
public void testSimpleJsonSettings() throws Exception { public void testSimpleJsonSettings() throws Exception {
String json = "/org/elasticsearch/common/settings/loader/test-settings.json"; String json = "/org/elasticsearch/common/settings/loader/test-settings.json";
@ -50,4 +50,17 @@ public class JsonSettingsLoaderTests extends ESTestCase {
assertThat(settings.getAsArray("test1.test3")[0], equalTo("test3-1")); assertThat(settings.getAsArray("test1.test3")[0], equalTo("test3-1"));
assertThat(settings.getAsArray("test1.test3")[1], equalTo("test3-2")); assertThat(settings.getAsArray("test1.test3")[1], equalTo("test3-2"));
} }
public void testDuplicateKeysThrowsException() {
String json = "{\"foo\":\"bar\",\"foo\":\"baz\"}";
try {
settingsBuilder()
.loadFromSource(json)
.build();
fail("expected exception");
} catch (SettingsException e) {
assertEquals(e.getCause().getClass(), ElasticsearchParseException.class);
assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [1], column number [13], previous value [bar], current value [baz]"));
}
}
} }

View File

@ -0,0 +1,47 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.common.settings.loader;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.test.ESTestCase;
import java.io.IOException;
import java.nio.charset.Charset;
public class PropertiesSettingsLoaderTests extends ESTestCase {
public void testDuplicateKeyFromStringThrowsException() throws IOException {
PropertiesSettingsLoader loader = new PropertiesSettingsLoader();
try {
loader.load("foo=bar\nfoo=baz");
fail("expected exception");
} catch (ElasticsearchParseException e) {
assertEquals(e.getMessage(), "duplicate settings key [foo] found, previous value [bar], current value [baz]");
}
}
public void testDuplicateKeysFromBytesThrowsException() throws IOException {
PropertiesSettingsLoader loader = new PropertiesSettingsLoader();
try {
loader.load("foo=bar\nfoo=baz".getBytes(Charset.defaultCharset()));
} catch (ElasticsearchParseException e) {
assertEquals(e.getMessage(), "duplicate settings key [foo] found, previous value [bar], current value [baz]");
}
}
}

View File

@ -19,6 +19,7 @@
package org.elasticsearch.common.settings.loader; package org.elasticsearch.common.settings.loader;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.ESTestCase;
@ -31,7 +32,6 @@ import static org.hamcrest.Matchers.equalTo;
* *
*/ */
public class YamlSettingsLoaderTests extends ESTestCase { public class YamlSettingsLoaderTests extends ESTestCase {
@Test @Test
public void testSimpleYamlSettings() throws Exception { public void testSimpleYamlSettings() throws Exception {
String yaml = "/org/elasticsearch/common/settings/loader/test-settings.yml"; String yaml = "/org/elasticsearch/common/settings/loader/test-settings.yml";
@ -66,4 +66,17 @@ public class YamlSettingsLoaderTests extends ESTestCase {
.loadFromStream(yaml, getClass().getResourceAsStream(yaml)) .loadFromStream(yaml, getClass().getResourceAsStream(yaml))
.build(); .build();
} }
}
public void testDuplicateKeysThrowsException() {
String yaml = "foo: bar\nfoo: baz";
try {
settingsBuilder()
.loadFromSource(yaml)
.build();
fail("expected exception");
} catch (SettingsException e) {
assertEquals(e.getCause().getClass(), ElasticsearchParseException.class);
assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [2], column number [6], previous value [bar], current value [baz]"));
}
}
}