Checkstyle: fix export/import of duplicated rules (for example RegexpSingleline)

plugins/sonar-checkstyle-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileExporter.java

@@ -29,7 +29,6 @@ import org.sonar.api.profiles.ProfileExporter;
 import org.sonar.api.profiles.RulesProfile;
 import org.sonar.api.resources.Java;
 import org.sonar.api.rules.ActiveRule;
-import org.sonar.api.rules.ActiveRuleParam;
 import org.sonar.api.rules.RuleParam;
 import org.sonar.api.utils.SonarException;
 
@@ -94,7 +93,7 @@ public class CheckstyleProfileExporter extends ProfileExporter {
       if (!isInTreeWalker(configKey)) {
         List<ActiveRule> activeRules = activeRulesByConfigKey.get(configKey);
         for (ActiveRule activeRule : activeRules) {
-          appendModule(writer, activeRule, activeRules.size() > 1);
+          appendModule(writer, activeRule);
         }
       }
     }
@@ -107,7 +106,7 @@ public class CheckstyleProfileExporter extends ProfileExporter {
       if (isInTreeWalker(configKey)) {
         List<ActiveRule> activeRules = activeRulesByConfigKey.get(configKey);
         for (ActiveRule activeRule : activeRules) {
-          appendModule(writer, activeRule, activeRules.size() > 1);
+          appendModule(writer, activeRule);
         }
       }
     }
@@ -132,12 +131,12 @@ public class CheckstyleProfileExporter extends ProfileExporter {
     return result;
   }
 
