SONAR-4908 support tags in sonar-check-api and in standard XML format

sonar-check-api/src/main/java/org/sonar/check/Rule.java

@@ -42,12 +42,12 @@ public @interface Rule {
   String name() default "";
 
   /**
-   * The description, optional.
+   * HTML description
    */
   String description() default "";
 
   /**
-   * Default priority.
+   * Default severity used when activating the rule in a Quality profile.
    */
   Priority priority() default Priority.MAJOR;
 
@@ -58,4 +58,10 @@ public @interface Rule {
    * @since 3.6
    */
   String status() default "READY";
+
+  /**
+   * Rule tags
+   * @since 4.2
+   */
+  String[] tags() default {};
 }

sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RuleDefinitions.java

@@ -19,13 +19,8 @@
  */
 package org.sonar.api.server.rule;
 
-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 com.google.common.collect.*;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.ServerExtension;
@@ -35,8 +30,9 @@ import org.sonar.api.rule.Severity;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
-
+import java.io.IOException;
 import java.io.InputStream;
+import java.net.URL;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -58,6 +54,7 @@ public interface RuleDefinitions extends ServerExtension {
     private final Map<String, Repository> repositoriesByKey = Maps.newHashMap();
     private final ListMultimap<String, ExtendedRepository> extendedRepositoriesByKey = ArrayListMultimap.create();
 
+
     public NewRepository newRepository(String key, String language) {
       return new NewRepositoryImpl(this, key, language, false);
     }
@@ -121,12 +118,14 @@ public interface RuleDefinitions extends ServerExtension {
      * &lt;/description&gt;
      *
      * &lt;!-- optional fields --&gt;
-     * &lt;configKey&gt;Checker/TreeWalker/LocalVariableName&lt;/configKey&gt;
+     * &lt;internalKey&gt;Checker/TreeWalker/LocalVariableName&lt;/internalKey&gt;
      * &lt;severity&gt;BLOCKER&lt;/severity&gt;
      * &lt;cardinality&gt;MULTIPLE&lt;/cardinality&gt;
      * &lt;status&gt;BETA&lt;/status&gt;
      * &lt;param&gt;
      * &lt;key&gt;the-param-key&lt;/key&gt;
+     * &lt;tag&gt;style&lt;/tag&gt;
+     * &lt;tag&gt;security&lt;/tag&gt;
      * &lt;description&gt;
      * &lt;![CDATA[
      * the param-description
@@ -139,6 +138,7 @@ public interface RuleDefinitions extends ServerExtension {
      * &lt;/param&gt;
      *
      * &lt;!-- deprecated fields --&gt;
+     * &lt;configKey&gt;Checker/TreeWalker/LocalVariableName&lt;/configKey&gt;
      * &lt;priority&gt;BLOCKER&lt;/priority&gt;
      * &lt;/rule&gt;
      * &lt;/rules&gt;
@@ -307,7 +307,7 @@ public interface RuleDefinitions extends ServerExtension {
 
   class NewRule {
     private final String repoKey, key;
-    private String name, htmlDescription, engineKey, severity = Severity.MAJOR;
+    private String name, htmlDescription, internalKey, severity = Severity.MAJOR;
     private boolean template;
     private RuleStatus status = RuleStatus.defaultStatus();
     private final Set<String> tags = Sets.newTreeSet();
@@ -323,8 +323,7 @@ public interface RuleDefinitions extends ServerExtension {
     }
 
     public NewRule setName(String s) {
-      // TODO remove newlines
-      this.name = s;
+      this.name = StringUtils.trim(s);
       return this;
     }
 
@@ -342,7 +341,23 @@ public interface RuleDefinitions extends ServerExtension {
     }
 
     public NewRule setHtmlDescription(String s) {
-      this.htmlDescription = s;
+      this.htmlDescription = StringUtils.trim(s);
+      return this;
+    }
+
+    /**
+     * Load description from a file available in classpath. Example : <code>setHtmlDescription(getClass().getResource("/myrepo/Rule1234.html")</code>
+     */
+    public NewRule setHtmlDescription(@Nullable URL classpathUrl) {
+      if (classpathUrl != null) {
+        try {
+          setHtmlDescription(IOUtils.toString(classpathUrl));
+        } catch (IOException e) {
+          throw new IllegalStateException("Fail to read: " + classpathUrl);
+        }
+      } else {
+        this.htmlDescription = null;
+      }
       return this;
     }
 
@@ -393,8 +408,8 @@ public interface RuleDefinitions extends ServerExtension {
      * in webapp. For example the Java Checkstyle plugin feeds this field
      * with the internal path ("Checker/TreeWalker/AnnotationUseStyle").
      */
-    public NewRule setEngineKey(@Nullable String s) {
-      this.engineKey = s;
+    public NewRule setInternalKey(@Nullable String s) {
+      this.internalKey = s;
       return this;
     }
 
@@ -416,7 +431,7 @@ public interface RuleDefinitions extends ServerExtension {
   @Immutable
   class Rule {
     private final Repository repository;
-    private final String repoKey, key, name, htmlDescription, engineKey, severity;
+    private final String repoKey, key, name, htmlDescription, internalKey, severity;
     private final boolean template;
     private final Set<String> tags;
     private final Map<String, Param> params;
@@ -428,7 +443,7 @@ public interface RuleDefinitions extends ServerExtension {
       this.key = newRule.key;
       this.name = newRule.name;
       this.htmlDescription = newRule.htmlDescription;
-      this.engineKey = newRule.engineKey;
+      this.internalKey = newRule.internalKey;
       this.severity = newRule.severity;
       this.template = newRule.template;
       this.status = newRule.status;
@@ -483,11 +498,11 @@ public interface RuleDefinitions extends ServerExtension {
     }
 
     /**
-     * @see RuleDefinitions.NewRule#setEngineKey(String)
+     * @see RuleDefinitions.NewRule#setInternalKey(String)
      */
     @CheckForNull
-    public String engineKey() {
-      return engineKey;
+    public String internalKey() {
+      return internalKey;
     }
 
     @Override

sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RuleDefinitionsFromAnnotations.java

@@ -75,6 +75,7 @@ class RuleDefinitionsFromAnnotations {
     rule.setSeverity(ruleAnnotation.priority().name());
     rule.setTemplate(ruleAnnotation.cardinality() == Cardinality.MULTIPLE);
     rule.setStatus(RuleStatus.valueOf(ruleAnnotation.status()));
+    rule.setTags(ruleAnnotation.tags());
 
     List<Field> fields = FieldUtils2.getFields(clazz, true);
     for (Field field : fields) {

sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RuleDefinitionsFromXml.java

@@ -80,9 +80,10 @@ class RuleDefinitionsFromXml {
   }
 
   private void processRule(RuleDefinitions.NewRepository repo, SMInputCursor ruleC) throws XMLStreamException {
-    String key = null, name = null, description = null, engineKey = null, severity = Severity.defaultSeverity(), status = null;
+    String key = null, name = null, description = null, internalKey = null, severity = Severity.defaultSeverity(), status = null;
     Cardinality cardinality = Cardinality.SINGLE;
     List<ParamStruct> params = new ArrayList<ParamStruct>();
+    List<String> tags = new ArrayList<String>();
 
     /* BACKWARD COMPATIBILITY WITH VERY OLD FORMAT */
     String keyAttribute = ruleC.getAttrValue("key");
@@ -108,7 +109,11 @@ class RuleDefinitionsFromXml {
         key = StringUtils.trim(cursor.collectDescendantText(false));
 
       } else if (StringUtils.equalsIgnoreCase("configKey", nodeName)) {
-        engineKey = StringUtils.trim(cursor.collectDescendantText(false));
+        // deprecated field, replaced by internalKey
+        internalKey = StringUtils.trim(cursor.collectDescendantText(false));
+
+      } else if (StringUtils.equalsIgnoreCase("internalKey", nodeName)) {
+        internalKey = StringUtils.trim(cursor.collectDescendantText(false));
 
       } else if (StringUtils.equalsIgnoreCase("priority", nodeName)) {
         // deprecated field, replaced by severity
@@ -125,13 +130,17 @@ class RuleDefinitionsFromXml {
 
       } else if (StringUtils.equalsIgnoreCase("param", nodeName)) {
         params.add(processParameter(cursor));
+
+      } else if (StringUtils.equalsIgnoreCase("tag", nodeName)) {
+        tags.add(StringUtils.trim(cursor.collectDescendantText(false)));
       }
     }
     RuleDefinitions.NewRule rule = repo.newRule(key)
       .setHtmlDescription(description)
       .setSeverity(severity)
       .setName(name)
-      .setEngineKey(engineKey)
+      .setInternalKey(internalKey)
+      .setTags(tags.toArray(new String[tags.size()]))
       .setTemplate(cardinality == Cardinality.MULTIPLE);
     if (status != null) {
       rule.setStatus(RuleStatus.valueOf(status));

sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RuleDefinitionsFromAnnotationsTest.java

@@ -45,6 +45,7 @@ public class RuleDefinitionsFromAnnotationsTest {
     assertThat(rule.htmlDescription()).isEqualTo("Foo Bar");
     assertThat(rule.severity()).isEqualTo(Severity.BLOCKER);
     assertThat(rule.params()).hasSize(1);
+    assertThat(rule.tags()).isEmpty();
 
     RuleDefinitions.Param prop = rule.param("property");
     assertThat(prop.key()).isEqualTo("property");
@@ -100,15 +101,6 @@ public class RuleDefinitionsFromAnnotationsTest {
     assertThat(prop.type()).isEqualTo(RuleParamType.TEXT);
   }
 
-  @Test
-  @Ignore("TODO list supported types in RuleParamType")
-  public void should_reject_invalid_property_types() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Invalid property type [INVALID]");
-
-    load(RuleWithInvalidPropertyType.class);
-  }
-
   @Test
   public void should_recognize_type() {
     assertThat(RuleDefinitionsFromAnnotations.guessType(Integer.class)).isEqualTo(RuleParamType.INTEGER);
@@ -130,6 +122,14 @@ public class RuleDefinitionsFromAnnotationsTest {
     assertThat(rule.name()).isEqualTo("foo");
   }
 
+  @Test
+  public void rule_with_tags() {
+    RuleDefinitions.Repository repository = load(RuleWithTags.class);
+    assertThat(repository.rules()).hasSize(1);
+    RuleDefinitions.Rule rule = repository.rules().get(0);
+    assertThat(rule.tags()).containsOnly("style", "security");
+  }
+
   @Test
   public void overridden_class() {
     RuleDefinitions.Repository repository = load(OverridingRule.class);
@@ -154,10 +154,6 @@ public class RuleDefinitionsFromAnnotationsTest {
   static class RuleWithoutKey {
   }
 
-  @org.sonar.check.Rule(key = "foo")
-  static class RuleWithoutNameNorDescription {
-  }
-
   @org.sonar.check.Rule(key = "foo", name = "bar", description = "Foo Bar", priority = Priority.BLOCKER, status = "BETA")
   static class RuleWithProperty {
     @org.sonar.check.RuleProperty(description = "Ignore ?", defaultValue = "false")
@@ -187,4 +183,8 @@ public class RuleDefinitionsFromAnnotationsTest {
     @org.sonar.check.RuleProperty(description = "text", defaultValue = "Long text", type = "INVALID")
     public String property;
   }
+
+  @org.sonar.check.Rule(key = "foo", name = "bar", description = "Bar", tags = {"style", "security"})
+  static class RuleWithTags {
+  }
 }

sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RuleDefinitionsFromXmlTest.java

@@ -58,7 +58,8 @@ public class RuleDefinitionsFromXmlTest {
     assertThat(rule.severity()).isEqualTo(Severity.BLOCKER);
     assertThat(rule.template()).isTrue();
     assertThat(rule.status()).isEqualTo(RuleStatus.BETA);
-    assertThat(rule.engineKey()).isEqualTo("Checker/TreeWalker/LocalVariableName");
+    assertThat(rule.internalKey()).isEqualTo("Checker/TreeWalker/LocalVariableName");
+    assertThat(rule.tags()).containsOnly("style", "security");
 
     assertThat(rule.params()).hasSize(2);
     RuleDefinitions.Param ignore = rule.param("ignore");
@@ -116,6 +117,7 @@ public class RuleDefinitionsFromXmlTest {
     assertThat(repository.rules()).hasSize(1);
     RuleDefinitions.Rule rule = repository.rules().get(0);
     assertThat(rule.key()).isEqualTo("org.sonar.it.checkstyle.MethodsCountCheck");
+    assertThat(rule.internalKey()).isEqualTo("Checker/TreeWalker/org.sonar.it.checkstyle.MethodsCountCheck");
     assertThat(rule.severity()).isEqualTo(Severity.CRITICAL);
     assertThat(rule.htmlDescription()).isEqualTo("Count methods");
     assertThat(rule.param("minMethodsCount")).isNotNull();

sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RuleDefinitionsTest.java

@@ -23,6 +23,8 @@ import org.junit.Test;
 import org.sonar.api.rule.Severity;
 import org.sonar.api.rule.RuleStatus;
 
+import java.net.URL;
+
 import static org.fest.assertions.Assertions.assertThat;
 import static org.fest.assertions.Fail.fail;
 
@@ -67,7 +69,7 @@ public class RuleDefinitionsTest {
       .setHtmlDescription("Detect <code>NPE</code>")
       .setHtmlDescription("Detect <code>java.lang.NullPointerException</code>")
       .setSeverity(Severity.BLOCKER)
-      .setEngineKey("/something")
+      .setInternalKey("/something")
       .setStatus(RuleStatus.BETA)
       .setTags("one", "two")
       .addTags("two", "three", "four");
@@ -84,7 +86,7 @@ public class RuleDefinitionsTest {
     assertThat(npeRule.htmlDescription()).isEqualTo("Detect <code>java.lang.NullPointerException</code>");
     assertThat(npeRule.tags()).containsOnly("one", "two", "three", "four");
     assertThat(npeRule.params()).isEmpty();
-    assertThat(npeRule.engineKey()).isEqualTo("/something");
+    assertThat(npeRule.internalKey()).isEqualTo("/something");
     assertThat(npeRule.template()).isFalse();
     assertThat(npeRule.status()).isEqualTo(RuleStatus.BETA);
     assertThat(npeRule.toString()).isEqualTo("[repository=findbugs, key=NPE]");
@@ -106,7 +108,7 @@ public class RuleDefinitionsTest {
     assertThat(rule.key()).isEqualTo("NPE");
     assertThat(rule.severity()).isEqualTo(Severity.MAJOR);
     assertThat(rule.params()).isEmpty();
-    assertThat(rule.engineKey()).isNull();
+    assertThat(rule.internalKey()).isNull();
     assertThat(rule.status()).isEqualTo(RuleStatus.defaultStatus());
     assertThat(rule.tags()).isEmpty();
   }
@@ -140,6 +142,16 @@ public class RuleDefinitionsTest {
     assertThat(level.hashCode()).isEqualTo(level.hashCode());
   }
 
+  @Test
+  public void sanitize_rule_name() {
+    RuleDefinitions.NewRepository newFindbugs = context.newRepository("findbugs", "java");
+    newFindbugs.newRule("NPE").setName("   \n  NullPointer   \n   ").setHtmlDescription("NPE");
+    newFindbugs.done();
+
+    RuleDefinitions.Rule rule = context.repository("findbugs").rule("NPE");
+    assertThat(rule.name()).isEqualTo("NullPointer");
+  }
+
   @Test
   public void extend_repository() {
     assertThat(context.extendedRepositories()).isEmpty();
@@ -220,10 +232,32 @@ public class RuleDefinitionsTest {
     }
   }
 
+  @Test
+  public void load_rule_description_from_file() {
+    RuleDefinitions.NewRepository newRepository = context.newRepository("findbugs", "java");
+    newRepository.newRule("NPE").setName("NPE").setHtmlDescription(getClass().getResource("/org/sonar/api/server/rule/RuleDefinitionsTest/sample.html"));
+    newRepository.done();
+
+    RuleDefinitions.Rule rule = context.repository("findbugs").rule("NPE");
+    assertThat(rule.htmlDescription()).isEqualTo("description of rule loaded from file");
+  }
+
+  @Test
+  public void fail_to_load_rule_description_from_file() {
+    RuleDefinitions.NewRepository newRepository = context.newRepository("findbugs", "java");
+    newRepository.newRule("NPE").setName("NPE").setHtmlDescription((URL)null);
+    try {
+      newRepository.done();
+      fail();
+    } catch (IllegalStateException e) {
+      assertThat(e).hasMessage("HTML description of rule [repository=findbugs, key=NPE] is empty");
+    }
+  }
+
   @Test
   public void fail_if_blank_rule_html_description() {
     RuleDefinitions.NewRepository newRepository = context.newRepository("findbugs", "java");
-    newRepository.newRule("NPE").setName("NPE").setHtmlDescription(null);
+    newRepository.newRule("NPE").setName("NPE").setHtmlDescription((String)null);
     try {
       newRepository.done();
       fail();

sonar-plugin-api/src/test/resources/org/sonar/api/server/rule/RuleDefinitionsFromXmlTest/rules.xml

@@ -6,10 +6,12 @@
     <description>
       <![CDATA[Description of Complete]]>
     </description>
-    <configKey>Checker/TreeWalker/LocalVariableName</configKey>
+    <internalKey>Checker/TreeWalker/LocalVariableName</internalKey>
     <severity>BLOCKER</severity>
     <cardinality>MULTIPLE</cardinality>
     <status>BETA</status>
+    <tag>style</tag>
+    <tag>security</tag>
     <param>
       <key>tokens</key>
       <description>
@@ -36,4 +38,4 @@
       <![CDATA[Description of Minimal]]>
     </description>
   </rule>
-</rules>
+</rules>

sonar-plugin-api/src/test/resources/org/sonar/api/server/rule/RuleDefinitionsTest/sample.html

@@ -0,0 +1 @@
+description of rule loaded from file

sonar-server/src/main/java/org/sonar/server/rule/DeprecatedRuleDefinitions.java

@@ -63,7 +63,7 @@ public class DeprecatedRuleDefinitions implements RuleDefinitions {
         NewRule newRule = newRepository.newRule(rule.getKey());
         newRule.setName(ruleName(repository.getKey(), rule));
         newRule.setHtmlDescription(ruleDescription(repository.getKey(), rule));
-        newRule.setEngineKey(rule.getConfigKey());
+        newRule.setInternalKey(rule.getConfigKey());
         newRule.setTemplate(Cardinality.MULTIPLE.equals(rule.getCardinality()));
         newRule.setSeverity(rule.getSeverity().toString());
         newRule.setStatus(rule.getStatus() == null ? RuleStatus.defaultStatus() : RuleStatus.valueOf(rule.getStatus()));

sonar-server/src/main/java/org/sonar/server/rule/RuleRegistration.java

@@ -154,7 +154,7 @@ public class RuleRegistration implements Startable {
   private RuleDto enableAndInsert(Buffer buffer, SqlSession sqlSession, RuleDefinitions.Rule ruleDef) {
     RuleDto ruleDto = new RuleDto()
       .setCardinality(ruleDef.template() ? Cardinality.MULTIPLE : Cardinality.SINGLE)
-      .setConfigKey(ruleDef.engineKey())
+      .setConfigKey(ruleDef.internalKey())
       .setDescription(ruleDef.htmlDescription())
       .setLanguage(ruleDef.repository().language())
       .setName(ruleDef.name())
@@ -200,8 +200,8 @@ public class RuleRegistration implements Startable {
       dto.setDescription(def.htmlDescription());
       changed = true;
     }
-    if (!StringUtils.equals(dto.getConfigKey(), def.engineKey())) {
-      dto.setConfigKey(def.engineKey());
+    if (!StringUtils.equals(dto.getConfigKey(), def.internalKey())) {
+      dto.setConfigKey(def.internalKey());
       changed = true;
     }
     String severity = RulePriority.valueOf(def.severity()).name();

sonar-server/src/test/java/org/sonar/server/rule/DeprecatedRuleDefinitionsTest.java

@@ -75,7 +75,7 @@ public class DeprecatedRuleDefinitionsTest {
     assertThat(rule.name()).isEqualTo("Constant Name");
     assertThat(rule.htmlDescription()).isEqualTo("Checks that constant names conform to the specified format");
     assertThat(rule.severity()).isEqualTo(Severity.BLOCKER);
-    assertThat(rule.engineKey()).isEqualTo("Checker/TreeWalker/ConstantName");
+    assertThat(rule.internalKey()).isEqualTo("Checker/TreeWalker/ConstantName");
     assertThat(rule.status()).isEqualTo(RuleStatus.BETA);
     assertThat(rule.tags()).isEmpty();
     assertThat(rule.params()).hasSize(1);

sonar-server/src/test/java/org/sonar/server/rule/RuleRegistrationTest.java

@@ -217,7 +217,7 @@ public class RuleRegistrationTest extends AbstractDaoTestCase {
           .setName("One")
           .setHtmlDescription("Description of One")
           .setSeverity(Severity.BLOCKER)
-          .setEngineKey("config1")
+          .setInternalKey("config1")
           .setTags("tag1", "tag3", "tag5");
       rule1.newParam("param1").setDescription("parameter one").setDefaultValue("default value one");
       rule1.newParam("param2").setDescription("parameter two").setDefaultValue("default value two");
@@ -242,7 +242,7 @@ public class RuleRegistrationTest extends AbstractDaoTestCase {
             .setName("name of " + i)
             .setHtmlDescription("description of " + i)
             .setSeverity(Severity.BLOCKER)
-            .setEngineKey("config1")
+            .setInternalKey("config1")
             .setTags("tag1", "tag3", "tag5");
         for (int j = 0; j < 20; j++) {
           rule.newParam("param" + j);