SONAR-13191 Fail analysis when project is using an invalid key

2 files changed, 49 insertions, 71 deletions

diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectReactorValidator.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectReactorValidator.java
index 9b50bb48a73..e009b3b1002 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectReactorValidator.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/ProjectReactorValidator.java
@@ -26,14 +26,11 @@ import javax.annotation.Nullable;
 import org.sonar.api.batch.bootstrap.ProjectDefinition;
 import org.sonar.api.batch.bootstrap.ProjectReactor;
 import org.sonar.api.utils.MessageException;
-import org.sonar.api.utils.log.Logger;
-import org.sonar.api.utils.log.Loggers;
 import org.sonar.core.component.ComponentKeys;
 import org.sonar.scanner.bootstrap.GlobalConfiguration;
 import org.sonar.scanner.scan.branch.BranchParamsValidator;
 
 import static java.lang.String.format;
-import static java.util.Collections.singletonList;
 import static java.util.Objects.nonNull;
 import static org.apache.commons.lang.StringUtils.isNotEmpty;
 import static org.sonar.core.component.ComponentKeys.ALLOWED_CHARACTERS_MESSAGE;
@@ -49,9 +46,6 @@ import static org.sonar.core.config.ScannerProperties.PULL_REQUEST_KEY;
  * @since 3.6
  */
 public class ProjectReactorValidator {
-
-  private static final Logger LOG = Loggers.get(ProjectReactorValidator.class);
-
   private final GlobalConfiguration settings;
 
   // null = branch plugin is not available
@@ -71,7 +65,7 @@ public class ProjectReactorValidator {
     List<String> validationMessages = new ArrayList<>();
 
     for (ProjectDefinition moduleDef : reactor.getProjects()) {
-      validateModule(moduleDef);
+      validateModule(moduleDef, validationMessages);
     }
 
     if (isBranchFeatureAvailable()) {
@@ -82,17 +76,15 @@ public class ProjectReactorValidator {
     }
 
     if (!validationMessages.isEmpty()) {
-      throw MessageException.of("Validation of project reactor failed:\n  o " +
+      throw MessageException.of("Validation of project failed:\n  o " +
         String.join("\n  o ", validationMessages));
     }
   }
 
   private void validateBranchParamsWhenPluginAbsent(List<String> validationMessages) {
-    for (String param : singletonList(BRANCH_NAME)) {
-      if (isNotEmpty(settings.get(param).orElse(null))) {
-        validationMessages.add(format("To use the property \"%s\" and analyze branches, Developer Edition or above is required. "
-          + "See %s for more information.", param, BRANCHES_DOC_LINK));
-      }
+    if (isNotEmpty(settings.get(BRANCH_NAME).orElse(null))) {
+      validationMessages.add(format("To use the property \"%s\" and analyze branches, Developer Edition or above is required. "
+        + "See %s for more information.", BRANCH_NAME, BRANCHES_DOC_LINK));
     }
   }
 
@@ -103,12 +95,9 @@ public class ProjectReactorValidator {
         + "See %s for more information.", param, BRANCHES_DOC_LINK)));
   }
 
-  private static void validateModule(ProjectDefinition moduleDef) {
-    if (!ComponentKeys.isValidProjectKey(moduleDef.getKey())) {
-      // As it was possible in the past to use project key with a format that is no more compatible, we need to display a warning to the user in
-      // order for him to update his project key.
-      // SONAR-13191 This warning should be removed in 9.0
-      LOG.warn("\"{}\" is not a valid project or module key. {}.", moduleDef.getKey(), ALLOWED_CHARACTERS_MESSAGE);
+  private static void validateModule(ProjectDefinition projectDefinition, List<String> validationMessages) {
+    if (!ComponentKeys.isValidProjectKey(projectDefinition.getKey())) {
+      validationMessages.add(format("\"%s\" is not a valid project key. %s.", projectDefinition.getKey(), ALLOWED_CHARACTERS_MESSAGE));
     }
   }
 
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorValidatorTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorValidatorTest.java
index 5c984860484..1bdbac554a2 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorValidatorTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorValidatorTest.java
@@ -28,23 +28,19 @@ import java.util.function.Consumer;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.batch.bootstrap.ProjectDefinition;
 import org.sonar.api.batch.bootstrap.ProjectReactor;
 import org.sonar.api.utils.MessageException;
-import org.sonar.api.utils.log.LogAndArguments;
 import org.sonar.api.utils.log.LogTester;
-import org.sonar.api.utils.log.LoggerLevel;
 import org.sonar.core.config.ScannerProperties;
 import org.sonar.scanner.ProjectInfo;
 import org.sonar.scanner.bootstrap.GlobalConfiguration;
 
 import static org.apache.commons.lang.RandomStringUtils.randomAscii;
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -52,14 +48,11 @@ import static org.mockito.Mockito.when;
 public class ProjectReactorValidatorTest {
 
   @Rule
-  public ExpectedException thrown = ExpectedException.none();
-
-  @Rule
   public LogTester logTester = new LogTester();
 
-  private GlobalConfiguration settings = mock(GlobalConfiguration.class);
-  private ProjectInfo projectInfo = mock(ProjectInfo.class);
-  private ProjectReactorValidator underTest = new ProjectReactorValidator(settings);
+  private final GlobalConfiguration settings = mock(GlobalConfiguration.class);
+  private final ProjectInfo projectInfo = mock(ProjectInfo.class);
+  private final ProjectReactorValidator underTest = new ProjectReactorValidator(settings);
 
   @Before
   public void prepare() {
@@ -69,7 +62,8 @@ public class ProjectReactorValidatorTest {
   @Test
   @UseDataProvider("validKeys")
   public void not_fail_with_valid_key(String validKey) {
-    underTest.validate(createProjectReactor(validKey));
+    ProjectReactor projectReactor = createProjectReactor(validKey);
+    underTest.validate(projectReactor);
   }
 
   @DataProvider
@@ -93,36 +87,33 @@ public class ProjectReactorValidatorTest {
   }
 
   @Test
-  public void log_warning_when_invalid_key() {
+  public void failg_when_invalid_key() {
     ProjectReactor reactor = createProjectReactor("foo$bar");
 
-    underTest.validate(reactor);
-
-    assertThat(logTester.getLogs(LoggerLevel.WARN))
-      .extracting(LogAndArguments::getFormattedMsg)
-      .containsOnly("\"foo$bar\" is not a valid project or module key. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit.");
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("\"foo$bar\" is not a valid project key. Allowed characters are alphanumeric,"
+        + " '-', '_', '.' and ':', with at least one non-digit.");
   }
 
   @Test
-  public void log_warning_when_only_digits() {
+  public void fail_when_only_digits() {
     ProjectReactor reactor = createProjectReactor("12345");
 
-    underTest.validate(reactor);
-
-    assertThat(logTester.getLogs(LoggerLevel.WARN))
-      .extracting(LogAndArguments::getFormattedMsg)
-      .containsOnly("\"12345\" is not a valid project or module key. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit.");
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("\"12345\" is not a valid project key. Allowed characters are alphanumeric, "
+        + "'-', '_', '.' and ':', with at least one non-digit.");
   }
 
   @Test
-  public void log_warning_when_backslash_in_key() {
+  public void fail_when_backslash_in_key() {
     ProjectReactor reactor = createProjectReactor("foo\\bar");
 
-    underTest.validate(reactor);
-
-    assertThat(logTester.getLogs(LoggerLevel.WARN))
-      .extracting(LogAndArguments::getFormattedMsg)
-      .containsOnly("\"foo\\bar\" is not a valid project or module key. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit.");
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("\"foo\\bar\" is not a valid project key. Allowed characters are alphanumeric, "
+        + "'-', '_', '.' and ':', with at least one non-digit.");
   }
 
   @Test
@@ -130,12 +121,11 @@ public class ProjectReactorValidatorTest {
     ProjectDefinition def = ProjectDefinition.create().setProperty(CoreProperties.PROJECT_KEY_PROPERTY, "foo");
     ProjectReactor reactor = new ProjectReactor(def);
 
-    when(settings.get(eq(ScannerProperties.BRANCH_NAME))).thenReturn(Optional.of("feature1"));
+    when(settings.get(ScannerProperties.BRANCH_NAME)).thenReturn(Optional.of("feature1"));
 
-    thrown.expect(MessageException.class);
-    thrown.expectMessage("To use the property \"sonar.branch.name\" and analyze branches, Developer Edition or above is required");
-
-    underTest.validate(reactor);
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("To use the property \"sonar.branch.name\" and analyze branches, Developer Edition or above is required");
   }
 
   @Test
@@ -143,12 +133,11 @@ public class ProjectReactorValidatorTest {
     ProjectDefinition def = ProjectDefinition.create().setProperty(CoreProperties.PROJECT_KEY_PROPERTY, "foo");
     ProjectReactor reactor = new ProjectReactor(def);
 
-    when(settings.get(eq(ScannerProperties.PULL_REQUEST_KEY))).thenReturn(Optional.of("#1984"));
-
-    thrown.expect(MessageException.class);
-    thrown.expectMessage("To use the property \"sonar.pullrequest.key\" and analyze pull requests, Developer Edition or above is required");
+    when(settings.get(ScannerProperties.PULL_REQUEST_KEY)).thenReturn(Optional.of("#1984"));
 
-    underTest.validate(reactor);
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("To use the property \"sonar.pullrequest.key\" and analyze pull requests, Developer Edition or above is required");
   }
 
   @Test
@@ -156,12 +145,11 @@ public class ProjectReactorValidatorTest {
     ProjectDefinition def = ProjectDefinition.create().setProperty(CoreProperties.PROJECT_KEY_PROPERTY, "foo");
     ProjectReactor reactor = new ProjectReactor(def);
 
-    when(settings.get(eq(ScannerProperties.PULL_REQUEST_BRANCH))).thenReturn(Optional.of("feature1"));
+    when(settings.get(ScannerProperties.PULL_REQUEST_BRANCH)).thenReturn(Optional.of("feature1"));
 
-    thrown.expect(MessageException.class);
-    thrown.expectMessage("To use the property \"sonar.pullrequest.branch\" and analyze pull requests, Developer Edition or above is required");
-
-    underTest.validate(reactor);
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("To use the property \"sonar.pullrequest.branch\" and analyze pull requests, Developer Edition or above is required");
   }
 
   @Test
@@ -169,12 +157,11 @@ public class ProjectReactorValidatorTest {
     ProjectDefinition def = ProjectDefinition.create().setProperty(CoreProperties.PROJECT_KEY_PROPERTY, "foo");
     ProjectReactor reactor = new ProjectReactor(def);
 
-    when(settings.get(eq(ScannerProperties.PULL_REQUEST_BASE))).thenReturn(Optional.of("feature1"));
-
-    thrown.expect(MessageException.class);
-    thrown.expectMessage("To use the property \"sonar.pullrequest.base\" and analyze pull requests, Developer Edition or above is required");
+    when(settings.get(ScannerProperties.PULL_REQUEST_BASE)).thenReturn(Optional.of("feature1"));
 
-    underTest.validate(reactor);
+    assertThatThrownBy(() -> underTest.validate(reactor))
+      .isInstanceOf(MessageException.class)
+      .hasMessageContaining("To use the property \"sonar.pullrequest.base\" and analyze pull requests, Developer Edition or above is required");
   }
 
   @Test
@@ -182,7 +169,8 @@ public class ProjectReactorValidatorTest {
   public void not_fail_with_valid_version(String validVersion) {
     when(projectInfo.getProjectVersion()).thenReturn(Optional.ofNullable(validVersion));
 
-    underTest.validate(createProjectReactor("foo"));
+    ProjectReactor projectReactor = createProjectReactor("foo");
+    underTest.validate(projectReactor);
   }
 
   @DataProvider
@@ -200,7 +188,8 @@ public class ProjectReactorValidatorTest {
   public void not_fail_with_valid_buildString(String validBuildString) {
     when(projectInfo.getBuildString()).thenReturn(Optional.ofNullable(validBuildString));
 
-    underTest.validate(createProjectReactor("foo"));
+    ProjectReactor projectReactor = createProjectReactor("foo");
+    underTest.validate(projectReactor);
   }
 
   @DataProvider