SONAR-4206 Refactor rules registration in order to set the removed status only when...

diff --git a/sonar-server/src/main/java/org/sonar/server/startup/RegisterRules.java b/sonar-server/src/main/java/org/sonar/server/startup/RegisterRules.java
index f083efb4c8b2b82ffb1bc1387706c281375aa520..7cf0967532c990b6af1df2bcc286fe595e0a6296 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/startup/RegisterRules.java
+++ b/sonar-server/src/main/java/org/sonar/server/startup/RegisterRules.java
@@ -21,9 +21,12 @@
 package org.sonar.server.startup;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -39,9 +42,6 @@ import org.sonar.core.i18n.RuleI18nManager;
 import org.sonar.core.rule.RuleStatus;
 import org.sonar.jpa.session.DatabaseSessionFactory;
 
-import javax.persistence.Query;
-
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Date;
 import java.util.Iterator;
@@ -49,16 +49,19 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 
+import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Maps.newHashMap;
+
 public final class RegisterRules {
 
-  private static final Logger LOG = LoggerFactory.getLogger(RegisterRules.class);
+  private static final Logger LOG = LoggerFactory.getLogger(RegisterRules2.class);
   private final DatabaseSessionFactory sessionFactory;
   private final List<RuleRepository> repositories;
   private final RuleI18nManager ruleI18nManager;
 
   public RegisterRules(DatabaseSessionFactory sessionFactory, RuleRepository[] repos, RuleI18nManager ruleI18nManager) {
     this.sessionFactory = sessionFactory;
-    this.repositories = Arrays.asList(repos);
+    this.repositories = newArrayList(repos);
     this.ruleI18nManager = ruleI18nManager;
   }
 
@@ -68,71 +71,54 @@ public final class RegisterRules {
 
   public void start() {
     TimeProfiler profiler = new TimeProfiler();
-
     DatabaseSession session = sessionFactory.getSession();
-    disableAllRules(session);
-    for (RuleRepository repository : repositories) {
-      profiler.start("Register rules [" + repository.getKey() + "/" + StringUtils.defaultString(repository.getLanguage(), "-") + "]");
-      registerRepository(repository, session);
-      profiler.stop();
-    }
+    RulesByRepository existingRules = new RulesByRepository(findAllRules(session));
 
-    profiler.start("Disable deprecated user rules");
-    disableDeprecatedUserRules(session);
-    profiler.stop();
+    registerRules(existingRules, profiler, session);
+    disableDeprecatedRepositories(existingRules, repositories, session);
+    disableDeprecatedUserRules(profiler, session);
 
     session.commit();
   }
 
-  private void disableDeprecatedUserRules(DatabaseSession session) {
-    List<Integer> deprecatedUserRuleIds = Lists.newLinkedList();
-    deprecatedUserRuleIds.addAll(session.createQuery(
-        "SELECT r.id FROM " + Rule.class.getSimpleName() +
-            " r WHERE r.parent IS NOT NULL AND NOT EXISTS(FROM " + Rule.class.getSimpleName() + " p WHERE r.parent=p)").getResultList());
-
-    deprecatedUserRuleIds.addAll(session.createQuery(
-        "SELECT r.id FROM " + Rule.class.getSimpleName() +
-            " r WHERE r.parent IS NOT NULL AND EXISTS(FROM " + Rule.class.getSimpleName() + " p WHERE r.parent=p and p.status=:status)")
-        .setParameter("status", RuleStatus.REMOVED.name())
-        .getResultList());
-
-    for (Integer deprecatedUserRuleId : deprecatedUserRuleIds) {
-      Rule rule = session.getSingleResult(Rule.class, "id", deprecatedUserRuleId);
-      rule.setStatus(RuleStatus.REMOVED.name());
-      rule.setUpdatedAt(new Date());
-      session.saveWithoutFlush(rule);
+  private void registerRules(RulesByRepository existingRules, TimeProfiler profiler, DatabaseSession session){
+    for (RuleRepository repository : repositories) {
+      profiler.start("Register rules [" + repository.getKey() + "/" + StringUtils.defaultString(repository.getLanguage(), "-") + "]");
+      registerRepository(repository, existingRules, session);
+      profiler.stop();
     }
   }
 
-  private void disableAllRules(DatabaseSession session) {
+  private List<Rule> findAllRules(DatabaseSession session) {
     // the hardcoded repository "manual" is used for manual violations
-    Query query = session.createQuery(
-        "UPDATE " + Rule.class.getSimpleName() + " SET status=:status, updated_at=current_timestamp WHERE parent IS NULL AND pluginName<>'manual' AND status<>:status");
-    query.setParameter("status", RuleStatus.REMOVED.name());
-    query.executeUpdate();
+    return session.createQuery("from " + Rule.class.getSimpleName() + " r WHERE r.parent IS NULL AND r.pluginName<>:repository")
+        .setParameter("repository", "manual")
+      .getResultList();
   }
 
-  private void registerRepository(RuleRepository repository, DatabaseSession session) {
-    Map<String, Rule> rulesByKey = Maps.newHashMap();
+  private void registerRepository(RuleRepository repository, RulesByRepository existingRules, DatabaseSession session) {
+    Map<String, Rule> ruleByKey = newHashMap();
     for (Rule rule : repository.createRules()) {
-      validateRule(rule, repository.getKey());
       rule.setRepositoryKey(repository.getKey());
       rule.setLanguage(repository.getLanguage());
       rule.setStatus(!Strings.isNullOrEmpty(rule.getStatus()) ? rule.getStatus() : RuleStatus.defaultValue().name());
-      rulesByKey.put(rule.getKey(), rule);
+      validateRule(rule, repository.getKey());
+      ruleByKey.put(rule.getKey(), rule);
     }
-    LOG.debug(rulesByKey.size() + " rules");
+    LOG.debug(ruleByKey.size() + " rules");
 
-    List<Rule> persistedRules = session.getResults(Rule.class, "pluginName", repository.getKey());
-    for (Rule persistedRule : persistedRules) {
-      Rule rule = rulesByKey.get(persistedRule.getKey());
+    for (Rule persistedRule : existingRules.get(repository.getKey())) {
+      Rule rule = ruleByKey.get(persistedRule.getKey());
       if (rule != null) {
         updateRule(persistedRule, rule, session);
-        rulesByKey.remove(rule.getKey());
+        ruleByKey.remove(rule.getKey());
+      } else {
+        disable(persistedRule);
       }
+      session.saveWithoutFlush(persistedRule);
     }
 
-    saveNewRules(rulesByKey.values(), session);
+    saveNewRules(ruleByKey.values(), session);
   }
 
   private void validateRule(Rule rule, String repositoryKey) {
@@ -141,13 +127,13 @@ public final class RegisterRules {
     validateStatus(rule);
   }
 
-  private void validateRuleRepositoryName(Rule rule, String repositoryKey){
+  private void validateRuleRepositoryName(Rule rule, String repositoryKey) {
     if (Strings.isNullOrEmpty(rule.getName()) && Strings.isNullOrEmpty(ruleI18nManager.getName(repositoryKey, rule.getKey(), Locale.ENGLISH))) {
       throw new SonarException("The following rule (repository: " + repositoryKey + ") must have a name: " + rule);
     }
   }
 
-  private void validateRuleDescription(Rule rule, String repositoryKey){
+  private void validateRuleDescription(Rule rule, String repositoryKey) {
     if (Strings.isNullOrEmpty(rule.getDescription()) && Strings.isNullOrEmpty(ruleI18nManager.getDescription(repositoryKey, rule.getKey(), Locale.ENGLISH))) {
       if (!Strings.isNullOrEmpty(rule.getName()) && Strings.isNullOrEmpty(ruleI18nManager.getName(repositoryKey, rule.getKey(), Locale.ENGLISH))) {
         // specific case
@@ -160,16 +146,16 @@ public final class RegisterRules {
   }
 
   private void validateStatus(Rule rule) {
-    if (!Strings.isNullOrEmpty(rule.getStatus())) {
-      try {
-        Status.valueOf(rule.getStatus());
-      } catch (IllegalArgumentException e) {
-        throw new SonarException("The status of a rule can only contains : " + Joiner.on(", ").join(Status.values()), e);
-      }
+    try {
+      Status.valueOf(rule.getStatus());
+    } catch (IllegalArgumentException e) {
+      throw new SonarException("The status of a rule can only contains : " + Joiner.on(", ").join(Status.values()), e);
     }
   }
 
   private void updateRule(Rule persistedRule, Rule rule, DatabaseSession session) {
+    LOG.debug("Update rule " + rule);
+
     persistedRule.setName(rule.getName());
     persistedRule.setConfigKey(rule.getConfigKey());
     persistedRule.setDescription(rule.getDescription());
@@ -184,8 +170,6 @@ public final class RegisterRules {
 
     // add new params and update existing params
     updateParameters(persistedRule, rule);
-
-    session.saveWithoutFlush(persistedRule);
   }
 
   private void updateParameters(Rule persistedRule, Rule rule) {
@@ -219,9 +203,74 @@ public final class RegisterRules {
 
   private void saveNewRules(Collection<Rule> rules, DatabaseSession session) {
     for (Rule rule : rules) {
+      LOG.debug("Save new rule " + rule);
       rule.setCreatedAt(new Date());
       session.saveWithoutFlush(rule);
     }
   }
 
+  private void disableDeprecatedRepositories(RulesByRepository existingRules, List<RuleRepository> repositories, DatabaseSession session) {
+    for(final String repositoryKey : existingRules.repositories()) {
+      if (!Iterables.any(repositories, new Predicate<RuleRepository>() {
+        public boolean apply(RuleRepository input) {
+          return input.getKey().equals(repositoryKey);
+        }
+      })) {
+        for (Rule rule : existingRules.get(repositoryKey)) {
+          disable(rule);
+          session.saveWithoutFlush(rule);
+        }
+      }
+    }
+  }
+
+  private void disableDeprecatedUserRules(TimeProfiler profiler, DatabaseSession session) {
+    profiler.start("Disable deprecated user rules");
+    List<Integer> deprecatedUserRuleIds = Lists.newLinkedList();
+    deprecatedUserRuleIds.addAll(session.createQuery(
+        "SELECT r.id FROM " + Rule.class.getSimpleName() +
+            " r WHERE r.parent IS NOT NULL AND NOT EXISTS(FROM " + Rule.class.getSimpleName() + " p WHERE r.parent=p)").getResultList());
+
+    deprecatedUserRuleIds.addAll(session.createQuery(
+        "SELECT r.id FROM " + Rule.class.getSimpleName() +
+            " r WHERE r.parent IS NOT NULL AND EXISTS(FROM " + Rule.class.getSimpleName() + " p WHERE r.parent=p and p.status=:status)")
+        .setParameter("status", RuleStatus.REMOVED.name())
+        .getResultList());
+
+    for (Integer deprecatedUserRuleId : deprecatedUserRuleIds) {
+      Rule rule = session.getSingleResult(Rule.class, "id", deprecatedUserRuleId);
+      disable(rule);
+      session.saveWithoutFlush(rule);
+    }
+    profiler.stop();
+  }
+
+  private void disable(Rule rule){
+    if (!rule.getStatus().equals(RuleStatus.REMOVED.name())) {
+      LOG.debug("Disable rule " + rule);
+      rule.setStatus(RuleStatus.REMOVED.name());
+      rule.setUpdatedAt(new Date());
+    }
+  }
+
+  class RulesByRepository {
+    Multimap<String, Rule> ruleRepositoryList;
+
+    public RulesByRepository(List<Rule> rules){
+      ruleRepositoryList = ArrayListMultimap.create();
+      for (Rule rule :rules) {
+        ruleRepositoryList.put(rule.getRepositoryKey(), rule);
+      }
+    }
+
+    public Collection<Rule> get(String repositoryKey) {
+      return ruleRepositoryList.get(repositoryKey);
+    }
+
+    public Collection<String> repositories(){
+      return ruleRepositoryList.keySet();
+    }
+
+  }
+
 }

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/profiles_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/profiles_controller.rb
index 54c62f613bf803d4e17253ae5260393f90d66f40..d78f616f3108c51ba9bfc8c8ad1a8c88e2387b40 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/profiles_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/profiles_controller.rb
@@ -180,7 +180,7 @@ class ProfilesController < ApplicationController
     require_parameters 'id'
     @profile = Profile.find(params[:id])
 
-    profiles=Profile.find(:all, :conditions => ['language=? and id<>? and (parent_name is null or parent_name<>?)', @profile.language, @profile.id, @profile.name], :order => 'name')
+    profiles=Profile.all(:conditions => ['language=? and id<>? and (parent_name is null or parent_name<>?)', @profile.language, @profile.id, @profile.name], :order => 'name')
     @select_parent = [[message('none'), nil]] + profiles.collect { |profile| [profile.name, profile.name] }
 
     set_profile_breadcrumbs
@@ -191,7 +191,7 @@ class ProfilesController < ApplicationController
     require_parameters 'id'
     @profile = Profile.find(params[:id])
 
-    versions = ActiveRuleChange.find(:all, :select => 'profile_version, MAX(change_date) AS change_date', :conditions => ['profile_id=?', @profile.id], :group => 'profile_version')
+    versions = ActiveRuleChange.all(:select => 'profile_version, MAX(change_date) AS change_date', :conditions => ['profile_id=?', @profile.id], :group => 'profile_version')
     versions.sort! { |a, b| b.profile_version <=> a.profile_version }
 
     if !versions.empty?
@@ -209,7 +209,7 @@ class ProfilesController < ApplicationController
       if @since_version > @to_version
         @since_version, @to_version = @to_version, @since_version
       end
-      @changes = ActiveRuleChange.find(:all, :conditions => ['profile_id=? and ?<profile_version and profile_version<=?', @profile.id, @since_version, @to_version], :order => 'id desc')
+      @changes = ActiveRuleChange.all(:conditions => ['profile_id=? and ?<profile_version and profile_version<=?', @profile.id, @since_version, @to_version], :order => 'id desc')
 
       @select_versions = versions.map { |u| [message(u.profile_version == last_version ? 'quality_profiles.last_version_x_with_date' : 'quality_profiles.version_x_with_date', :params => [u.profile_version.to_s, l(u.change_date)]), u.profile_version] } | [[message('quality_profiles.no_version'), 0]];
     end

diff --git a/sonar-server/src/test/java/org/sonar/server/startup/RegisterRulesTest.java b/sonar-server/src/test/java/org/sonar/server/startup/RegisterRulesTest.java
index 9e043276d1482021fba0d4ee3c6994bd8a980510..c291e0c0246f310ae9ffed8aa39930cbed4ebcb5 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/startup/RegisterRulesTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/startup/RegisterRulesTest.java
@@ -17,6 +17,7 @@
  * License along with Sonar; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
  */
+
 package org.sonar.server.startup;
 
 import org.junit.Before;
@@ -41,6 +42,7 @@ import java.util.Locale;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.number.OrderingComparisons.greaterThan;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -93,6 +95,26 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase {
     }
   }
 
+  @Test
+  public void should_reactivate_disabled_rules() {
+    setupData("reactivateDisabledRules");
+    task.start();
+
+    Rule rule = getSession().getSingleResult(Rule.class, "id", 1);
+    assertThat(rule.getStatus(), is(RuleStatus.READY.name()));
+    assertThat(rule.getUpdatedAt(), notNullValue());
+  }
+
+  @Test
+  public void should_not_update_already_disabled_rules() {
+    setupData("notUpdateAlreadyDisabledRule");
+    task.start();
+
+    Rule rule = getSession().getSingleResult(Rule.class, "id", 1);
+    assertThat(rule.getStatus(), is(RuleStatus.REMOVED.name()));
+    assertThat(rule.getUpdatedAt(), nullValue());
+  }
+
   @Test
   public void should_disable_deprecated_active_rules() {
     setupData("disableDeprecatedActiveRules");

diff --git a/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/notUpdateAlreadyDisabledRule.xml b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/notUpdateAlreadyDisabledRule.xml
new file mode 100644 (file)
index 0000000..f3d63ec
--- /dev/null
+++ b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/notUpdateAlreadyDisabledRule.xml
@@ -0,0 +1,26 @@
+<!--
+  ~ Sonar, open source software quality management tool.
+  ~ Copyright (C) 2008-2012 SonarSource
+  ~ mailto:contact AT sonarsource DOT com
+  ~
+  ~ Sonar is free software; you can redistribute it and/or
+  ~ modify it under the terms of the GNU Lesser General Public
+  ~ License as published by the Free Software Foundation; either
+  ~ version 3 of the License, or (at your option) any later version.
+  ~
+  ~ Sonar is distributed in the hope that it will be useful,
+  ~ but WITHOUT ANY WARRANTY; without even the implied warranty of
+  ~ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  ~ Lesser General Public License for more details.
+  ~
+  ~ You should have received a copy of the GNU Lesser General Public
+  ~ License along with Sonar; if not, write to the Free Software
+  ~ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+  -->
+
+<dataset>
+
+  <rules id="1" plugin_rule_key="deprecated-key" plugin_name="deprecated-repo" plugin_config_key="[null]" name="Deprecated" description="[null]"
+         status="REMOVED" priority="4" cardinality="SINGLE" parent_id="[null]"/>
+
+</dataset>

diff --git a/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/reactivateDisabledRules.xml b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/reactivateDisabledRules.xml
new file mode 100644 (file)
index 0000000..66eb56f
--- /dev/null
+++ b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/reactivateDisabledRules.xml
@@ -0,0 +1,26 @@
+<!--
+  ~ Sonar, open source software quality management tool.
+  ~ Copyright (C) 2008-2012 SonarSource
+  ~ mailto:contact AT sonarsource DOT com
+  ~
+  ~ Sonar is free software; you can redistribute it and/or
+  ~ modify it under the terms of the GNU Lesser General Public
+  ~ License as published by the Free Software Foundation; either
+  ~ version 3 of the License, or (at your option) any later version.
+  ~
+  ~ Sonar is distributed in the hope that it will be useful,
+  ~ but WITHOUT ANY WARRANTY; without even the implied warranty of
+  ~ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  ~ Lesser General Public License for more details.
+  ~
+  ~ You should have received a copy of the GNU Lesser General Public
+  ~ License along with Sonar; if not, write to the Free Software
+  ~ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+  -->
+
+<dataset>
+
+  <rules id="1" plugin_rule_key="rule1" plugin_name="fake" plugin_config_key="config1" name="One" description="Description of One"
+         status="REMOVED" priority="4" cardinality="SINGLE" parent_id="[null]"/>
+
+</dataset>

diff --git a/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/shared.xml b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/shared.xml
index a825a3c4bbb22512430ff29375ec7e56c4003d85..8f917e3a37d8ef441f3d761d22240420cfeb6908 100644 (file)
--- a/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/shared.xml
+++ b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/shared.xml
@@ -1,8 +1,26 @@
+<!--
+  ~ Sonar, open source software quality management tool.
+  ~ Copyright (C) 2008-2012 SonarSource
+  ~ mailto:contact AT sonarsource DOT com
+  ~
+  ~ Sonar is free software; you can redistribute it and/or
+  ~ modify it under the terms of the GNU Lesser General Public
+  ~ License as published by the Free Software Foundation; either
+  ~ version 3 of the License, or (at your option) any later version.
+  ~
+  ~ Sonar is distributed in the hope that it will be useful,
+  ~ but WITHOUT ANY WARRANTY; without even the implied warranty of
+  ~ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  ~ Lesser General Public License for more details.
+  ~
+  ~ You should have received a copy of the GNU Lesser General Public
+  ~ License along with Sonar; if not, write to the Free Software
+  ~ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+  -->
+
 <dataset>
 
   <rules id="1" plugin_rule_key="deprecated-key" plugin_name="deprecated-repo" plugin_config_key="[null]" name="Deprecated" description="[null]"
                    status="READY" priority="4" cardinality="SINGLE" parent_id="[null]"/>
 
-  <rules_parameters id="1" rule_id="1" name="deprecated-prop" description="[null]" param_type="STRING"/>
-
 </dataset>
\ No newline at end of file