SONAR-6665 Fix StackOverflow error when analyzing project with several modules having same key

8 files changed, 70 insertions, 77 deletions

diff --git a/it/it-projects/batch/multi-module-repeated-names/modules/module1/module1/sources/Fake.java b/it/it-projects/batch/multi-module-repeated-names/modules/module1/module1/sources/Fake.java
deleted file mode 100644
index e67004defc5..00000000000
--- a/it/it-projects/batch/multi-module-repeated-names/modules/module1/module1/sources/Fake.java
+++ /dev/null
@@ -1 +0,0 @@
-class Fake {}
diff --git a/it/it-projects/batch/multi-module-repeated-names/modules/module1/sources/Fake.java b/it/it-projects/batch/multi-module-repeated-names/modules/module1/sources/Fake.java
deleted file mode 100644
index e67004defc5..00000000000
--- a/it/it-projects/batch/multi-module-repeated-names/modules/module1/sources/Fake.java
+++ /dev/null
@@ -1 +0,0 @@
-class Fake {}
diff --git a/it/it-projects/batch/multi-module-repeated-names/sonar-project.properties b/it/it-projects/batch/multi-module-repeated-names/sonar-project.properties
deleted file mode 100644
index b1816be030b..00000000000
--- a/it/it-projects/batch/multi-module-repeated-names/sonar-project.properties
+++ /dev/null
@@ -1,19 +0,0 @@
-sonar.projectKey=com.foo.project
-sonar.projectName=Foo Project
-sonar.projectVersion=1.0-SNAPSHOT
-sonar.projectDescription=Description of Foo Project
-
-sonar.sources=sources
-sonar.tests=tests
-sonar.binaries=target/classes
-
-sonar.modules=module1
-
-module1.sonar.projectBaseDir=modules/module1
-module1.sonar.projectKey=com.foo.project.module1
-module1.sonar.projectName=Foo Module 1
-module1.sonar.modules=module1
-
-module1.module1.sonar.projectBaseDir=module1
-module1.module1.sonar.projectKey=com.foo.project.module1.module1
-module1.module1.sonar.projectName=Foo Sub Module 1
diff --git a/it/it-tests/src/test/java/batch/suite/ProjectBuilderTest.java b/it/it-tests/src/test/java/batch/suite/ProjectBuilderTest.java
index 8dff64ce5fd..9728fc0a3d2 100644
--- a/it/it-tests/src/test/java/batch/suite/ProjectBuilderTest.java
+++ b/it/it-tests/src/test/java/batch/suite/ProjectBuilderTest.java
@@ -5,18 +5,15 @@
  */
 package batch.suite;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
-import com.sonar.orchestrator.build.BuildFailureException;
-
-import com.sonar.orchestrator.build.SonarRunner;
-import util.ItUtils;
 import com.sonar.orchestrator.Orchestrator;
 import com.sonar.orchestrator.build.MavenBuild;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.sonar.wsclient.services.Resource;
 import org.sonar.wsclient.services.ResourceQuery;
