SONAR-8335 better error reporting on property values and UT coverage

6 files changed, 195 insertions, 32 deletions

diff --git a/server/sonar-process/src/main/java/org/sonar/process/LogbackHelper.java b/server/sonar-process/src/main/java/org/sonar/process/LogbackHelper.java
index 7a86fe5dd4a..6e513626d8f 100644
--- a/server/sonar-process/src/main/java/org/sonar/process/LogbackHelper.java
+++ b/server/sonar-process/src/main/java/org/sonar/process/LogbackHelper.java
@@ -37,13 +37,13 @@ import ch.qos.logback.core.rolling.SizeBasedTriggeringPolicy;
 import ch.qos.logback.core.rolling.TimeBasedRollingPolicy;
 import java.io.File;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.Set;
 import javax.annotation.CheckForNull;
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.LoggerFactory;
 
-import static java.util.Collections.unmodifiableSet;
 import static java.util.Objects.requireNonNull;
 import static org.slf4j.Logger.ROOT_LOGGER_NAME;
 
@@ -60,7 +60,11 @@ public class LogbackHelper {
   private static final String ROLLING_POLICY_PROPERTY = "sonar.log.rollingPolicy";
   private static final String MAX_FILES_PROPERTY = "sonar.log.maxFiles";
   private static final String LOG_FORMAT = "%d{yyyy.MM.dd HH:mm:ss} %-5level " + PROCESS_NAME_PLACEHOLDER + "[" + THREAD_ID_PLACEHOLDER + "][%logger{20}] %msg%n";
-  public static final Set<Level> ALLOWED_ROOT_LOG_LEVELS = unmodifiableSet(setOf(Level.TRACE, Level.DEBUG, Level.INFO));
+  private static final Level[] ALLOWED_ROOT_LOG_LEVELS = new Level[] {Level.TRACE, Level.DEBUG, Level.INFO};
+
+  public static Collection<Level> allowedLogLevels() {
+    return Arrays.asList(ALLOWED_ROOT_LOG_LEVELS);
+  }
 
   public LoggerContext getRootContext() {
     org.slf4j.Logger logger;
@@ -165,8 +169,8 @@ public class LogbackHelper {
 
   public String buildLogPattern(LogbackHelper.RootLoggerConfig config) {
     return LOG_FORMAT
-        .replace(PROCESS_NAME_PLACEHOLDER, config.getProcessName())
-        .replace(THREAD_ID_PLACEHOLDER, config.getThreadIdFieldPattern());
+      .replace(PROCESS_NAME_PLACEHOLDER, config.getProcessName())
+      .replace(THREAD_ID_PLACEHOLDER, config.getThreadIdFieldPattern());
   }
 
   /**
@@ -257,8 +261,8 @@ public class LogbackHelper {
    */
   public Level configureRootLogLevel(Props props, ProcessId processId) {
     return configureRootLogLevel(props,
-        SONAR_LOG_LEVEL_PROPERTY,
-        SONAR_PROCESS_LOG_LEVEL_PROPERTY.replace(PROCESS_NAME_PLACEHOLDER, processId.getKey()));
+      SONAR_LOG_LEVEL_PROPERTY,
+      SONAR_PROCESS_LOG_LEVEL_PROPERTY.replace(PROCESS_NAME_PLACEHOLDER, processId.getKey()));
   }
 
   /**
@@ -268,14 +272,37 @@ public class LogbackHelper {
    * @throws IllegalArgumentException if the value of the specified property is not one of {@link #ALLOWED_ROOT_LOG_LEVELS}
    */
   public Level configureRootLogLevel(Props props, String... propertyKeys) {
+    return configureLoggerLogLevel(getRootContext().getLogger(ROOT_LOGGER_NAME), props, propertyKeys);
+  }
+
+  private Level configureLoggerLogLevel(Logger logger, Props props, String... propertyKeys) {
     Level newLevel = Level.INFO;
     for (String propertyKey : propertyKeys) {
-      Level level = Level.toLevel(props.value(propertyKey, Level.INFO.toString()), Level.INFO);
+      Level level = getPropertyValueAsLevel(props, propertyKey);
       if (!level.isGreaterOrEqual(newLevel)) {
         newLevel = level;
       }
     }
-    return configureRootLogLevel(newLevel);
+    logger.setLevel(newLevel);
+    return newLevel;
+  }
+
+  private static Level getPropertyValueAsLevel(Props props, String propertyKey) {
+    Level level = Level.toLevel(props.value(propertyKey, Level.INFO.toString()), Level.INFO);
+    if (!isAllowed(level)) {
+      throw new IllegalArgumentException(String.format("log level %s in property %s is not a supported value (allowed levels are %s)",
+        level, propertyKey, Arrays.toString(ALLOWED_ROOT_LOG_LEVELS)));
+    }
+    return level;
+  }
+
+  private static boolean isAllowed(Level level) {
+    for (Level allowedRootLogLevel : ALLOWED_ROOT_LOG_LEVELS) {
+      if (level == allowedRootLogLevel) {
+        return true;
+      }
+    }
+    return false;
   }
 
   /**
@@ -285,13 +312,17 @@ public class LogbackHelper {
    */
   public Level configureRootLogLevel(Level newLevel) {
     Logger rootLogger = getRootContext().getLogger(ROOT_LOGGER_NAME);
-    if (!ALLOWED_ROOT_LOG_LEVELS.contains(newLevel)) {
-      throw new IllegalArgumentException(String.format("%s log level is not supported (allowed levels are %s)", newLevel, ALLOWED_ROOT_LOG_LEVELS));
-    }
+    ensureSupportedLevel(newLevel);
     rootLogger.setLevel(newLevel);
     return newLevel;
   }
 
+  private static void ensureSupportedLevel(Level newLevel) {
+    if (!isAllowed(newLevel)) {
+      throw new IllegalArgumentException(String.format("%s log level is not supported (allowed levels are %s)", newLevel, Arrays.toString(ALLOWED_ROOT_LOG_LEVELS)));
+    }
+  }
+
   /**
    * Configure the log level of the specified logger to specified level.
    * <p>
diff --git a/server/sonar-search/src/main/java/org/sonar/search/SearchLogging.java b/server/sonar-search/src/main/java/org/sonar/search/SearchLogging.java
index 5a35bd69c65..a7167c9b33e 100644
--- a/server/sonar-search/src/main/java/org/sonar/search/SearchLogging.java
+++ b/server/sonar-search/src/main/java/org/sonar/search/SearchLogging.java
@@ -39,7 +39,7 @@ public class SearchLogging {
     helper.configureGlobalFileLog(props, config, logPattern);
     helper.configureForSubprocessGobbler(props, logPattern);
     // SQ's global log level must not change ES's log level
-    helper.configureRootLogLevel(props, "sonar.log.es.level");
+    helper.configureRootLogLevel(props, "sonar.log.level.es");
 
     return ctx;
   }
diff --git a/server/sonar-search/src/test/java/org/sonar/search/SearchLoggingTest.java b/server/sonar-search/src/test/java/org/sonar/search/SearchLoggingTest.java
index cd4533691b9..b7f2fa60afd 100644
--- a/server/sonar-search/src/test/java/org/sonar/search/SearchLoggingTest.java
+++ b/server/sonar-search/src/test/java/org/sonar/search/SearchLoggingTest.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.search;
 
+import ch.qos.logback.classic.Level;
 import ch.qos.logback.classic.Logger;
 import ch.qos.logback.classic.LoggerContext;
 import ch.qos.logback.classic.encoder.PatternLayoutEncoder;
@@ -38,6 +39,7 @@ import org.sonar.process.ProcessProperties;
 import org.sonar.process.Props;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.slf4j.Logger.ROOT_LOGGER_NAME;
 
 public class SearchLoggingTest {
   @Rule
@@ -63,7 +65,7 @@ public class SearchLoggingTest {
   public void do_not_log_to_console() {
     LoggerContext ctx = underTest.configure(props);
 
-    Logger root = ctx.getLogger(Logger.ROOT_LOGGER_NAME);
+    Logger root = ctx.getLogger(ROOT_LOGGER_NAME);
     Appender appender = root.getAppender("CONSOLE");
     assertThat(appender).isNull();
   }
@@ -72,7 +74,7 @@ public class SearchLoggingTest {
   public void log_to_es_file() {
     LoggerContext ctx = underTest.configure(props);
 
-    Logger root = ctx.getLogger(Logger.ROOT_LOGGER_NAME);
+    Logger root = ctx.getLogger(ROOT_LOGGER_NAME);
     Appender<ILoggingEvent> appender = root.getAppender("file_es");
     assertThat(appender).isInstanceOf(FileAppender.class);
     FileAppender fileAppender = (FileAppender) appender;
@@ -81,4 +83,24 @@ public class SearchLoggingTest {
     PatternLayoutEncoder encoder = (PatternLayoutEncoder) fileAppender.getEncoder();
     assertThat(encoder.getPattern()).isEqualTo("%d{yyyy.MM.dd HH:mm:ss} %-5level es[][%logger{20}] %msg%n");
   }
+
+  @Test
+  public void root_log_level_does_not_change_with_global_property_value() {
+    props.set("sonar.log.level", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    Logger rootLogger = ctx.getLogger(ROOT_LOGGER_NAME);
+    assertThat(rootLogger.getLevel()).isEqualTo(Level.INFO);
+  }
+
+  @Test
+  public void root_log_level_change_with_es_property_value() {
+    props.set("sonar.log.level.es", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    Logger rootLogger = ctx.getLogger(ROOT_LOGGER_NAME);
+    assertThat(rootLogger.getLevel()).isEqualTo(Level.TRACE);
+  }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/ChangeLogLevelAction.java b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/ChangeLogLevelAction.java
index e64eac0db8f..74917769dab 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/ChangeLogLevelAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/ChangeLogLevelAction.java
@@ -28,7 +28,7 @@ import org.sonar.db.Database;
 import org.sonar.server.platform.ServerLogging;
 import org.sonar.server.user.UserSession;
 
-import static org.sonar.process.LogbackHelper.ALLOWED_ROOT_LOG_LEVELS;
+import static org.sonar.process.LogbackHelper.allowedLogLevels;
 
 public class ChangeLogLevelAction implements SystemWsAction {
 
@@ -57,7 +57,7 @@ public class ChangeLogLevelAction implements SystemWsAction {
 
     newAction.createParam(PARAM_LEVEL)
       .setDescription("The new level. Be cautious: DEBUG, and even more TRACE, may have performance impacts.")
-      .setPossibleValues(ALLOWED_ROOT_LOG_LEVELS)
+      .setPossibleValues(allowedLogLevels())
       .setRequired(true);
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/ce/log/CeProcessLoggingTest.java b/server/sonar-server/src/test/java/org/sonar/ce/log/CeProcessLoggingTest.java
index 6b2dbfd704d..8d2999e23e6 100644
--- a/server/sonar-server/src/test/java/org/sonar/ce/log/CeProcessLoggingTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/ce/log/CeProcessLoggingTest.java
@@ -34,19 +34,21 @@ import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.sonar.process.LogbackHelper;
 import org.sonar.process.ProcessProperties;
 import org.sonar.process.Props;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.slf4j.Logger.ROOT_LOGGER_NAME;
 
 public class CeProcessLoggingTest {
 
-  private static final String LOG_LEVEL_PROPERTY = "sonar.log.level";
-
   @Rule
   public TemporaryFolder temp = new TemporaryFolder();
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   private File logDir;
   private Props props = new Props(new Properties());
@@ -87,16 +89,69 @@ public class CeProcessLoggingTest {
   }
 
   @Test
-  public void enable_debug_logs() {
-    props.set(LOG_LEVEL_PROPERTY, "DEBUG");
+  public void default_level_for_root_logger_is_INFO() {
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.INFO);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_global_property() {
+    props.set("sonar.log.level", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.TRACE);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_app_property() {
+    props.set("sonar.log.level.ce", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.TRACE);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_app_property_and_is_case_insensitive() {
+    props.set("sonar.log.level.ce", "debug");
+
     LoggerContext ctx = underTest.configure(props);
-    assertThat(ctx.getLogger(Logger.ROOT_LOGGER_NAME).getLevel()).isEqualTo(Level.DEBUG);
+
+    verifyRootLogLevel(ctx, Level.DEBUG);
   }
 
   @Test
-  public void enable_trace_logs() {
-    props.set(LOG_LEVEL_PROPERTY, "TRACE");
+  public void default_to_INFO_if_app_property_has_invalid_value() {
+    props.set("sonar.log.level.ce", "DodoDouh!");
+
     LoggerContext ctx = underTest.configure(props);
-    assertThat(ctx.getLogger(Logger.ROOT_LOGGER_NAME).getLevel()).isEqualTo(Level.TRACE);
+    verifyRootLogLevel(ctx, Level.INFO);
+  }
+
+  @Test
+  public void fail_with_IAE_if_global_property_unsupported_level() {
+    props.set("sonar.log.level", "ERROR");
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("log level ERROR in property sonar.log.level is not a supported value (allowed levels are [TRACE, DEBUG, INFO])");
+
+    underTest.configure(props);
+  }
+
+  @Test
+  public void fail_with_IAE_if_app_property_unsupported_level() {
+    props.set("sonar.log.level.ce", "ERROR");
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("log level ERROR in property sonar.log.level.ce is not a supported value (allowed levels are [TRACE, DEBUG, INFO])");
+
+    underTest.configure(props);
+  }
+
+  private void verifyRootLogLevel(LoggerContext ctx, Level expected) {
+    Logger rootLogger = ctx.getLogger(ROOT_LOGGER_NAME);
+    assertThat(rootLogger.getLevel()).isEqualTo(expected);
   }
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/app/WebServerProcessLoggingTest.java b/server/sonar-server/src/test/java/org/sonar/server/app/WebServerProcessLoggingTest.java
index 0ab5bc96715..02e6127f882 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/app/WebServerProcessLoggingTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/app/WebServerProcessLoggingTest.java
@@ -34,19 +34,21 @@ import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.sonar.process.LogbackHelper;
 import org.sonar.process.ProcessProperties;
 import org.sonar.process.Props;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.slf4j.Logger.ROOT_LOGGER_NAME;
 
 public class WebServerProcessLoggingTest {
 
-  private static final String LOG_LEVEL_PROPERTY = "sonar.log.level";
-
   @Rule
   public TemporaryFolder temp = new TemporaryFolder();
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   private File logDir;
   private Props props = new Props(new Properties());
@@ -87,17 +89,70 @@ public class WebServerProcessLoggingTest {
   }
 
   @Test
-  public void enable_debug_logs() {
-    props.set(LOG_LEVEL_PROPERTY, "DEBUG");
+  public void default_level_for_root_logger_is_INFO() {
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.INFO);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_global_property() {
+    props.set("sonar.log.level", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.TRACE);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_app_property() {
+    props.set("sonar.log.level.web", "TRACE");
+
+    LoggerContext ctx = underTest.configure(props);
+
+    verifyRootLogLevel(ctx, Level.TRACE);
+  }
+
+  @Test
+  public void root_logger_level_changes_with_app_property_and_is_case_insensitive() {
+    props.set("sonar.log.level.web", "debug");
+
     LoggerContext ctx = underTest.configure(props);
-    assertThat(ctx.getLogger(Logger.ROOT_LOGGER_NAME).getLevel()).isEqualTo(Level.DEBUG);
+
+    verifyRootLogLevel(ctx, Level.DEBUG);
   }
 
   @Test
-  public void enable_trace_logs() {
-    props.set(LOG_LEVEL_PROPERTY, "TRACE");
+  public void default_to_INFO_if_app_property_has_invalid_value() {
+    props.set("sonar.log.level.web", "DodoDouh!");
+
     LoggerContext ctx = underTest.configure(props);
-    assertThat(ctx.getLogger(Logger.ROOT_LOGGER_NAME).getLevel()).isEqualTo(Level.TRACE);
+    verifyRootLogLevel(ctx, Level.INFO);
+  }
+
+  @Test
+  public void fail_with_IAE_if_global_property_unsupported_level() {
+    props.set("sonar.log.level", "ERROR");
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("log level ERROR in property sonar.log.level is not a supported value (allowed levels are [TRACE, DEBUG, INFO])");
+
+    underTest.configure(props);
+  }
+
+  @Test
+  public void fail_with_IAE_if_app_property_unsupported_level() {
+    props.set("sonar.log.level.web", "ERROR");
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("log level ERROR in property sonar.log.level.web is not a supported value (allowed levels are [TRACE, DEBUG, INFO])");
+
+    underTest.configure(props);
+  }
+
+  private void verifyRootLogLevel(LoggerContext ctx, Level expected) {
+    Logger rootLogger = ctx.getLogger(ROOT_LOGGER_NAME);
+    assertThat(rootLogger.getLevel()).isEqualTo(expected);
   }
 
 }