From 2022f77214c6456941f10591e435db8ebead08c0 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 19 Dec 2017 13:42:58 +0100 Subject: SONAR-8216 System info page fails when a setting is defined both in sonar.properties and in DB --- .../server/setting/DatabaseSettingLoader.java | 10 +++--- .../org/sonar/server/setting/NopSettingLoader.java | 7 ++-- .../org/sonar/server/setting/SettingLoader.java | 4 +-- .../sonar/server/setting/ThreadLocalSettings.java | 24 +++++++------- .../server/setting/DatabaseSettingLoaderTest.java | 25 ++++++++------- .../sonar/server/setting/NopSettingLoaderTest.java | 6 +--- .../server/setting/ThreadLocalSettingsTest.java | 37 ++++++++++++---------- 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 appendTo) { + public Map 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 appendTo) { - // nothing to load + public Map 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 appendTo); + Map 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. * *

* System settings have precedence on others. @@ -167,21 +167,19 @@ public class ThreadLocalSettings extends Settings { @Override public Map getProperties() { - ImmutableMap.Builder builder = ImmutableMap.builder(); - loadAll(builder); - systemProps.forEach((key, value) -> builder.put((String) key, (String) value)); - return builder.build(); + Map result = new HashMap<>(); + loadAll(result); + systemProps.forEach((key, value) -> result.put((String) key, (String) value)); + return unmodifiableMap(result); } - private void loadAll(ImmutableMap.Builder builder) { + private void loadAll(Map appendTo) { try { - ImmutableMap.Builder cacheBuilder = ImmutableMap.builder(); - settingLoader.loadAll(cacheBuilder); - Map cache = cacheBuilder.build(); - builder.putAll(cache); + Map 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 map = ImmutableMap.builder(); - underTest.loadAll(map); - assertThat(map.build().isEmpty()).isTrue(); + Map map = underTest.loadAll(); + assertThat(map).isEmpty(); } - @Test - public void test_loadAll() { - insertPropertyIntoDb("foo", "1"); - insertPropertyIntoDb("bar", "2"); - ImmutableMap.Builder 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 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 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 builder = (ImmutableMap.Builder) invocationOnMock.getArguments()[0]; - builder.put(key, value1); - return null; - }).doThrow(toBeThrown) - .doAnswer(invocationOnMock -> { - ImmutableMap.Builder builder = (ImmutableMap.Builder) 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 appendTo) { - appendTo.putAll(map); + public Map loadAll() { + return unmodifiableMap(map); } } } -- cgit v1.2.3