SONAR-6709 Simplify RulesDefinition API when two plugins define the same repository

2 files changed, 131 insertions, 64 deletions

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java
index 0c2919e3dc1..1d1903991c6 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java
@@ -19,34 +19,33 @@
  */
 package org.sonar.api.server.rule;
 
+import com.google.common.base.Objects;
 import com.google.common.base.Strings;
-import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
-import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
+import java.io.IOException;
+import java.net.URL;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.Immutable;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.ExtensionPoint;
-import org.sonar.api.server.ServerSide;
 import org.sonar.api.rule.RuleStatus;
 import org.sonar.api.rule.Severity;
+import org.sonar.api.server.ServerSide;
 import org.sonar.api.server.debt.DebtRemediationFunction;
 import org.sonar.api.utils.log.Loggers;
 
-import javax.annotation.CheckForNull;
-import javax.annotation.Nullable;
-import javax.annotation.concurrent.Immutable;
-
-import java.io.IOException;
-import java.net.URL;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
 /**
  * Defines some coding rules of the same repository. For example the Java Findbugs plugin provides an implementation of
  * this extension point in order to define the rules that it supports.
@@ -363,18 +362,28 @@ public interface RulesDefinition {
   }
 
   /**
-   * Instantiated by core but not by plugins
+   * Instantiated by core but not by plugins, except for their tests.
    */
