Skip to content

Commit

Permalink
Internal: Convert empty and size checks of settings to not use getAsM…
Browse files Browse the repository at this point in the history 
…ap() (#22890)

With the new secure settings, methods like getAsMap() no longer work
correctly as a means of checking for empty settings, or the total size.
This change converts the existing uses of that method to use methods
directly on Settings. Note this does not update the implementations to
account for SecureSettings, as that will require a followup which
changes how secure settings work.
  • Loading branch information
@rjernst
rjernst authored Jan 31, 2017
1 parent f6d38d4 commit 29f63c7
Show file tree
Hide file tree
Showing 16 changed files with 51 additions and 47 deletions.
2 changes: 1 addition & 1 deletion ...in/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public ClusterUpdateSettingsRequest() {
@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
if (transientSettings.getAsMap().isEmpty() && persistentSettings.getAsMap().isEmpty()) {
if (transientSettings.isEmpty() && persistentSettings.isEmpty()) {
validationException = addValidationError("no settings to update", validationException);
}
return validationException;
Expand Down
6 changes: 3 additions & 3 deletions ...org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ protected String executor() {
@Override
protected ClusterBlockException checkBlock(ClusterUpdateSettingsRequest request, ClusterState state) {
// allow for dedicated changes to the metadata blocks, so we don't block those to allow to "re-enable" it
if ((request.transientSettings().getAsMap().isEmpty() &&
request.persistentSettings().getAsMap().size() == 1 &&
if ((request.transientSettings().isEmpty() &&
request.persistentSettings().size() == 1 &&
MetaData.SETTING_READ_ONLY_SETTING.exists(request.persistentSettings())) ||
(request.persistentSettings().getAsMap().isEmpty() && request.transientSettings().getAsMap().size() == 1 &&
(request.persistentSettings().isEmpty() && request.transientSettings().size() == 1 &&
MetaData.SETTING_READ_ONLY_SETTING.exists(request.transientSettings()))) {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion ...va/org/elasticsearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected ClusterBlockException checkBlock(UpdateSettingsRequest request, Cluste
if (globalBlock != null) {
return globalBlock;
}
if (request.settings().getAsMap().size() == 1 && IndexMetaData.INDEX_BLOCKS_METADATA_SETTING.exists(request.settings()) || IndexMetaData.INDEX_READ_ONLY_SETTING.exists(request.settings())) {
if (request.settings().size() == 1 && IndexMetaData.INDEX_BLOCKS_METADATA_SETTING.exists(request.settings()) || IndexMetaData.INDEX_READ_ONLY_SETTING.exists(request.settings())) {
return null;
}
return state.blocks().indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request));
Expand Down
2 changes: 1 addition & 1 deletion .../main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public UpdateSettingsRequest(Settings settings, String... indices) {
@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
if (settings.getAsMap().isEmpty()) {
if (settings.isEmpty()) {
validationException = addValidationError("no settings to update", validationException);
}
return validationException;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -973,15 +973,15 @@ public static void toXContent(MetaData metaData, XContentBuilder builder, ToXCon
builder.field("version", metaData.version());
builder.field("cluster_uuid", metaData.clusterUUID);

if (!metaData.persistentSettings().getAsMap().isEmpty()) {
if (!metaData.persistentSettings().isEmpty()) {
builder.startObject("settings");
for (Map.Entry<String, String> entry : metaData.persistentSettings().getAsMap().entrySet()) {
builder.field(entry.getKey(), entry.getValue());
}
builder.endObject();
}

if (context == XContentContext.API && !metaData.transientSettings().getAsMap().isEmpty()) {
if (context == XContentContext.API && !metaData.transientSettings().isEmpty()) {
builder.startObject("transient_settings");
for (Map.Entry<String, String> entry : metaData.transientSettings().getAsMap().entrySet()) {
builder.field(entry.getKey(), entry.getValue());
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public ClusterState execute(ClusterState currentState) {
closeIndices
));
}
if (!skippedSettigns.getAsMap().isEmpty() && !openIndices.isEmpty()) {
if (!skippedSettigns.isEmpty() && !openIndices.isEmpty()) {
throw new IllegalArgumentException(String.format(Locale.ROOT,
"Can't update non dynamic settings [%s] for open indices %s",
skippedSettigns.getAsMap().keySet(),
Expand Down
10 changes: 7 additions & 3 deletions core/src/main/java/org/elasticsearch/common/settings/Settings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.loader.SettingsLoader;
import org.elasticsearch.common.settings.loader.SettingsLoaderFactory;
import org.elasticsearch.common.unit.ByteSizeUnit;
Expand Down Expand Up @@ -589,7 +588,7 @@ public static Settings readSettingsFromStream(StreamInput in) throws IOException
}

public static void writeSettingsToStream(Settings settings, StreamOutput out) throws IOException {
out.writeVInt(settings.getAsMap().size());
out.writeVInt(settings.size());
for (Map.Entry<String, String> entry : settings.getAsMap().entrySet()) {
out.writeString(entry.getKey());
out.writeOptionalString(entry.getValue());
Expand Down Expand Up @@ -626,7 +625,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
* @return <tt>true</tt> if this settings object contains no settings
*/
public boolean isEmpty() {
return this.settings.isEmpty();
return this.settings.isEmpty(); // TODO: account for secure settings
}

/** Returns the number of settings in this settings object. */
public int size() {
return this.settings.size(); // TODO: account for secure settings
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public RestResponse buildResponse(GetSettingsResponse getSettingsResponse, XCont
builder.startObject();
for (ObjectObjectCursor<String, Settings> cursor : getSettingsResponse.getIndexToSettings()) {
// no settings, jump over it to shorten the response data
if (cursor.value.getAsMap().isEmpty()) {
if (cursor.value.isEmpty()) {
continue;
}
builder.startObject(cursor.key);
Expand Down
24 changes: 12 additions & 12 deletions core/src/test/java/org/elasticsearch/cluster/metadata/ToAndFromJsonMetaDataTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(3));
assertThat(indexMetaData.getSettings().size(), equalTo(3));
assertThat(indexMetaData.getMappings().size(), equalTo(0));

indexMetaData = parsedMetaData.index("test2");
Expand All @@ -164,7 +164,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.primaryTerm(0), equalTo(2L));
assertThat(indexMetaData.primaryTerm(1), equalTo(2L));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(5));
assertThat(indexMetaData.getSettings().size(), equalTo(5));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(0));
Expand All @@ -173,22 +173,22 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(3));
assertThat(indexMetaData.getSettings().size(), equalTo(3));
assertThat(indexMetaData.getMappings().size(), equalTo(1));
assertThat(indexMetaData.getMappings().get("mapping1").source().string(), equalTo(MAPPING_SOURCE1));

indexMetaData = parsedMetaData.index("test4");
assertThat(indexMetaData.getCreationDate(), equalTo(2L));
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(4));
assertThat(indexMetaData.getSettings().size(), equalTo(4));
assertThat(indexMetaData.getMappings().size(), equalTo(0));

indexMetaData = parsedMetaData.index("test5");
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(5));
assertThat(indexMetaData.getSettings().size(), equalTo(5));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand All @@ -199,7 +199,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(2L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(6));
assertThat(indexMetaData.getSettings().size(), equalTo(6));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(0));
Expand All @@ -208,7 +208,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(2L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(4));
assertThat(indexMetaData.getSettings().size(), equalTo(4));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
assertThat(indexMetaData.getMappings().get("mapping1").source().string(), equalTo(MAPPING_SOURCE1));
assertThat(indexMetaData.getMappings().get("mapping2").source().string(), equalTo(MAPPING_SOURCE2));
Expand All @@ -217,7 +217,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(5));
assertThat(indexMetaData.getSettings().size(), equalTo(5));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand All @@ -231,7 +231,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(2L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(6));
assertThat(indexMetaData.getSettings().size(), equalTo(6));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand All @@ -245,7 +245,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(5));
assertThat(indexMetaData.getSettings().size(), equalTo(5));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand All @@ -259,7 +259,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(-1L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(5));
assertThat(indexMetaData.getSettings().size(), equalTo(5));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand All @@ -277,7 +277,7 @@ public void testSimpleJsonFromAndTo() throws IOException {
assertThat(indexMetaData.getNumberOfShards(), equalTo(1));
assertThat(indexMetaData.getNumberOfReplicas(), equalTo(2));
assertThat(indexMetaData.getCreationDate(), equalTo(2L));
assertThat(indexMetaData.getSettings().getAsMap().size(), equalTo(6));
assertThat(indexMetaData.getSettings().size(), equalTo(6));
assertThat(indexMetaData.getSettings().get("setting1"), equalTo("value1"));
assertThat(indexMetaData.getSettings().get("setting2"), equalTo("value2"));
assertThat(indexMetaData.getMappings().size(), equalTo(2));
Expand Down
18 changes: 9 additions & 9 deletions core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,21 +270,21 @@ public void testDiff() throws IOException {
ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux,
someGroup, someAffix)));
Settings diff = settings.diff(Settings.builder().put("foo.bar", 5).build(), Settings.EMPTY);
assertEquals(4, diff.getAsMap().size()); // 4 since foo.bar.quux has 3 values essentially
assertEquals(4, diff.size()); // 4 since foo.bar.quux has 3 values essentially
assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"});

diff = settings.diff(
Settings.builder().put("foo.bar", 5).build(),
Settings.builder().put("foo.bar.baz", 17).putArray("foo.bar.quux", "d", "e", "f").build());
assertEquals(4, diff.getAsMap().size()); // 4 since foo.bar.quux has 3 values essentially
assertEquals(4, diff.size()); // 4 since foo.bar.quux has 3 values essentially
assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(17));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"d", "e", "f"});

diff = settings.diff(
Settings.builder().put("some.group.foo", 5).build(),
Settings.builder().put("some.group.foobar", 17, "some.group.foo", 25).build());
assertEquals(6, diff.getAsMap().size()); // 6 since foo.bar.quux has 3 values essentially
assertEquals(6, diff.size()); // 6 since foo.bar.quux has 3 values essentially
assertThat(diff.getAsInt("some.group.foobar", null), equalTo(17));
assertNull(diff.get("some.group.foo"));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"});
Expand All @@ -295,7 +295,7 @@ public void testDiff() throws IOException {
Settings.builder().put("some.prefix.foo.somekey", 5).build(),
Settings.builder().put("some.prefix.foobar.somekey", 17,
"some.prefix.foo.somekey", 18).build());
assertEquals(6, diff.getAsMap().size()); // 6 since foo.bar.quux has 3 values essentially
assertEquals(6, diff.size()); // 6 since foo.bar.quux has 3 values essentially
assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17));
assertNull(diff.get("some.prefix.foo.somekey"));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"});
Expand All @@ -314,21 +314,21 @@ public void testDiffWithAffixAndComplexMatcher() {
ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux,
someGroup, someAffix)));
Settings diff = settings.diff(Settings.builder().put("foo.bar", 5).build(), Settings.EMPTY);
assertEquals(1, diff.getAsMap().size());
assertEquals(1, diff.size());
assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1));
assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys

diff = settings.diff(
Settings.builder().put("foo.bar", 5).build(),
Settings.builder().put("foo.bar.baz", 17).putArray("foo.bar.quux", "d", "e", "f").build());
assertEquals(4, diff.getAsMap().size());
assertEquals(4, diff.size());
assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(17));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"d", "e", "f"});

