SONAR-13272 fix issue with setting some properties through env variables

6 files changed, 62 insertions, 69 deletions

diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/platform/UrlSettings.java b/server/sonar-server-common/src/main/java/org/sonar/server/platform/UrlSettings.java
index 559c251bc36..efd7254f7e1 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/platform/UrlSettings.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/platform/UrlSettings.java
@@ -27,12 +27,12 @@ import org.sonar.api.server.ServerSide;
 import static org.apache.commons.lang.StringUtils.isEmpty;
 import static org.apache.commons.lang.StringUtils.isNotEmpty;
 import static org.sonar.api.CoreProperties.SERVER_BASE_URL;
+import static org.sonar.process.ProcessProperties.Property.WEB_CONTEXT;
+import static org.sonar.process.ProcessProperties.Property.WEB_HOST;
 
 @ComputeEngineSide
 @ServerSide
 public class UrlSettings {
-  private static final String PROPERTY_CONTEXT = "sonar.web.context";
-
   private static final int DEFAULT_PORT = 9000;
   private static final int DEFAULT_HTTP_PORT = 80;
   private static final String ALL_IPS_HOST = "0.0.0.0";
@@ -43,7 +43,7 @@ public class UrlSettings {
 
   public UrlSettings(Configuration config) {
     this.config = config;
-    this.contextPath = config.get(PROPERTY_CONTEXT).orElse("")
+    this.contextPath = config.get(WEB_CONTEXT.getKey()).orElse("")
       // Remove trailing slashes
       .replaceFirst("(\\/+)$", "");
   }
@@ -66,9 +66,9 @@ public class UrlSettings {
   }
 
   private String computeBaseUrl() {
-    String host = config.get("sonar.web.host").orElse("");
+    String host = config.get(WEB_HOST.getKey()).orElse("");
     int port = config.getInt("sonar.web.port").orElse(0);
-    String context = config.get(PROPERTY_CONTEXT).orElse("");
+    String context = config.get(WEB_CONTEXT.getKey()).orElse("");
 
     StringBuilder res = new StringBuilder();
     res.append("http://");
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java b/server/sonar-server-common/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
index 74c7affdf53..0b55727ce58 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java
@@ -20,25 +20,19 @@
 package org.sonar.server.setting;
 
 import com.google.common.annotations.VisibleForTesting;
-import java.util.AbstractMap;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import org.apache.ibatis.exceptions.PersistenceException;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.ce.ComputeEngineSide;
-import org.sonar.api.config.internal.Encryption;
-import org.sonar.api.config.PropertyDefinition;
 import org.sonar.api.config.PropertyDefinitions;
+import org.sonar.api.config.internal.Encryption;
 import org.sonar.api.config.internal.Settings;
 import org.sonar.api.server.ServerSide;
-import org.sonar.api.utils.System2;
-import org.sonar.core.util.SettingFormatter;
 
 import static java.util.Collections.unmodifiableMap;
 import static java.util.Objects.requireNonNull;
@@ -61,24 +55,19 @@ import static java.util.Objects.requireNonNull;
 @ServerSide
 public class ThreadLocalSettings extends Settings {
   private final Properties systemProps = new Properties();
-  private final Properties corePropsFromEnvVariables = new Properties();
   private static final ThreadLocal<Map<String, String>> CACHE = new ThreadLocal<>();
   private Map<String, String> getPropertyDbFailureCache = Collections.emptyMap();
   private Map<String, String> getPropertiesDbFailureCache = Collections.emptyMap();
   private SettingLoader settingLoader;
-  private System2 system2;
 
-  public ThreadLocalSettings(System2 system2, PropertyDefinitions definitions, Properties props) {
-    this(system2, definitions, props, new NopSettingLoader());
+  public ThreadLocalSettings(PropertyDefinitions definitions, Properties props) {
+    this(definitions, props, new NopSettingLoader());
   }
 
   @VisibleForTesting
-  ThreadLocalSettings(System2 system2, PropertyDefinitions definitions, Properties props, SettingLoader settingLoader) {
+  ThreadLocalSettings(PropertyDefinitions definitions, Properties props, SettingLoader settingLoader) {
     super(definitions, new Encryption(null));
-    this.system2 = system2;
     this.settingLoader = settingLoader;
-
-    resolveCorePropertiesFromEnvironment();
     props.forEach((k, v) -> systemProps.put(k, v == null ? null : v.toString().trim()));
 
     // TODO something wrong about lifecycle here. It could be improved
@@ -107,9 +96,9 @@ public class ThreadLocalSettings extends Settings {
       return Optional.of(value);
     }
 
-    value = corePropsFromEnvVariables.getProperty(key);
-    if (value != null) {
-      return Optional.of(value);
+    Optional<String> envVal = getDefinitions().getValueFromEnv(key);
+    if (envVal.isPresent()) {
+      return envVal;
     }
 
     Map<String, String> dbProps = CACHE.get();
@@ -183,7 +172,7 @@ public class ThreadLocalSettings extends Settings {
   public Map<String, String> getProperties() {
     Map<String, String> result = new HashMap<>();
     loadAll(result);
-    corePropsFromEnvVariables.forEach((k, v) -> result.put((String) k, (String) v));
+    getDefinitions().getAllPropertiesSetInEnv().forEach(result::put);
     systemProps.forEach((key, value) -> result.put((String) key, (String) value));
     return unmodifiableMap(result);
   }
@@ -197,20 +186,4 @@ public class ThreadLocalSettings extends Settings {
       appendTo.putAll(getPropertiesDbFailureCache);
     }
   }
-
-  private void resolveCorePropertiesFromEnvironment() {
-    corePropsFromEnvVariables.putAll(this.getDefinitions().getAll()
-      .stream()
-      .map(PropertyDefinition::key)
-      .flatMap(p -> {
-        String envVar = SettingFormatter.fromJavaPropertyToEnvVariable(p);
-        String envVarValue = system2.envVariable(envVar);
-        if (envVarValue != null) {
-          return Stream.of(new AbstractMap.SimpleEntry<>(p, envVarValue));
-        } else {
-          return Stream.empty();
-        }
-      })
-      .collect(Collectors.toMap(AbstractMap.SimpleEntry::getKey, AbstractMap.SimpleEntry::getValue)));
-  }
 }
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/config/ConfigurationProviderTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/config/ConfigurationProviderTest.java
index 833f447f52d..29957eaa22d 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/config/ConfigurationProviderTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/config/ConfigurationProviderTest.java
@@ -28,6 +28,7 @@ import org.junit.runner.RunWith;
 import org.sonar.api.config.Configuration;
 import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.config.internal.MapSettings;
+import org.sonar.api.utils.System2;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.sonar.api.config.PropertyDefinition.builder;
@@ -38,7 +39,7 @@ public class ConfigurationProviderTest {
   private final String nonDeclaredKey = RandomStringUtils.randomAlphabetic(3);
   private final String nonMultivalueKey = RandomStringUtils.randomAlphabetic(3);
   private final String multivalueKey = RandomStringUtils.randomAlphabetic(3);
-  private final MapSettings settings = new MapSettings(new PropertyDefinitions(
+  private final MapSettings settings = new MapSettings(new PropertyDefinitions(System2.INSTANCE,
     builder(nonMultivalueKey).multiValues(false).build(),
     builder(multivalueKey).multiValues(true).build()));
 
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/platform/UrlSettingsTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/platform/UrlSettingsTest.java
index 1a2c451d633..ea842bc515b 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/platform/UrlSettingsTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/platform/UrlSettingsTest.java
@@ -24,6 +24,7 @@ import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.config.internal.MapSettings;
+import org.sonar.api.utils.System2;
 import org.sonar.core.config.CorePropertyDefinitions;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -38,7 +39,7 @@ public class UrlSettingsTest {
   @Rule
   public ExpectedException expectedException = ExpectedException.none();
 
-  private MapSettings settings = new MapSettings(new PropertyDefinitions(CorePropertyDefinitions.all()));
+  private MapSettings settings = new MapSettings(new PropertyDefinitions(System2.INSTANCE, CorePropertyDefinitions.all()));
 
   @Test
   public void use_default_context_path() {
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/setting/ChildSettingsTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/setting/ChildSettingsTest.java
index 2964925628e..4e2858e70dd 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/setting/ChildSettingsTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/setting/ChildSettingsTest.java
@@ -29,6 +29,7 @@ import org.junit.rules.ExpectedException;
 import org.sonar.api.config.PropertyDefinition;
 import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.config.internal.MapSettings;
+import org.sonar.api.utils.System2;
 
 import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -46,7 +47,7 @@ public class ChildSettingsTest {
   public void childSettings_should_retrieve_parent_settings() {
     String multipleValuesKey = randomAlphanumeric(19);
     PropertyDefinition multipleValues = PropertyDefinition.builder(multipleValuesKey).multiValues(true).build();
-    MapSettings parent = new MapSettings(new PropertyDefinitions(Collections.singletonList(multipleValues)));
+    MapSettings parent = new MapSettings(new PropertyDefinitions(System2.INSTANCE, Collections.singletonList(multipleValues)));
     ChildSettings underTest = new ChildSettings(parent);
 
     parent.setProperty(randomAlphanumeric(10), randomAlphanumeric(20));
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
index 90624314c4a..859c0ae4c20 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java
@@ -33,6 +33,7 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
+import org.sonar.api.Property;
 import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.utils.System2;
 import org.sonar.core.config.CorePropertyDefinitions;
@@ -70,7 +71,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void can_not_add_property_if_no_cache() {
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
 
     underTest.set("foo", "wiz");
 
@@ -79,7 +80,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void can_not_remove_system_property_if_no_cache() {
-    underTest = create(ImmutableMap.of("foo", "bar"));
+    underTest = create(system, ImmutableMap.of("foo", "bar"));
 
     underTest.remove("foo");
 
@@ -88,7 +89,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void add_property_to_cache() {
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
 
     underTest.load();
     underTest.set("foo", "bar");
@@ -101,7 +102,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void remove_property_from_cache() {
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
 
     underTest.load();
     underTest.set("foo", "bar");
@@ -120,7 +121,7 @@ public class ThreadLocalSettingsTest {
   @Test
   public void getProperties_does_not_fail_on_duplicated_key() {
     insertPropertyIntoDb("foo", "from_db");
-    underTest = create(ImmutableMap.of("foo", "from_system"));
+    underTest = create(system, ImmutableMap.of("foo", "from_system"));
 
     assertThat(underTest.get("foo")).hasValue("from_system");
     assertThat(underTest.getProperties().get("foo")).isEqualTo("from_system");
@@ -130,27 +131,31 @@ public class ThreadLocalSettingsTest {
   public void load_encryption_secret_key_from_system_properties() throws Exception {
     File secretKey = temp.newFile();
 
-    underTest = create(ImmutableMap.of("foo", "bar", "sonar.secretKeyPath", secretKey.getAbsolutePath()));
+    underTest = create(system, ImmutableMap.of("foo", "bar", "sonar.secretKeyPath", secretKey.getAbsolutePath()));
 
     assertThat(underTest.getEncryption().hasSecretKey()).isTrue();
   }
 
   @Test
   public void encryption_secret_key_is_undefined_by_default() {
-    underTest = create(ImmutableMap.of("foo", "bar", "sonar.secretKeyPath", "unknown/path/to/sonar-secret.txt"));
+    underTest = create(system, ImmutableMap.of("foo", "bar", "sonar.secretKeyPath", "unknown/path/to/sonar-secret.txt"));
 
     assertThat(underTest.getEncryption().hasSecretKey()).isFalse();
   }
 
-  private ThreadLocalSettings create(Map<String, String> systemProps) {
+  private ThreadLocalSettings create(System2 system, Map<String, String> systemProps) {
+    PropertyDefinitions definitions = new PropertyDefinitions(system);
+    definitions.addComponents(CorePropertyDefinitions.all());
+    definitions.addComponent(new AnnotatedTestClass());
+
     Properties p = new Properties();
     p.putAll(systemProps);
-    return new ThreadLocalSettings(system, new PropertyDefinitions(CorePropertyDefinitions.all()), p, dbSettingLoader);
+    return new ThreadLocalSettings(definitions, p, dbSettingLoader);
   }
 
   @Test
   public void load_system_properties() {
-    underTest = create(ImmutableMap.of("foo", "1", "bar", "2"));
+    underTest = create(system, ImmutableMap.of("foo", "1", "bar", "2"));
 
     assertThat(underTest.get("foo")).hasValue("1");
     assertThat(underTest.get("missing")).isNotPresent();
@@ -160,17 +165,19 @@ public class ThreadLocalSettingsTest {
   @Test
   public void load_core_properties_from_environment() {
     when(system.envVariable("SONAR_FORCEAUTHENTICATION")).thenReturn("true");
-    underTest = create(ImmutableMap.of());
+    when(system.envVariable("SONAR_ANNOTATION_TEST_PROP")).thenReturn("113");
+    underTest = create(system, ImmutableMap.of());
 
     assertThat(underTest.get("sonar.forceAuthentication")).hasValue("true");
+    assertThat(underTest.get("sonar.annotation.test.prop")).hasValue("113");
     assertThat(underTest.get("missing")).isNotPresent();
-    assertThat(underTest.getProperties()).containsOnly(entry("sonar.forceAuthentication", "true"));
+    assertThat(underTest.getProperties()).containsOnly(entry("sonar.forceAuthentication", "true"), entry("sonar.annotation.test.prop", "113"));
   }
 
   @Test
   public void database_properties_are_not_cached_by_default() {
     insertPropertyIntoDb("foo", "from db");
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
 
     assertThat(underTest.get("foo")).hasValue("from db");
 
@@ -182,7 +189,7 @@ public class ThreadLocalSettingsTest {
   @Test
   public void system_settings_have_precedence_over_database() {
     insertPropertyIntoDb("foo", "from db");
-    underTest = create(ImmutableMap.of("foo", "from system"));
+    underTest = create(system, ImmutableMap.of("foo", "from system"));
 
     assertThat(underTest.get("foo")).hasValue("from system");
   }
@@ -191,7 +198,7 @@ public class ThreadLocalSettingsTest {
   public void getProperties_are_all_properties_with_value() {
     insertPropertyIntoDb("db", "from db");
     insertPropertyIntoDb("empty", "");
-    underTest = create(ImmutableMap.of("system", "from system"));
+    underTest = create(system, ImmutableMap.of("system", "from system"));
 
     assertThat(underTest.getProperties()).containsOnly(entry("system", "from system"), entry("db", "from db"), entry("empty", ""));
   }
@@ -199,7 +206,7 @@ public class ThreadLocalSettingsTest {
   @Test
   public void getProperties_is_not_cached_in_thread_cache() {
     insertPropertyIntoDb("foo", "bar");
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
     underTest.load();
 
     assertThat(underTest.getProperties())
@@ -219,7 +226,7 @@ public class ThreadLocalSettingsTest {
   public void load_creates_a_thread_specific_cache() throws InterruptedException {
     insertPropertyIntoDb(A_KEY, "v1");
 
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
     underTest.load();
 
     assertThat(underTest.get(A_KEY)).hasValue("v1");
@@ -239,7 +246,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void load_invalidates_cache_if_unload_has_not_been_called() {
-    underTest = create(emptyMap());
+    underTest = create(system, emptyMap());
     underTest.load();
     underTest.set("foo", "bar");
     // unload() is not called
@@ -250,7 +257,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void keep_in_thread_cache_the_fact_that_a_property_is_not_in_db() {
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
     underTest.load();
     assertThat(underTest.get(A_KEY)).isNotPresent();
 
@@ -263,7 +270,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void change_setting_loader() {
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties());
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties());
 
     assertThat(underTest.getSettingLoader()).isNotNull();
 
@@ -274,7 +281,7 @@ public class ThreadLocalSettingsTest {
 
   @Test
   public void cache_db_calls_if_property_is_not_persisted() {
-    underTest = create(Collections.emptyMap());
+    underTest = create(system, Collections.emptyMap());
     underTest.load();
     assertThat(underTest.get(A_KEY)).isNotPresent();
     assertThat(underTest.get(A_KEY)).isNotPresent();
@@ -286,7 +293,7 @@ public class ThreadLocalSettingsTest {
     SettingLoader settingLoaderMock = mock(SettingLoader.class);
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
     doThrow(toBeThrown).when(settingLoaderMock).loadAll();
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties(), settingLoaderMock);
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties(), settingLoaderMock);
 
     assertThat(underTest.getProperties())
       .isEmpty();
@@ -297,7 +304,7 @@ public class ThreadLocalSettingsTest {
     SettingLoader settingLoaderMock = mock(SettingLoader.class);
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
     doThrow(toBeThrown).when(settingLoaderMock).loadAll();
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties(), settingLoaderMock);
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties(), settingLoaderMock);
     underTest.load();
 
     assertThat(underTest.getProperties())
@@ -316,7 +323,7 @@ public class ThreadLocalSettingsTest {
       .doAnswer(invocationOnMock -> ImmutableMap.of(key, value2))
       .when(settingLoaderMock)
       .loadAll();
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties(), settingLoaderMock);
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties(), settingLoaderMock);
 
     underTest.load();
     assertThat(underTest.getProperties())
@@ -340,7 +347,7 @@ public class ThreadLocalSettingsTest {
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
     String key = randomAlphanumeric(3);
     doThrow(toBeThrown).when(settingLoaderMock).load(key);
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties(), settingLoaderMock);
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties(), settingLoaderMock);
 
     assertThat(underTest.get(key)).isEmpty();
   }
@@ -351,7 +358,7 @@ public class ThreadLocalSettingsTest {
     PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB");
     String key = randomAlphanumeric(3);
     doThrow(toBeThrown).when(settingLoaderMock).load(key);
-    underTest = new ThreadLocalSettings(system, new PropertyDefinitions(), new Properties(), settingLoaderMock);
+    underTest = new ThreadLocalSettings(new PropertyDefinitions(system), new Properties(), settingLoaderMock);
     underTest.load();
 
     assertThat(underTest.get(key)).isEmpty();
@@ -417,4 +424,14 @@ public class ThreadLocalSettingsTest {
       return unmodifiableMap(map);
     }
   }
+
+  @org.sonar.api.Properties({
+    @Property(
+      key = "sonar.annotation.test.prop",
+      defaultValue = "60",
+      name = "Test annotation property",
+      global = false)
+  })
+  class AnnotatedTestClass {
+  }
 }