Use SettingsLoader only when needed for branches

7 files changed, 62 insertions, 87 deletions

diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java
index a999fa980cf..928d9c8ba1f 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java
@@ -20,19 +20,15 @@
 package org.sonar.scanner.repository.settings;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Throwables;
 import java.io.IOException;
 import java.io.InputStream;
-import java.net.HttpURLConnection;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.apache.commons.lang.StringEscapeUtils;
-import org.sonar.api.utils.MessageException;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
 import org.sonar.api.utils.log.Profiler;
@@ -42,10 +38,9 @@ import org.sonarqube.ws.Settings.FieldValues.Value;
 import org.sonarqube.ws.Settings.Setting;
 import org.sonarqube.ws.Settings.ValuesWsResponse;
 import org.sonarqube.ws.client.GetRequest;
-import org.sonarqube.ws.client.HttpException;
 
 public class DefaultSettingsLoader implements SettingsLoader {
-  static final String URL = "api/settings/values.protobuf";
+
   private ScannerWsClient wsClient;
   private static final Logger LOG = Loggers.get(DefaultSettingsLoader.class);
 
@@ -55,58 +50,23 @@ public class DefaultSettingsLoader implements SettingsLoader {
 
   @Override
   public Map<String, String> load(@Nullable String componentKey) {
+    String url = "api/settings/values.protobuf";
     Profiler profiler = Profiler.create(LOG);
-
-    try {
-      if (componentKey != null) {
-        profiler.startInfo("Load settings for component '" + componentKey + "'");
-        return loadProjectSettings(componentKey);
-      } else {
-        profiler.startInfo("Load global settings");
-        return loadSettings(URL);
-      }
-    } catch (IOException e) {
-      throw new IllegalStateException("Failed to load server settings", e);
-    } finally {
-      profiler.stopInfo();
+    if (componentKey != null) {
+      url += "?component=" + ScannerUtils.encodeForUrl(componentKey);
+      profiler.startInfo("Load settings for component '" + componentKey + "'");
+    } else {
+      profiler.startInfo("Load global settings");
     }
-  }
-
-  private Map<String, String> loadSettings(String url) throws IOException {
     try (InputStream is = wsClient.call(new GetRequest(url)).contentStream()) {
       ValuesWsResponse values = ValuesWsResponse.parseFrom(is);
+      profiler.stopInfo();
       return toMap(values.getSettingsList());
+    } catch (IOException e) {
+      throw new IllegalStateException("Failed to load server settings", e);
     }
   }
 
-  private Map<String, String> loadProjectSettings(String componentKey) throws IOException {
-    String url = URL + "?component=" + ScannerUtils.encodeForUrl(componentKey);
-    try {
-      return loadSettings(url);
-    } catch (RuntimeException e) {
-      if (shouldThrow(e)) {
-        throw e;
-      }
-
-      LOG.debug("Project settings not available - continuing without it");
-    }
-    return Collections.emptyMap();
-  }
-
-  private static boolean shouldThrow(Exception e) {
-    for (Throwable t : Throwables.getCausalChain(e)) {
-      if (t instanceof HttpException) {
-        HttpException http = (HttpException) t;
-        return http.code() != HttpURLConnection.HTTP_NOT_FOUND;
-      }
-      if (t instanceof MessageException) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
   @VisibleForTesting
   static Map<String, String> toMap(List<Setting> settingsList) {
     Map<String, String> result = new LinkedHashMap<>();
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectSettingsProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectSettingsProvider.java
index 9029c3ed486..fb303ed7d2b 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectSettingsProvider.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectSettingsProvider.java
@@ -25,18 +25,18 @@ import org.picocontainer.injectors.ProviderAdapter;
 import org.sonar.api.batch.bootstrap.ProjectReactor;
 import org.sonar.scanner.bootstrap.GlobalAnalysisMode;
 import org.sonar.scanner.bootstrap.GlobalConfiguration;
-import org.sonar.scanner.repository.settings.SettingsLoader;
+import org.sonar.scanner.repository.ProjectRepositories;
 
 public class ProjectSettingsProvider extends ProviderAdapter {
 
   private ProjectSettings projectSettings;
 
-  public ProjectSettings provide(ProjectReactor reactor, GlobalConfiguration globalSettings, SettingsLoader settingsLoader, GlobalAnalysisMode mode) {
+  public ProjectSettings provide(ProjectReactor reactor, GlobalConfiguration globalSettings, ProjectRepositories projectRepositories, GlobalAnalysisMode mode) {
     if (projectSettings == null) {
 
       Map<String, String> settings = new LinkedHashMap<>();
       settings.putAll(globalSettings.getProperties());
-      settings.putAll(settingsLoader.load(reactor.getRoot().getKeyWithBranch()));
+      settings.putAll(projectRepositories.settings(reactor.getRoot().getKeyWithBranch()));
       settings.putAll(reactor.getRoot().properties());
 
       projectSettings = new ProjectSettings(globalSettings.getDefinitions(), globalSettings.getEncryption(), mode, settings);
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationLoader.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationLoader.java
index ba0e0cd0f8a..94bb612a3b4 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationLoader.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationLoader.java
@@ -19,12 +19,13 @@
  */
 package org.sonar.scanner.scan.branch;
 
+import java.util.Map;
+import java.util.function.Supplier;
 import org.sonar.api.batch.InstantiationStrategy;
 import org.sonar.api.batch.ScannerSide;
-import org.sonar.scanner.scan.ProjectSettings;
 
 @ScannerSide
 @InstantiationStrategy(InstantiationStrategy.PER_BATCH)
 public interface BranchConfigurationLoader {
-  BranchConfiguration load(ProjectSettings projectSettings, ProjectBranches branches);
+  BranchConfiguration load(Map<String, String> localSettings, Supplier<Map<String, String>> remoteSettingsSupplier, ProjectBranches branches);
 }
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationProvider.java
index aac1661959d..505cbb51293 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationProvider.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/branch/BranchConfigurationProvider.java
@@ -19,12 +19,17 @@
  */
 package org.sonar.scanner.scan.branch;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Supplier;
 import org.picocontainer.annotations.Nullable;
 import org.picocontainer.injectors.ProviderAdapter;
+import org.sonar.api.batch.bootstrap.ProjectKey;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
 import org.sonar.api.utils.log.Profiler;
-import org.sonar.scanner.scan.ProjectSettings;
+import org.sonar.scanner.bootstrap.GlobalConfiguration;
+import org.sonar.scanner.repository.settings.SettingsLoader;
 
 public class BranchConfigurationProvider extends ProviderAdapter {
 
@@ -33,16 +38,27 @@ public class BranchConfigurationProvider extends ProviderAdapter {
 
   private BranchConfiguration branchConfiguration = null;
 
-  public BranchConfiguration provide(@Nullable BranchConfigurationLoader loader, ProjectSettings projectSettings, ProjectBranches branches) {
+  public BranchConfiguration provide(@Nullable BranchConfigurationLoader loader, GlobalConfiguration globalConfiguration, ProjectKey projectKey,
+    SettingsLoader settingsLoader, ProjectBranches branches) {
     if (branchConfiguration == null) {
       if (loader == null) {
         branchConfiguration = new DefaultBranchConfiguration();
       } else {
         Profiler profiler = Profiler.create(LOG).startInfo(LOG_MSG);
-        branchConfiguration = loader.load(projectSettings, branches);
+        Supplier<Map<String, String>> settingsSupplier = createSettingsSupplier(globalConfiguration, projectKey, settingsLoader);
+        branchConfiguration = loader.load(globalConfiguration.getProperties(), settingsSupplier, branches);
         profiler.stopInfo();
       }
     }
     return branchConfiguration;
   }
+
+  private Supplier<Map<String, String>> createSettingsSupplier(GlobalConfiguration globalConfiguration, ProjectKey projectKey, SettingsLoader settingsLoader) {
+    return () -> {
+      Map<String, String> settings = new HashMap<>();
+      settings.putAll(globalConfiguration.getProperties());
+      settings.putAll(settingsLoader.load(projectKey.get()));
+      return settings;
+    };
+  }
 }
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/mediumtest/ScannerMediumTester.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/mediumtest/ScannerMediumTester.java
index ffd845e3479..430ab5dc62a 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/mediumtest/ScannerMediumTester.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/mediumtest/ScannerMediumTester.java
@@ -37,6 +37,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.apache.commons.io.FileUtils;
@@ -67,7 +68,6 @@ import org.sonar.scanner.repository.settings.SettingsLoader;
 import org.sonar.scanner.rule.ActiveRulesLoader;
 import org.sonar.scanner.rule.LoadedActiveRule;
 import org.sonar.scanner.rule.RulesLoader;
-import org.sonar.scanner.scan.ProjectSettings;
 import org.sonar.scanner.scan.branch.BranchConfiguration;
 import org.sonar.scanner.scan.branch.BranchConfigurationLoader;
 import org.sonar.scanner.scan.branch.BranchType;
@@ -439,7 +439,7 @@ public class ScannerMediumTester extends ExternalResource {
 
   private class FakeBranchConfigurationLoader implements BranchConfigurationLoader {
     @Override
-    public BranchConfiguration load(ProjectSettings settings, ProjectBranches branches) {
+    public BranchConfiguration load(Map<String, String> localSettings, Supplier<Map<String, String>> settingsSupplier, ProjectBranches branches) {
       return branchConfiguration;
     }
   }
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java
index 2171a2b8809..f2ecbe4d5fb 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java
@@ -19,11 +19,7 @@
  */
 package org.sonar.scanner.repository.settings;
 
-import java.io.IOException;
-import org.junit.Before;
 import org.junit.Test;
-import org.sonar.scanner.WsTestUtil;
-import org.sonar.scanner.bootstrap.ScannerWsClient;
 import org.sonarqube.ws.Settings.FieldValues;
 import org.sonarqube.ws.Settings.FieldValues.Value;
 import org.sonarqube.ws.Settings.FieldValues.Value.Builder;
@@ -33,17 +29,8 @@ import org.sonarqube.ws.Settings.Values;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
-import static org.mockito.Mockito.mock;
 
 public class DefaultSettingsLoaderTest {
-  private ScannerWsClient wsClient;
-  private DefaultSettingsLoader loader;
-
-  @Before
-  public void prepare() throws IOException {
-    wsClient = mock(ScannerWsClient.class);
-    loader = new DefaultSettingsLoader(wsClient);
-  }
 
   @Test
   public void should_load_global_multivalue_settings() {
@@ -82,10 +69,4 @@ public class DefaultSettingsLoaderTest {
           entry("sonar.issue.exclusions.multicriteria.2.rulepattern", "*:S456"));
   }
 
-  @Test
-  public void continue_on_error() {
-    WsTestUtil.mockException(wsClient, DefaultSettingsLoader.URL + "?component=project", new IllegalStateException());
-    assertThat(loader.load("project")).isEmpty();
-  }
-
 }
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/branch/BranchConfigurationProviderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/branch/BranchConfigurationProviderTest.java
index 7201374fa78..7f6210e28f8 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/branch/BranchConfigurationProviderTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/branch/BranchConfigurationProviderTest.java
@@ -19,46 +19,63 @@
  */
 package org.sonar.scanner.scan.branch;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Supplier;
 import org.junit.Before;
 import org.junit.Test;
-import org.sonar.scanner.scan.ProjectSettings;
+import org.sonar.api.batch.bootstrap.ProjectKey;
+import org.sonar.scanner.bootstrap.GlobalConfiguration;
+import org.sonar.scanner.repository.settings.SettingsLoader;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 public class BranchConfigurationProviderTest {
   private BranchConfigurationProvider provider = new BranchConfigurationProvider();
-  private ProjectSettings projectSettings;
+  private GlobalConfiguration globalConfiguration;
   private BranchConfigurationLoader loader;
   private BranchConfiguration config;
   private ProjectBranches branches;
+  private ProjectKey projectKey;
+  private Map<String, String> globalPropertiesMap;
+  private Map<String, String> remoteProjectSettings;
+  private SettingsLoader settingsLoader;
 
   @Before
   public void setUp() {
-    projectSettings = mock(ProjectSettings.class);
+    globalConfiguration = mock(GlobalConfiguration.class);
     loader = mock(BranchConfigurationLoader.class);
     config = mock(BranchConfiguration.class);
     branches = mock(ProjectBranches.class);
+    settingsLoader = mock(SettingsLoader.class);
+    projectKey = mock(ProjectKey.class);
+    globalPropertiesMap = new HashMap<>();
+    when(globalConfiguration.getProperties()).thenReturn(globalPropertiesMap);
+    when(settingsLoader.load(anyString())).thenReturn(remoteProjectSettings);
   }
 
   @Test
   public void should_cache_config() {
-    BranchConfiguration configuration = provider.provide(null, projectSettings, branches);
-    assertThat(provider.provide(null, projectSettings, branches)).isSameAs(configuration);
+    BranchConfiguration configuration = provider.provide(null, globalConfiguration, projectKey, settingsLoader, branches);
+    assertThat(provider.provide(null, globalConfiguration, projectKey, settingsLoader, branches)).isSameAs(configuration);
   }
 
   @Test
   public void should_use_loader() {
-    when(loader.load(projectSettings, branches)).thenReturn(config);
-    BranchConfiguration branchConfig = provider.provide(loader, projectSettings, branches);
+    when(loader.load(eq(globalPropertiesMap), any(Supplier.class), eq(branches))).thenReturn(config);
+    BranchConfiguration branchConfig = provider.provide(loader, globalConfiguration, projectKey, settingsLoader, branches);
 
     assertThat(branchConfig).isSameAs(config);
   }
 
   @Test
   public void should_return_default_if_no_loader() {
-    BranchConfiguration configuration = provider.provide(null, projectSettings, branches);
+    BranchConfiguration configuration = provider.provide(null, globalConfiguration, projectKey, settingsLoader, branches);
     assertThat(configuration.branchTarget()).isNull();
     assertThat(configuration.branchType()).isEqualTo(BranchType.LONG);
   }