From 1df34b6f6bbfe507146a534f869377ea587d4336 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 15 Mar 2013 16:02:06 +0100 Subject: [PATCH] SONAR-4206 Refactor rules registration in order to set the removed status only when a rule was removed --- .../sonar/server/startup/RegisterRules.java | 165 ++++++++++++------ .../app/controllers/profiles_controller.rb | 6 +- .../server/startup/RegisterRulesTest.java | 22 +++ .../notUpdateAlreadyDisabledRule.xml | 26 +++ .../reactivateDisabledRules.xml | 26 +++ .../startup/RegisterRulesTest/shared.xml | 22 ++- 6 files changed, 204 insertions(+), 63 deletions(-) create mode 100644 sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/notUpdateAlreadyDisabledRule.xml create mode 100644 sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/reactivateDisabledRules.xml 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 f083efb4c8b..7cf0967532c 100644 --- 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 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 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 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 rulesByKey = Maps.newHashMap(); + private void registerRepository(RuleRepository repository, RulesByRepository existingRules, DatabaseSession session) { + Map 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 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 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 repositories, DatabaseSession session) { + for(final String repositoryKey : existingRules.repositories()) { + if (!Iterables.any(repositories, new Predicate() { + 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 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 ruleRepositoryList; + + public RulesByRepository(List rules){ + ruleRepositoryList = ArrayListMultimap.create(); + for (Rule rule :rules) { + ruleRepositoryList.put(rule.getRepositoryKey(), rule); + } + } + + public Collection get(String repositoryKey) { + return ruleRepositoryList.get(repositoryKey); + } + + public Collection 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 54c62f613bf..d78f616f310 100644 --- 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 ? 'id desc') + @changes = ActiveRuleChange.all(:conditions => ['profile_id=? and ? '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 9e043276d14..c291e0c0246 100644 --- 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 index 00000000000..f3d63ec4212 --- /dev/null +++ b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/notUpdateAlreadyDisabledRule.xml @@ -0,0 +1,26 @@ + + + + + + + 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 index 00000000000..66eb56fa7d7 --- /dev/null +++ b/sonar-server/src/test/resources/org/sonar/server/startup/RegisterRulesTest/reactivateDisabledRules.xml @@ -0,0 +1,26 @@ + + + + + + + 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 a825a3c4bbb..8f917e3a37d 100644 --- 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 @@ + + - - \ No newline at end of file -- 2.39.5