-  private void appendModule(Writer writer, ActiveRule activeRule, boolean manyInstances) throws IOException {
+  private void appendModule(Writer writer, ActiveRule activeRule) throws IOException {
     String moduleName = StringUtils.substringAfterLast(activeRule.getConfigKey(), "/");
     writer.append("<module name=\"");
     StringEscapeUtils.escapeXml(writer, moduleName);
     writer.append("\">");
-    if (manyInstances) {
+    if (activeRule.getRule().getParent() != null) {
       appendModuleProperty(writer, "id", activeRule.getRuleKey());
     }
     appendModuleProperty(writer, "severity", CheckstyleSeverityUtils.toSeverity(activeRule.getPriority()));
@@ -164,5 +163,5 @@ public class CheckstyleProfileExporter extends ProfileExporter {
     }
   }
 
-  
+
 }

plugins/sonar-checkstyle-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java

@@ -19,6 +19,7 @@
  */
 package org.sonar.plugins.checkstyle;
 
+import com.google.common.collect.Maps;
 import org.apache.commons.lang.StringUtils;
 import org.codehaus.stax2.XMLInputFactory2;
 import org.codehaus.staxmate.SMInputFactory;
@@ -36,6 +37,7 @@ import org.sonar.api.utils.ValidationMessages;
 import javax.xml.stream.XMLInputFactory;
 import javax.xml.stream.XMLStreamException;
 import java.io.Reader;
+import java.util.Map;
 
 public class CheckstyleProfileImporter extends ProfileImporter {
 
@@ -86,22 +88,12 @@ public class CheckstyleProfileImporter extends ProfileImporter {
   }
 
   private void processModule(RulesProfile profile, String path, SMInputCursor moduleCursor, ValidationMessages messages) throws XMLStreamException {
-    String configKey = moduleCursor.getAttrValue("name");
-    if (isFilter(configKey)) {
-      messages.addWarningText("Checkstyle filters are not imported: " + configKey);
+    String moduleName = moduleCursor.getAttrValue("name");
+    if (isFilter(moduleName)) {
+      messages.addWarningText("Checkstyle filters are not imported: " + moduleName);
 
-    } else if (isIgnored(configKey)) {
-      // ignore !
-
-    } else {
-      Rule rule = ruleFinder.find(RuleQuery.create().withRepositoryKey(CheckstyleConstants.REPOSITORY_KEY).withConfigKey(path + configKey));
-      if (rule == null) {
-        messages.addWarningText("Checkstyle rule not found: " + path + configKey);
-        
-      } else {
-        ActiveRule activeRule = profile.activateRule(rule, null);
-        processProperties(moduleCursor, messages, activeRule);
-      }
+    } else if (!isIgnored(moduleName)) {
+      processRule(profile, path, moduleName, moduleCursor, messages);
     }
   }
 
@@ -116,24 +108,50 @@ public class CheckstyleProfileImporter extends ProfileImporter {
         StringUtils.equals(configKey, "SuppressWithNearbyCommentFilter");
   }
 
-  private void processProperties(SMInputCursor moduleCursor, ValidationMessages messages, ActiveRule activeRule) throws XMLStreamException {
+  private void processRule(RulesProfile profile, String path, String moduleName, SMInputCursor moduleCursor, ValidationMessages messages) throws XMLStreamException {
+    Map<String, String> properties = processProps(moduleCursor);
+
+    Rule rule;
+    String id = properties.get("id");
+    String warning;
+    if (StringUtils.isNotBlank(id)) {
+      rule = ruleFinder.find(RuleQuery.create().withRepositoryKey(CheckstyleConstants.REPOSITORY_KEY).withKey(id));
+      warning = "Checkstyle rule with key '" + id + "' not found";
+
+    } else {
+      String configKey = path + moduleName;
+      rule = ruleFinder.find(RuleQuery.create().withRepositoryKey(CheckstyleConstants.REPOSITORY_KEY).withConfigKey(configKey));
+      warning = "Checkstyle rule with config key '" + configKey + "' not found";
+    }
+
+    if (rule == null) {
+      messages.addWarningText(warning);
+
+    } else {
+      ActiveRule activeRule = profile.activateRule(rule, null);
+      activateProperties(activeRule, properties);
+    }
+  }
+
+  private Map<String, String> processProps(SMInputCursor moduleCursor) throws XMLStreamException {
+    Map<String, String> props = Maps.newHashMap();
     SMInputCursor propertyCursor = moduleCursor.childElementCursor("property");
     while (propertyCursor.getNext() != null) {
-      processProperty(activeRule, propertyCursor, messages);
+      String key = propertyCursor.getAttrValue("name");
+      String value = propertyCursor.getAttrValue("value");
+      props.put(key, value);
     }
+    return props;
   }
 
-  private void processProperty(ActiveRule activeRule, SMInputCursor propertyCursor, ValidationMessages messages) throws XMLStreamException {
-    String key = propertyCursor.getAttrValue("name");
-    String value = propertyCursor.getAttrValue("value");
-    if (StringUtils.equals("id", key)) {
-      messages.addWarningText("The property 'id' is not supported in the Checkstyle rule: " + activeRule.getConfigKey());
-
-    } else if (StringUtils.equals("severity", key)) {
-      activeRule.setPriority(CheckstyleSeverityUtils.fromSeverity(value));
+  private void activateProperties(ActiveRule activeRule, Map<String, String> properties) {
+    for (Map.Entry<String, String> property : properties.entrySet()) {
+      if (StringUtils.equals("severity", property.getKey())) {
+        activeRule.setPriority(CheckstyleSeverityUtils.fromSeverity(property.getValue()));
 
-    } else {
-      activeRule.setParameter(key, value);
+      } else if (!StringUtils.equals("id", property.getKey())) {
+        activeRule.setParameter(property.getKey(), property.getValue());
+      }
     }
   }
 }

plugins/sonar-checkstyle-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleProfileExporterTest.java

@@ -85,7 +85,7 @@ public class CheckstyleProfileExporterTest {
   public void addTheIdPropertyWhenManyInstancesWithTheSameConfigKey() throws IOException, SAXException {
     RulesProfile profile = RulesProfile.create("sonar way", "java");
     Rule rule1 = Rule.create("checkstyle", "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck", "Javadoc").setConfigKey("Checker/JavadocPackage");
-    Rule rule2 = Rule.create("checkstyle", "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345", "Javadoc").setConfigKey("Checker/JavadocPackage");
+    Rule rule2 = Rule.create("checkstyle", "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345", "Javadoc").setConfigKey("Checker/JavadocPackage").setParent(rule1);
 
     profile.activateRule(rule1, RulePriority.MAJOR);
     profile.activateRule(rule2, RulePriority.CRITICAL);

plugins/sonar-checkstyle-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest.java

@@ -19,6 +19,7 @@
  */
 package org.sonar.plugins.checkstyle;
 
+import org.apache.commons.lang.StringUtils;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.invocation.InvocationOnMock;
@@ -33,11 +34,8 @@ import java.io.StringReader;
 
 import static org.hamcrest.core.Is.is;
 import static org.hamcrest.core.IsNull.nullValue;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
+import static org.junit.Assert.*;
 import static org.mockito.Matchers.anyObject;
-import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -98,20 +96,29 @@ public class CheckstyleProfileImporterTest {
     Reader reader = new StringReader(TestUtils.getResourceContent("/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest/simple.xml"));
     RulesProfile profile = importer.importProfile(reader, messages);
 
-    ActiveRule activeRule= profile.getActiveRuleByConfigKey("checkstyle", "Checker/TreeWalker/EqualsHashCode");
+    ActiveRule activeRule = profile.getActiveRuleByConfigKey("checkstyle", "Checker/TreeWalker/EqualsHashCode");
     assertThat(activeRule.getPriority(), is(RulePriority.BLOCKER)); // reuse the rule default priority
   }
 
   @Test
-  public void idPropertyIsNotSupported() {
-    Reader reader = new StringReader(TestUtils.getResourceContent("/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest/idProperty.xml"));
+  public void idPropertyShouldBeTheRuleKey() {
+    Reader reader = new StringReader(TestUtils.getResourceContent("/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest/idPropertyShouldBeTheRuleKey.xml"));
     RulesProfile profile = importer.importProfile(reader, messages);
 
-    ActiveRule check = profile.getActiveRuleByConfigKey("checkstyle", "Checker/JavadocPackage");
-    assertThat(check.getParameter("id"), nullValue());
+    assertNull(profile.getActiveRuleByConfigKey("checkstyle", "Checker/JavadocPackage"));
     assertThat(messages.getWarnings().size(), is(1));
   }
 
+  @Test
+  public void shouldUseTheIdPropertyToFindRule() {
+    Reader reader = new StringReader(TestUtils.getResourceContent("/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest/shouldUseTheIdPropertyToFindRule.xml"));
+    RulesProfile profile = importer.importProfile(reader, messages);
+
+    assertNotNull(profile.getActiveRuleByConfigKey("checkstyle", "Checker/JavadocPackage"));
+    assertThat(profile.getActiveRuleByConfigKey("checkstyle", "Checker/JavadocPackage").getRule().getKey(), is("com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345"));
+    assertThat(messages.getWarnings().size(), is(0));
+  }
+
   @Test
   public void testUnvalidXML() {
     Reader reader = new StringReader("not xml");
@@ -132,21 +139,28 @@ public class CheckstyleProfileImporterTest {
 
   private RuleFinder newRuleFinder() {
     RuleFinder ruleFinder = mock(RuleFinder.class);
-    when(ruleFinder.find((RuleQuery)anyObject())).thenAnswer(new Answer<Rule>() {
+    when(ruleFinder.find((RuleQuery) anyObject())).thenAnswer(new Answer<Rule>() {
       public Rule answer(InvocationOnMock iom) throws Throwable {
-        RuleQuery query = (RuleQuery)iom.getArguments()[0];
+        RuleQuery query = (RuleQuery) iom.getArguments()[0];
         Rule rule = null;
-        if (query.getConfigKey().equals("Checker/JavadocPackage")) {
+        if (StringUtils.equals(query.getConfigKey(), "Checker/JavadocPackage")) {
           rule = Rule.create(query.getRepositoryKey(), "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck", "Javadoc Package")
               .setConfigKey("Checker/JavadocPackage")
               .setPriority(RulePriority.MAJOR);
           rule.createParameter("format");
           rule.createParameter("ignore");
 
-        } else if (query.getConfigKey().equals("Checker/TreeWalker/EqualsHashCode")) {
+        } else if (StringUtils.equals(query.getConfigKey(), "Checker/TreeWalker/EqualsHashCode")) {
           rule = Rule.create(query.getRepositoryKey(), "com.puppycrawl.tools.checkstyle.checks.coding.EqualsHashCodeCheck", "Equals HashCode")
               .setConfigKey("Checker/TreeWalker/EqualsHashCode")
               .setPriority(RulePriority.BLOCKER);
+
+        } else if (StringUtils.equals(query.getKey(), "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345")) {
+          rule = Rule.create(query.getRepositoryKey(), "com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345", "Javadoc Package")
+              .setConfigKey("Checker/JavadocPackage")
+              .setPriority(RulePriority.MAJOR);
+          rule.createParameter("format");
+          rule.createParameter("ignore");
         }
         return rule;
       }

plugins/sonar-checkstyle-plugin/src/test/resources/org/sonar/plugins/checkstyle/CheckstyleProfileExporterTest/addTheIdPropertyWhenManyInstancesWithTheSameConfigKey.xml

@@ -3,7 +3,6 @@
 <module name="Checker">
     <module name="SuppressionCommentFilter"/> 
     <module name="JavadocPackage">
-        <property name="id" value="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck" />
         <property name="severity" value="warning" />
     </module>
     <module name="JavadocPackage">

plugins/sonar-checkstyle-plugin/src/test/resources/org/sonar/plugins/checkstyle/CheckstyleProfileExporterTest/exportParameters.xml

@@ -1,12 +1,12 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- Generated by Sonar -->
 <module name="Checker">
-    <module name="SuppressionCommentFilter"/> 
-    <module name="JavadocPackage">
-        <property name="severity" value="warning" />
-        <property name="format" value="abcde" />
-    </module>
-    <module name="TreeWalker">
-        <module name="FileContentsHolder"/>
-    </module>
+  <module name="SuppressionCommentFilter"/>
+  <module name="JavadocPackage">
+    <property name="severity" value="warning"/>
+    <property name="format" value="abcde"/>
+  </module>
+  <module name="TreeWalker">
+    <module name="FileContentsHolder"/>
+  </module>
 </module>

plugins/sonar-checkstyle-plugin/src/test/resources/org/sonar/plugins/checkstyle/CheckstyleProfileExporterTest/singleCheckstyleRulesToExport.xml

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- Generated by Sonar -->
 <module name="Checker">
-    <module name="SuppressionCommentFilter"/>
-    <module name="JavadocPackage">
-        <property name="severity" value="warning"/>
+  <module name="SuppressionCommentFilter"/>
+  <module name="JavadocPackage">
+    <property name="severity" value="warning"/>
+  </module>
+  <module name="TreeWalker">
+    <module name="FileContentsHolder"/>
+    <module name="LocalFinalVariableName">
+      <property name="severity" value="info"/>
     </module>
-    <module name="TreeWalker">
-        <module name="FileContentsHolder"/>
-        <module name="LocalFinalVariableName">
-            <property name="severity" value="info"/>
-        </module>
-        <module name="LineLength">
-            <property name="severity" value="error"/>
-        </module>
+    <module name="LineLength">
+      <property name="severity" value="error"/>
     </module>
+  </module>
 </module>

plugins/sonar-checkstyle-plugin/src/test/resources/org/sonar/plugins/checkstyle/CheckstyleProfileImporterTest/shouldUseTheIdPropertyToFindRule.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.2//EN" "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">
+<!-- Generated by Sonar -->
+<module name="Checker">
+    <module name="JavadocPackage">
+        <property name="id" value="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck_12345" />      
+        <property name="severity" value="error" />
+        <property name="format" value="abcde" />
+        <property name="ignore" value="true" />
+    </module>
+</module>