+import util.ItUtils;
+
+import static org.assertj.core.api.Assertions.assertThat;
 
 /**
  * Test the extension point org.sonar.api.batch.bootstrap.ProjectBuilder
@@ -47,18 +44,6 @@ public class ProjectBuilderTest {
     assertThat(getResource("com.sonarsource.it.projects.batch:project-builder-module-b:src/IgnoredFile.java")).isNull();
   }
 
-  @Test
-  // SONAR-6665
-  public void errorSubModuleSameName() {
-    SonarRunner build = SonarRunner.create(ItUtils.projectDir("batch/multi-module-repeated-names"));
-
-    try {
-      orchestrator.executeBuild(build);
-    } catch (BuildFailureException e) {
-      assertThat(e.getResult().getLogs()).contains("Two modules have the same id: module1");
-    }
-  }
-
   private void checkProject() {
     Resource project = getResource("com.sonarsource.it.projects.batch:project-builder");
 
diff --git a/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java b/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
index d25a5d0e33e..e3fe21f32f9 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
@@ -117,21 +117,22 @@ public class ProjectReactorBuilder {
 
   public ProjectReactor execute() {
     Profiler profiler = Profiler.create(LOG).startInfo("Process project properties");
-    Map<String, Map<String, String>> propertiesByModuleId = new HashMap<>();
-    extractPropertiesByModule(propertiesByModuleId, "", taskProps.properties());
-    ProjectDefinition rootProject = defineRootProject(propertiesByModuleId.get(""), null);
+    Map<String, Map<String, String>> propertiesByModuleIdPath = new HashMap<>();
+    extractPropertiesByModule(propertiesByModuleIdPath, "", "", taskProps.properties());
+    ProjectDefinition rootProject = defineRootProject(propertiesByModuleIdPath.get(""), null);
     rootProjectWorkDir = rootProject.getWorkDir();
-    defineChildren(rootProject, propertiesByModuleId);
+    defineChildren(rootProject, propertiesByModuleIdPath, "");
     cleanAndCheckProjectDefinitions(rootProject);
     // Since task properties are now empty we should add root module properties
-    taskProps.properties().putAll(propertiesByModuleId.get(""));
+    taskProps.properties().putAll(propertiesByModuleIdPath.get(""));
     profiler.stopDebug();
     return new ProjectReactor(rootProject);
   }
 
-  private static void extractPropertiesByModule(Map<String, Map<String, String>> propertiesByModuleId, String currentModuleId, Map<String, String> parentProperties) {
-    if (propertiesByModuleId.containsKey(currentModuleId)) {
-      throw new IllegalStateException(String.format("Two modules have the same id: %s. Each module must have a unique id.", currentModuleId));
+  private static void extractPropertiesByModule(Map<String, Map<String, String>> propertiesByModuleIdPath, String currentModuleId, String currentModuleIdPath,
+    Map<String, String> parentProperties) {
+    if (propertiesByModuleIdPath.containsKey(currentModuleIdPath)) {
+      throw new IllegalStateException(String.format("Two modules have the same id: '%s'. Each module must have a unique id.", currentModuleId));
     }
 
     Map<String, String> currentModuleProperties = new HashMap<>();
@@ -153,10 +154,11 @@ public class ProjectReactorBuilder {
     Arrays.sort(moduleIds);
     ArrayUtils.reverse(moduleIds);
 
-    propertiesByModuleId.put(currentModuleId, currentModuleProperties);
+    propertiesByModuleIdPath.put(currentModuleIdPath, currentModuleProperties);
 
     for (String moduleId : moduleIds) {
-      extractPropertiesByModule(propertiesByModuleId, moduleId, currentModuleProperties);
+      String subModuleIdPath = currentModuleIdPath.isEmpty() ? moduleId : (currentModuleIdPath + "." + moduleId);
+      extractPropertiesByModule(propertiesByModuleIdPath, moduleId, subModuleIdPath, currentModuleProperties);
     }
   }
 
@@ -232,16 +234,17 @@ public class ProjectReactorBuilder {
     return new File(moduleBaseDir, customBuildDir.getPath());
   }
 
-  private void defineChildren(ProjectDefinition parentProject, Map<String, Map<String, String>> propertiesByModuleId) {
+  private void defineChildren(ProjectDefinition parentProject, Map<String, Map<String, String>> propertiesByModuleIdPath, String parentModuleIdPath) {
     Map<String, String> parentProps = parentProject.properties();
     if (parentProps.containsKey(PROPERTY_MODULES)) {
       for (String moduleId : getListFromProperty(parentProps, PROPERTY_MODULES)) {
-        Map<String, String> moduleProps = propertiesByModuleId.get(moduleId);
+        String moduleIdPath = parentModuleIdPath.isEmpty() ? moduleId : (parentModuleIdPath + "." + moduleId);
+        Map<String, String> moduleProps = propertiesByModuleIdPath.get(moduleIdPath);
         ProjectDefinition childProject = loadChildProject(parentProject, moduleProps, moduleId);
         // check the uniqueness of the child key
         checkUniquenessOfChildKey(childProject, parentProject);
         // the child project may have children as well
-        defineChildren(childProject, propertiesByModuleId);
+        defineChildren(childProject, propertiesByModuleIdPath, moduleIdPath);
         // and finally add this child project to its parent
         parentProject.addSubProject(childProject);
       }
diff --git a/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java b/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
index 75900b811de..b55f0c64e31 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
@@ -19,18 +19,7 @@
  */
 package org.sonar.batch.scan;
 
-import org.apache.commons.lang.StringUtils;
-import org.junit.Before;
-import org.sonar.api.batch.AnalysisMode;
-import org.sonar.batch.analysis.AnalysisProperties;
 import com.google.common.collect.Maps;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExpectedException;
-import org.sonar.api.batch.bootstrap.ProjectDefinition;
-import org.sonar.api.batch.bootstrap.ProjectReactor;
-import org.sonar.test.TestUtils;
-
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -38,11 +27,20 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
+import org.apache.commons.lang.StringUtils;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonar.api.batch.AnalysisMode;
+import org.sonar.api.batch.bootstrap.ProjectDefinition;
+import org.sonar.api.batch.bootstrap.ProjectReactor;
+import org.sonar.batch.analysis.AnalysisProperties;
+import org.sonar.test.TestUtils;
 