diff = settings.diff(
Settings.builder().put("some.group.foo", 5).build(),
Settings.builder().put("some.group.foobar", 17, "some.group.foo", 25).build());
assertEquals(3, diff.getAsMap().size());
assertEquals(3, diff.size());
assertThat(diff.getAsInt("some.group.foobar", null), equalTo(17));
assertNull(diff.get("some.group.foo"));
assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys
Expand All @@ -339,7 +339,7 @@ public void testDiffWithAffixAndComplexMatcher() {
Settings.builder().put("some.prefix.foo.somekey", 5).build(),
Settings.builder().put("some.prefix.foobar.somekey", 17,
"some.prefix.foo.somekey", 18).build());
assertEquals(3, diff.getAsMap().size());
assertEquals(3, diff.size());
assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17));
assertNull(diff.get("some.prefix.foo.somekey"));
assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys
Expand All @@ -353,7 +353,7 @@ public void testDiffWithAffixAndComplexMatcher() {
.putArray("foo.bar.quux", "x", "y", "z")
.putArray("foo.baz.quux", "d", "e", "f")
.build());
assertEquals(9, diff.getAsMap().size());
assertEquals(9, diff.size());
assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17));
assertNull(diff.get("some.prefix.foo.somekey"));
assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"x", "y", "z"});
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/elasticsearch/common/settings/SettingsModuleTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public void testRegisterSettingsFilter() {
Setting.boolSetting("bar.foo", true, Property.NodeScope, Property.Filtered),
Setting.boolSetting("bar.baz", true, Property.NodeScope)), Arrays.asList("foo.*"));
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
assertInstanceBinding(module, SettingsFilter.class, (s) -> s.filter(settings).getAsMap().size() == 1);
assertInstanceBinding(module, SettingsFilter.class, (s) -> s.filter(settings).size() == 1);
assertInstanceBinding(module, SettingsFilter.class, (s) -> s.filter(settings).getAsMap().containsKey("bar.baz"));
assertInstanceBinding(module, SettingsFilter.class, (s) -> s.filter(settings).getAsMap().get("bar.baz").equals("false"));

Expand Down
Loading

0 comments on commit 29f63c7

Please sign in to comment.