From 4d67b14c36132e501a749682e587271ad0c2b9a6 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Fri, 10 Jul 2015 16:04:12 +0200 Subject: [PATCH] SONAR-6709 Simplify RulesDefinition API when two plugins define the same repository --- .../server/rule/RuleDefinitionsLoader.java | 6 +- .../api/server/rule/RulesDefinition.java | 127 ++++++++++++------ .../api/server/rule/RulesDefinitionTest.java | 68 +++++++--- 3 files changed, 133 insertions(+), 68 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleDefinitionsLoader.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleDefinitionsLoader.java index 53c2b0389ad..21af13bfd1b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleDefinitionsLoader.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleDefinitionsLoader.java @@ -19,14 +19,12 @@ */ package org.sonar.server.rule; -import org.sonar.api.server.ServerSide; import org.sonar.api.server.rule.RulesDefinition; /** - * Loads all instances of {@link org.sonar.api.server.rule.RulesDefinition} - * and initializes {@link org.sonar.server.rule.RuleRepositories}. Used at server startup. + * Loads all instances of {@link RulesDefinition} + * and initializes {@link RuleRepositories}. Used at server startup. */ -@ServerSide public class RuleDefinitionsLoader { private final DeprecatedRulesDefinitionLoader deprecatedDefinitionConverter; 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 repositoriesByKey = Maps.newHashMap(); - private final ListMultimap extendedRepositoriesByKey = ArrayListMultimap.create(); + /** + * New builder for {@link org.sonar.api.server.rule.RulesDefinition.Repository}. + *

+ * 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 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 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 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 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 ruleBuilder = ImmutableMap.builder(); + + Map 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 @@ -227,23 +260,14 @@ public class RulesDefinitionTest { assertThat(context.repository("findbugs").name()).isEqualTo("findbugs"); } - @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 -- 2.39.5