Settings: Fix setting groups to include secure settings

This commit fixes the group methdos of Settings to properly include grouped secure settings. Previously the secure settings were included but without the group prefix being removed. closes elastic#25069
rjernst · Jun 6, 2017 · 028a547 · 028a547
1 parent ac82824
commit 028a547
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 26 deletions.
diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java
@@ -507,35 +507,21 @@ public Map<String, Settings> getGroups(String settingPrefix, boolean ignoreNonGr
     }
 
     private Map<String, Settings> getGroupsInternal(String settingPrefix, boolean ignoreNonGrouped) throws SettingsException {
-        // we don't really care that it might happen twice
-        Map<String, Map<String, String>> map = new LinkedHashMap<>();
-        for (Object o : settings.keySet()) {
-            String setting = (String) o;
-            if (setting.startsWith(settingPrefix)) {
-                String nameValue = setting.substring(settingPrefix.length());
-                int dotIndex = nameValue.indexOf('.');
-                if (dotIndex == -1) {
-                    if (ignoreNonGrouped) {
-                        continue;
-                    }
-                    throw new SettingsException("Failed to get setting group for [" + settingPrefix + "] setting prefix and setting ["
-                            + setting + "] because of a missing '.'");
-                }
-                String name = nameValue.substring(0, dotIndex);
-                String value = nameValue.substring(dotIndex + 1);
-                Map<String, String> groupSettings = map.get(name);
-                if (groupSettings == null) {
-                    groupSettings = new LinkedHashMap<>();
-                    map.put(name, groupSettings);
+        Settings prefixSettings = getByPrefix(settingPrefix);
+        Map<String, Settings> groups = new HashMap<>();
+        for (String groupName : prefixSettings.names()) {
+            Settings groupSettings = prefixSettings.getByPrefix(groupName + ".");
+            if (groupSettings.isEmpty()) {
+                if (ignoreNonGrouped) {
+                    continue;
                 }
-                groupSettings.put(value, get(setting));
+                throw new SettingsException("Failed to get setting group for [" + settingPrefix + "] setting prefix and setting ["
+                    + settingPrefix + groupName + "] because of a missing '.'");
             }
+            groups.put(groupName, groupSettings);
         }
-        Map<String, Settings> retVal = new LinkedHashMap<>();
-        for (Map.Entry<String, Map<String, String>> entry : map.entrySet()) {
-            retVal.put(entry.getKey(), new Settings(Collections.unmodifiableMap(entry.getValue()), secureSettings));
-        }
-        return Collections.unmodifiableMap(retVal);
+
+        return Collections.unmodifiableMap(groups);
     }
     /**
      * Returns group settings for the given setting prefix.

diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
@@ -38,6 +38,7 @@
 import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.arrayContaining;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasToString;
@@ -525,6 +526,29 @@ public void testSecureSettingsPrefix() {
         assertTrue(prefixSettings.names().contains("foo"));
     }
 
+    public void testGroupPrefix() {
+        MockSecureSettings secureSettings = new MockSecureSettings();
+        secureSettings.setString("test.key1.foo", "somethingsecure");
+        secureSettings.setString("test.key1.bar", "somethingsecure");
+        secureSettings.setString("test.key2.foo", "somethingsecure");
+        secureSettings.setString("test.key2.bog", "somethingsecure");
+        Settings.Builder builder = Settings.builder();
+        builder.put("test.key1.baz", "blah1");
+        builder.put("test.key1.other", "blah2");
+        builder.put("test.key2.baz", "blah3");
+        builder.put("test.key2.else", "blah4");
+        builder.setSecureSettings(secureSettings);
+        Settings settings = builder.build();
+        Map<String, Settings> groups = settings.getGroups("test");
+        assertEquals(2, groups.size());
+        Settings key1 = groups.get("key1");
+        assertNotNull(key1);
+        assertThat(key1.names(), containsInAnyOrder("foo", "bar", "baz", "other"));
+        Settings key2 = groups.get("key2");
+        assertNotNull(key2);
+        assertThat(key2.names(), containsInAnyOrder("foo", "bog", "baz", "else"));
+    }
+
     public void testEmptyFilterMap() {
         Settings.Builder builder = Settings.builder();
         builder.put("a", "a1");