-  public class Context {
+  class Context {
     private final Map<String, Repository> repositoriesByKey = Maps.newHashMap();
-    private final ListMultimap<String, ExtendedRepository> extendedRepositoriesByKey = ArrayListMultimap.create();
 
+    /**
+     * New builder for {@link org.sonar.api.server.rule.RulesDefinition.Repository}.
+     * <p/>
+     * A plugin can add rules to a repository that is defined then executed by another plugin. For instance
+     * the FbContrib plugin contributes to the Findbugs plugin rules. In this case no need
+     * to execute {@link org.sonar.api.server.rule.RulesDefinition.NewRepository#setName(String)}
+     */
     public NewRepository createRepository(String key, String language) {
-      return new NewRepositoryImpl(this, key, language, false);
+      return new NewRepositoryImpl(this, key, language);
     }
 
-    public NewExtendedRepository extendRepository(String key, String language) {
-      return new NewRepositoryImpl(this, key, language, true);
+    /**
+     * @deprecated since 5.2. Simply use {@link #createRepository(String, String)}
+     */
+    @Deprecated
+    public NewRepository extendRepository(String key, String language) {
+      return createRepository(key, language);
     }
 
     @CheckForNull
@@ -386,23 +395,33 @@ public interface RulesDefinition {
       return ImmutableList.copyOf(repositoriesByKey.values());
     }
 
+    /**
+     * @deprecated returns empty list since 5.2. Concept of "extended repository" was misleading and not valuable. Simply declare
+     * repositories and use {@link #repositories()}
+     */
+    @Deprecated
     public List<ExtendedRepository> extendedRepositories(String repositoryKey) {
-      return ImmutableList.copyOf(extendedRepositoriesByKey.get(repositoryKey));
+      return Collections.emptyList();
     }
 
+    /**
+     * @deprecated returns empty list since 5.2. Concept of "extended repository" was misleading and not valuable. Simply declare
+     * repositories and use {@link #repositories()}
+     */
+    @Deprecated
     public List<ExtendedRepository> extendedRepositories() {
-      return ImmutableList.copyOf(extendedRepositoriesByKey.values());
+      return Collections.emptyList();
     }
 
     private void registerRepository(NewRepositoryImpl newRepository) {
-      if (repositoriesByKey.containsKey(newRepository.key)) {
-        throw new IllegalStateException(String.format("The rule repository '%s' is defined several times", newRepository.key));
+      Repository existing = repositoriesByKey.get(newRepository.key());
+      if (existing != null) {
+        if (!existing.language().equals(newRepository.language)) {
+          throw new IllegalStateException(String.format("The rule repository '%s' must not be defined for two different languages: %s and %s", newRepository.key,
+            existing.language(), newRepository.language));
+        }
       }
-      repositoriesByKey.put(newRepository.key, new RepositoryImpl(newRepository));
-    }
-
-    private void registerExtendedRepository(NewRepositoryImpl newRepository) {
-      extendedRepositoriesByKey.put(newRepository.key, new RepositoryImpl(newRepository));
+      repositoriesByKey.put(newRepository.key, new RepositoryImpl(newRepository, existing));
     }
   }
 
@@ -425,14 +444,12 @@ public interface RulesDefinition {
 
   class NewRepositoryImpl implements NewRepository {
     private final Context context;
-    private final boolean extended;
     private final String key;
     private String language;
     private String name;
     private final Map<String, NewRule> newRules = Maps.newHashMap();
 
-    private NewRepositoryImpl(Context context, String key, String language, boolean extended) {
-      this.extended = extended;
+    private NewRepositoryImpl(Context context, String key, String language) {
       this.context = context;
       this.key = this.name = key;
       this.language = language;
@@ -480,11 +497,15 @@ public interface RulesDefinition {
       // note that some validations can be done here, for example for
       // verifying that at least one rule is declared
 
-      if (extended) {
-        context.registerExtendedRepository(this);
-      } else {
-        context.registerRepository(this);
-      }
+      context.registerRepository(this);
+    }
+
+    @Override
+    public String toString() {
+      return Objects.toStringHelper(this)
+        .add("key", key)
+        .add("language", language)
+        .toString();
     }
   }
 
@@ -510,16 +531,30 @@ public interface RulesDefinition {
     private final String name;
     private final Map<String, Rule> rulesByKey;
 
-    private RepositoryImpl(NewRepositoryImpl newRepository) {
+    private RepositoryImpl(NewRepositoryImpl newRepository, @Nullable Repository mergeInto) {
       this.key = newRepository.key;
       this.language = newRepository.language;
-      this.name = newRepository.name;
-      ImmutableMap.Builder<String, Rule> ruleBuilder = ImmutableMap.builder();
+
+      Map<String, Rule> ruleBuilder = new HashMap<>();
+      if (mergeInto != null) {
+        if (!StringUtils.equals(newRepository.language, mergeInto.language()) || !StringUtils.equals(newRepository.key, mergeInto.key())) {
+          throw new IllegalArgumentException(String.format("Bug - language and key of the repositories to be merged should be the sames: %s and %s", newRepository, mergeInto));
+        }
+        this.name = StringUtils.defaultIfBlank(mergeInto.name(), newRepository.name);
+        for (Rule rule : mergeInto.rules()) {
+          if (!newRepository.key().startsWith("common-") && ruleBuilder.containsKey(rule.key())) {
+            Loggers.get(getClass()).warn("The rule '{}' of repository '{}' is declared several times", rule.key(), mergeInto.key());
+          }
+          ruleBuilder.put(rule.key(), rule);
+        }
+      } else {
+        this.name = newRepository.name;
+      }
       for (NewRule newRule : newRepository.newRules.values()) {
         newRule.validate();
         ruleBuilder.put(newRule.key, new Rule(this, newRule));
       }
-      this.rulesByKey = ruleBuilder.build();
+      this.rulesByKey = ImmutableMap.copyOf(ruleBuilder);
     }
 
     @Override
@@ -564,6 +599,14 @@ public interface RulesDefinition {
     public int hashCode() {
       return key.hashCode();
     }
+
+    @Override
+    public String toString() {
+      return Objects.toStringHelper(this)
+        .add("language", language)
+        .add("key", key)
+        .toString();
+    }
   }
 
   /**
@@ -607,7 +650,7 @@ public interface RulesDefinition {
     /**
      * Required rule name
      */
-    public NewRule setName(@Nullable String s) {
+    public NewRule setName(String s) {
       this.name = StringUtils.trimToNull(s);
       return this;
     }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java
index b4158ce60ba..28b21f8c96f 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java
@@ -19,12 +19,15 @@
  */
 package org.sonar.api.server.rule;
 
+import org.junit.Rule;
 import org.junit.Test;
 import org.sonar.api.rule.RuleStatus;
 import org.sonar.api.rule.Severity;
 import org.sonar.api.server.debt.DebtRemediationFunction;
 
 import java.net.URL;
+import org.sonar.api.utils.log.LogTester;
+import org.sonar.api.utils.log.LoggerLevel;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
@@ -33,6 +36,9 @@ public class RulesDefinitionTest {
 
   RulesDefinition.Context context = new RulesDefinition.Context();
 
+  @Rule
+  public LogTester logTester = new LogTester();
+
   @Test
   public void define_repositories() {
     assertThat(context.repositories()).isEmpty();
@@ -202,22 +208,49 @@ public class RulesDefinitionTest {
   }
 
   @Test
-  public void extend_repository() {
-    assertThat(context.extendedRepositories()).isEmpty();
-
-    // for example fb-contrib
-    RulesDefinition.NewExtendedRepository newFindbugs = context.extendRepository("findbugs", "java");
+  public void add_rules_to_existing_repository() {
+    RulesDefinition.NewRepository newFindbugs = context.createRepository("findbugs", "java").setName("Findbugs");
     newFindbugs.createRule("NPE").setName("NPE").setHtmlDescription("NPE");
     newFindbugs.done();
 
-    assertThat(context.repositories()).isEmpty();
-    assertThat(context.extendedRepositories()).hasSize(1);
-    assertThat(context.extendedRepositories("other")).isEmpty();
-    assertThat(context.extendedRepositories("findbugs")).hasSize(1);
+    RulesDefinition.NewRepository newFbContrib = context.createRepository("findbugs", "java");
+    newFbContrib.createRule("VULNERABILITY").setName("Vulnerability").setMarkdownDescription("Detect vulnerability");
+    newFbContrib.done();
 
-    RulesDefinition.ExtendedRepository findbugs = context.extendedRepositories("findbugs").get(0);
+    assertThat(context.repositories()).hasSize(1);
+    RulesDefinition.Repository findbugs = context.repository("findbugs");
+    assertThat(findbugs.key()).isEqualTo("findbugs");
     assertThat(findbugs.language()).isEqualTo("java");
-    assertThat(findbugs.rule("NPE")).isNotNull();
+    assertThat(findbugs.name()).isEqualTo("Findbugs");
+    assertThat(findbugs.rules()).extracting("key").containsOnly("NPE", "VULNERABILITY");
+  }
+
+  /**
+   * This is temporarily accepted only for the support of the common-rules that are still declared
+   * by plugins. It could be removed in 7.0
+   * @since 5.2
+   */
+  @Test
+  public void allow_to_replace_an_existing_common_rule() {
+    RulesDefinition.NewRepository newCommonJava1 = context.createRepository("common-java", "java").setName("Common Java");
+    newCommonJava1.createRule("coverage").setName("Lack of coverage").setHtmlDescription("Coverage must be high");
+    newCommonJava1.done();
+
+    RulesDefinition.NewRepository newCommonJava2 = context.createRepository("common-java", "java");
+    newCommonJava2.createRule("coverage").setName("Lack of coverage (V2)").setMarkdownDescription("Coverage must be high (V2)");
+    newCommonJava2.done();
+
+    RulesDefinition.Repository commonJava = context.repository("common-java");
+    assertThat(commonJava.rules()).hasSize(1);
+    RulesDefinition.Rule rule = commonJava.rule("coverage");
+    assertThat(rule.name()).isEqualTo("Lack of coverage (V2)");
+
+    // replacement but not merge -> keep only the v2 (which has markdown but not html description)
+    assertThat(rule.markdownDescription()).isEqualTo("Coverage must be high (V2)");
+    assertThat(rule.htmlDescription()).isNull();
+
+    // do not log warning
+    assertThat(logTester.logs()).isEmpty();
   }
 
   @Test
@@ -228,22 +261,13 @@ public class RulesDefinitionTest {
   }
 
   @Test
-  public void fail_if_duplicated_repo_keys() {
-    context.createRepository("findbugs", "java").done();
-    try {
-      context.createRepository("findbugs", "whatever_the_language").done();
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("The rule repository 'findbugs' is defined several times");
-    }
-  }
-
-  @Test
   public void warning_if_duplicated_rule_keys() {
     RulesDefinition.NewRepository findbugs = context.createRepository("findbugs", "java");
     findbugs.createRule("NPE");
     findbugs.createRule("NPE");
     // do not fail as long as http://jira.sonarsource.com/browse/SONARJAVA-428 is not fixed
+    // and as common-rules are packaged within plugins (common-rules were integrated to core in v5.2)
+    assertThat(logTester.logs(LoggerLevel.WARN)).hasSize(1);
   }
 
   @Test