SONAR-8216 System info page fails when a setting is defined both in sonar.properties and in DB

7 files changed, 58 insertions, 55 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/DatabaseSettingLoader.java b/server/sonar-server/src/main/java/org/sonar/server/setting/DatabaseSettingLoader.java
index 7e4203adae4..c8be9a21f24 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/setting/DatabaseSettingLoader.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/setting/DatabaseSettingLoader.java
@@ -19,7 +19,8 @@
  */
 package org.sonar.server.setting;
 
-import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.sonar.core.util.stream.MoreCollectors;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.property.PropertyDto;
@@ -44,10 +45,11 @@ public class DatabaseSettingLoader implements SettingLoader {
   }
 
   @Override
-  public void loadAll(ImmutableMap.Builder<String, String> appendTo) {
+  public Map<String, String> loadAll() {
     try (DbSession dbSession = dbClient.openSession(false)) {
-      dbClient.propertiesDao().selectGlobalProperties(dbSession)
-        .forEach(p -> appendTo.put(p.getKey(), defaultString(p.getValue())));
+      return dbClient.propertiesDao().selectGlobalProperties(dbSession)
+        .stream()
+        .collect(MoreCollectors.uniqueIndex(PropertyDto::getKey, p -> defaultString(p.getValue())));
     }
   }
 
diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/NopSettingLoader.java b/server/sonar-server/src/main/java/org/sonar/server/setting/NopSettingLoader.java
index 4c494e34519..e161d5f090e 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/setting/NopSettingLoader.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/setting/NopSettingLoader.java
@@ -19,7 +19,8 @@
  */
 package org.sonar.server.setting;
 
-import com.google.common.collect.ImmutableMap;
+import java.util.Collections;
+import java.util.Map;
 
 public class NopSettingLoader implements SettingLoader {
   @Override
@@ -28,8 +29,8 @@ public class NopSettingLoader implements SettingLoader {
   }
 
   @Override
-  public void loadAll(ImmutableMap.Builder<String, String> appendTo) {
-    // nothing to load
+  public Map<String, String> loadAll() {
+    return Collections.emptyMap();
   }
 
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/SettingLoader.java b/server/sonar-server/src/main/java/org/sonar/server/setting/SettingLoader.java
index 9115e7da8c8..49758eac29b 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/setting/SettingLoader.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/setting/SettingLoader.java
@@ -19,7 +19,7 @@
  */
 package org.sonar.server.setting;
 
-import com.google.common.collect.ImmutableMap;
+import java.util.Map;
 import javax.annotation.CheckForNull;
 
 public interface SettingLoader {
@@ -27,6 +27,6 @@ public interface SettingLoader {
   @CheckForNull
   String load(String key);
 
-  void loadAll(ImmutableMap.Builder<String, String> appendTo);
+  Map<String,String> loadAll();
 
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
index a12c4963e02..9a131fb7ed5 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
@@ -20,7 +20,6 @@
 package org.sonar.server.setting;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableMap;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -36,11 +35,12 @@ import org.sonar.api.config.Settings;
 import org.sonar.api.server.ServerSide;
 
 import static com.google.common.base.Preconditions.checkState;
+import static java.util.Collections.unmodifiableMap;
 import static java.util.Objects.requireNonNull;
 
 /**
- * Merge of {@link SystemSettings} and the global properties stored in the db table "properties". These
- * settings do not contain the settings specific to a project.
+ * Merge of system settings (including conf/sonar.properties) and the global properties stored
+ * in the db table "properties". These settings do not contain the settings specific to a project.
  *
  * <p>
  * System settings have precedence on others.
@@ -167,21 +167,19 @@ public class ThreadLocalSettings extends Settings {
 
   @Override
   public Map<String, String> getProperties() {
-    ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
-    loadAll(builder);
-    systemProps.forEach((key, value) -> builder.put((String) key, (String) value));
-    return builder.build();
+    Map<String, String> result = new HashMap<>();
+    loadAll(result);
+    systemProps.forEach((key, value) -> result.put((String) key, (String) value));
+    return unmodifiableMap(result);
   }
 
-  private void loadAll(ImmutableMap.Builder<String, String> builder) {
+  private void loadAll(Map<String, String> appendTo) {
     try {
-      ImmutableMap.Builder<String, String> cacheBuilder = ImmutableMap.builder();
-      settingLoader.loadAll(cacheBuilder);
-      Map<String, String> cache = cacheBuilder.build();
-      builder.putAll(cache);
+      Map<String, String> cache = settingLoader.loadAll();
+      appendTo.putAll(cache);
       getPropertiesDbFailureCache = cache;
     } catch (PersistenceException e) {
-      builder.putAll(getPropertiesDbFailureCache);
+      appendTo.putAll(getPropertiesDbFailureCache);
     }
   }
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/DatabaseSettingLoaderTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/DatabaseSettingLoaderTest.java
index 922554bec55..d14c66605ce 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/setting/DatabaseSettingLoaderTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/setting/DatabaseSettingLoaderTest.java
@@ -19,7 +19,7 @@
  */
 package org.sonar.server.setting;
 
-import com.google.common.collect.ImmutableMap;
+import java.util.Map;
 import org.junit.Rule;
 import org.junit.Test;
 import org.sonar.api.utils.System2;
@@ -49,6 +49,7 @@ public class DatabaseSettingLoaderTest {
   @Test
   public void null_value_in_db_is_considered_as_empty_string() {
     insertPropertyIntoDb(A_KEY, null);
+
     assertThat(underTest.load(A_KEY)).isEqualTo("");
   }
 
@@ -60,19 +61,19 @@ public class DatabaseSettingLoaderTest {
 
   @Test
   public void test_loadAll_with_no_properties() {
-    ImmutableMap.Builder<String, String> map = ImmutableMap.builder();
-    underTest.loadAll(map);
-   assertThat(map.build().isEmpty()).isTrue();
+    Map<String, String> map = underTest.loadAll();
+    assertThat(map).isEmpty();
   }
 
- @Test
- public void test_loadAll() {
-  insertPropertyIntoDb("foo", "1");
-  insertPropertyIntoDb("bar", "2");
-  ImmutableMap.Builder<String, String> map = ImmutableMap.builder();
-  underTest.loadAll(map);
-  assertThat(map.build()).containsOnly(entry("foo", "1"), entry("bar", "2"));
- }
+  @Test
+  public void test_loadAll() {
+    insertPropertyIntoDb("foo", "1");
+    insertPropertyIntoDb("bar", "2");
+
+    Map<String, String> map = underTest.loadAll();
+
+    assertThat(map).containsOnly(entry("foo", "1"), entry("bar", "2"));
+  }
 
   private void insertPropertyIntoDb(String key, String value) {
     dbTester.getDbClient().propertiesDao().saveProperty(new PropertyDto().setKey(key).setValue(value));
diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/NopSettingLoaderTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/NopSettingLoaderTest.java
index 329abda074e..798e534f4c8 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/setting/NopSettingLoaderTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/setting/NopSettingLoaderTest.java
@@ -19,9 +19,7 @@
  */
 package org.sonar.server.setting;
 
-import com.google.common.collect.ImmutableMap;
 import org.junit.Test;
-import org.sonar.server.setting.NopSettingLoader;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -33,8 +31,6 @@ public class NopSettingLoaderTest {
   public void do_nothing() {
     assertThat(underTest.load("foo")).isNull();
 
-    ImmutableMap.Builder<String,String> map = ImmutableMap.builder();
-    underTest.loadAll(map);
-    assertThat(map.build()).isEmpty();
+    assertThat(underTest.loadAll()).isEmpty();
   }
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
index 9dc3e4df4e7..4884c5fa8e7 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
@@ -35,11 +35,11 @@ import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.sonar.api.config.PropertyDefinitions;
 
+import static java.util.Collections.unmodifiableMap;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.data.MapEntry.entry;
-import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
@@ -109,6 +109,18 @@ public class ThreadLocalSettingsTest {
     assertThat(underTest.get("foo")).isNotPresent();
   }
 
+  /**
+   * SONAR-8216 System info page fails when a setting is defined both in sonar.properties and in DB
+   */
+  @Test
+  public void getProperties_does_not_fail_on_duplicated_key() {
+    insertPropertyIntoDb("foo", "from_db");
+    underTest = create(ImmutableMap.of("foo", "from_system"));
+
+    assertThat(underTest.get("foo")).hasValue("from_system");
+    assertThat(underTest.getProperties().get("foo")).isEqualTo("from_system");
+  }
+
   @Test
   public void load_encryption_secret_key_from_system_properties() throws Exception {
     File secretKey = temp.newFile();
@@ -259,7 +271,7 @@ public class ThreadLocalSettingsTest {
   public void getProperties_return_empty_if_DB_error_on_first_call_ever_out_of_thread_cache() {
     SettingLoader settingLoaderMock = mock(SettingLoader.class);
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
-    doThrow(toBeThrown).when(settingLoaderMock).loadAll(any(ImmutableMap.Builder.class));
+    doThrow(toBeThrown).when(settingLoaderMock).loadAll();
     underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock);
 
     assertThat(underTest.getProperties())
@@ -270,7 +282,7 @@ public class ThreadLocalSettingsTest {
   public void getProperties_returns_empty_if_DB_error_on_first_call_ever_in_thread_cache() {
     SettingLoader settingLoaderMock = mock(SettingLoader.class);
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
-    doThrow(toBeThrown).when(settingLoaderMock).loadAll(any(ImmutableMap.Builder.class));
+    doThrow(toBeThrown).when(settingLoaderMock).loadAll();
     underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock);
     underTest.load();
 
@@ -285,18 +297,11 @@ public class ThreadLocalSettingsTest {
     String value2 = randomAlphanumeric(5);
     SettingLoader settingLoaderMock = mock(SettingLoader.class);
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
-    doAnswer(invocationOnMock -> {
-      ImmutableMap.Builder<String, String> builder = (ImmutableMap.Builder<String, String>) invocationOnMock.getArguments()[0];
-      builder.put(key, value1);
-      return null;
-    }).doThrow(toBeThrown)
-      .doAnswer(invocationOnMock -> {
-        ImmutableMap.Builder<String, String> builder = (ImmutableMap.Builder<String, String>) invocationOnMock.getArguments()[0];
-        builder.put(key, value2);
-        return null;
-      })
+    doAnswer(invocationOnMock -> ImmutableMap.of(key, value1))
+      .doThrow(toBeThrown)
+      .doAnswer(invocationOnMock -> ImmutableMap.of(key, value2))
       .when(settingLoaderMock)
-      .loadAll(any(ImmutableMap.Builder.class));
+      .loadAll();
     underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock);
 
     underTest.load();
@@ -394,8 +399,8 @@ public class ThreadLocalSettingsTest {
     }
 
     @Override
-    public void loadAll(ImmutableMap.Builder<String, String> appendTo) {
-      appendTo.putAll(map);
+    public Map<String, String> loadAll() {
+      return unmodifiableMap(map);
     }
   }
 }