-import static org.mockito.Mockito.when;
-
-import static org.mockito.Mockito.mock;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class ProjectReactorBuilderTest {
 
@@ -50,12 +48,12 @@ public class ProjectReactorBuilderTest {
   public ExpectedException thrown = ExpectedException.none();
 
   private AnalysisMode mode;
-  
+
   @Before
   public void setUp() {
     mode = mock(AnalysisMode.class);
   }
-  
+
   @Test
   public void shouldDefineSimpleProject() {
     ProjectDefinition projectDefinition = loadProjectDefinition("simple-project");
@@ -91,10 +89,28 @@ public class ProjectReactorBuilderTest {
   }
 
   @Test
-  public void modulesRepeatedIds() {
+  public void modulesDuplicateIds() {
     thrown.expect(IllegalStateException.class);
-    thrown.expectMessage("Two modules have the same id: module1");
-    loadProjectDefinition("multi-module-repeated-id");
+    thrown.expectMessage("Two modules have the same id: 'module1'. Each module must have a unique id.");
+
+    loadProjectDefinition("multi-module-duplicate-id");
+  }
+
+  @Test
+  public void modulesRepeatedIds() {
+    ProjectDefinition rootProject = loadProjectDefinition("multi-module-repeated-id");
+
+    List<ProjectDefinition> modules = rootProject.getSubProjects();
+    assertThat(modules.size()).isEqualTo(1);
+    // Module 1
+    ProjectDefinition module1 = modules.get(0);
+    assertThat(module1.getKey()).isEqualTo("com.foo.project:module1");
+    assertThat(module1.getName()).isEqualTo("Foo Module 1");
+
+    // Module 1 -> Module 1
+    ProjectDefinition module1_module1 = module1.getSubProjects().get(0);
+    assertThat(module1_module1.getKey()).isEqualTo("com.foo.project:module1:module1");
+    assertThat(module1_module1.getName()).isEqualTo("Foo Sub Module 1");
   }
 
   @Test
@@ -460,12 +476,12 @@ public class ProjectReactorBuilderTest {
 
     assertThat(workDir).isEqualTo(new File(baseDir, ".sonar"));
   }
-  
+
   @Test
   public void nonAssociatedMode() {
     when(mode.isIssues()).thenReturn(true);
     ProjectDefinition project = loadProjectDefinition("multi-module-with-basedir-not-associated");
-    
+
     assertThat(project.getKey()).isEqualTo("project");
   }
 
@@ -537,10 +553,10 @@ public class ProjectReactorBuilderTest {
   private ProjectDefinition loadProjectDefinition(String projectFolder) {
     Map<String, String> props = loadProps(projectFolder);
     AnalysisProperties bootstrapProps = new AnalysisProperties(props, null);
-    ProjectReactor projectReactor = new ProjectReactorBuilder(bootstrapProps,mode).execute();
+    ProjectReactor projectReactor = new ProjectReactorBuilder(bootstrapProps, mode).execute();
     return projectReactor.getRoot();
   }
-  
+
   protected static Properties toProperties(File propertyFile) {
     Properties propsFromFile = new Properties();
     try (FileInputStream fileInputStream = new FileInputStream(propertyFile)) {
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-duplicate-id/sonar-project.properties b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-duplicate-id/sonar-project.properties
new file mode 100644
index 00000000000..0a971805d13
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-duplicate-id/sonar-project.properties
@@ -0,0 +1,12 @@
+sonar.projectKey=com.foo.project
+sonar.projectName=Foo Project
+sonar.projectVersion=1.0-SNAPSHOT
+sonar.projectDescription=Description of Foo Project
+
+sonar.sources=sources
+sonar.tests=tests
+sonar.binaries=target/classes
+
+sonar.modules=module1,module1
+
+module1.sonar.sources=src
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-repeated-id/sonar-project.properties b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-repeated-id/sonar-project.properties
index b1816be030b..e63d3da926c 100644
--- a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-repeated-id/sonar-project.properties
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/multi-module-repeated-id/sonar-project.properties
@@ -10,10 +10,8 @@ sonar.binaries=target/classes
 sonar.modules=module1
 
 module1.sonar.projectBaseDir=modules/module1
-module1.sonar.projectKey=com.foo.project.module1
 module1.sonar.projectName=Foo Module 1
 module1.sonar.modules=module1
 
 module1.module1.sonar.projectBaseDir=module1
-module1.module1.sonar.projectKey=com.foo.project.module1.module1
 module1.module1.sonar.projectName=Foo Sub Module 1