From 94c5097a927988dc43d47b95f5963c3d8e1a06a3 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Mon, 11 Mar 2013 12:14:57 +0100 Subject: [PATCH] SONAR-4170 Add language property on rule in order to track removed rules from removed plugin --- .../src/main/java/org/sonar/check/Rule.java | 2 +- .../src/main/java/org/sonar/check/Status.java | 6 +- .../org/sonar/core/persistence/schema-h2.ddl | 1 + .../main/java/org/sonar/api/rules/Rule.java | 38 +++++++++++++ .../org/sonar/api/rules/XMLRuleParser.java | 16 +----- .../sonar/api/rules/XMLRuleParserTest.java | 9 +-- .../sonar/server/startup/RegisterRules.java | 23 +++++--- .../main/webapp/WEB-INF/app/models/rule.rb | 16 +++++- ...add_status_language_and_dates_to_rules.rb} | 3 +- .../server/startup/RegisterRulesTest.java | 57 +++++++++++++------ 10 files changed, 120 insertions(+), 51 deletions(-) rename sonar-server/src/main/webapp/WEB-INF/db/migrate/{380_add_status_and_dates_to_rules.rb => 380_add_status_language_and_dates_to_rules.rb} (88%) diff --git a/sonar-check-api/src/main/java/org/sonar/check/Rule.java b/sonar-check-api/src/main/java/org/sonar/check/Rule.java index 3c92139c720..8ed70868ed4 100644 --- a/sonar-check-api/src/main/java/org/sonar/check/Rule.java +++ b/sonar-check-api/src/main/java/org/sonar/check/Rule.java @@ -65,5 +65,5 @@ public @interface Rule { * The rule status. Can be Normal, Beta or Deprecated * @since 3.6 */ - Status status() default Status.NORMAL; + Status status() default Status.READY; } diff --git a/sonar-check-api/src/main/java/org/sonar/check/Status.java b/sonar-check-api/src/main/java/org/sonar/check/Status.java index e8a1fd159ee..aa5d7e22506 100644 --- a/sonar-check-api/src/main/java/org/sonar/check/Status.java +++ b/sonar-check-api/src/main/java/org/sonar/check/Status.java @@ -24,5 +24,9 @@ package org.sonar.check; * @since 3.6 */ public enum Status { - NORMAL, BETA, DEPRECATED + READY, BETA, DEPRECATED; + + public static Status defaultValue() { + return Status.READY; + } } diff --git a/sonar-core/src/main/resources/org/sonar/core/persistence/schema-h2.ddl b/sonar-core/src/main/resources/org/sonar/core/persistence/schema-h2.ddl index 168b023238a..e4629823669 100644 --- a/sonar-core/src/main/resources/org/sonar/core/persistence/schema-h2.ddl +++ b/sonar-core/src/main/resources/org/sonar/core/persistence/schema-h2.ddl @@ -155,6 +155,7 @@ CREATE TABLE "RULES" ( "PLUGIN_CONFIG_KEY" VARCHAR(500), "NAME" VARCHAR(200), "STATUS" VARCHAR(40), + "LANGUAGE" VARCHAR(20), "CREATED_AT" TIMESTAMP, "UPDATED_AT" TIMESTAMP, ); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/rules/Rule.java b/sonar-plugin-api/src/main/java/org/sonar/api/rules/Rule.java index 9c5f23cb49e..f65ced89002 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/rules/Rule.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/rules/Rule.java @@ -89,6 +89,9 @@ public final class Rule { @Column(name = "status", updatable = true, nullable = true) private String status; + @Column(name = "language", updatable = true, nullable = true) + private String language; + @ManyToOne(fetch = FetchType.EAGER) @JoinColumn(name = "parent_id", updatable = true, nullable = true) private Rule parent = null; @@ -380,33 +383,67 @@ public final class Rule { return this; } + /** + * @since 3.6 + */ public String getStatus() { return status; } + /** + * @since 3.6 + */ public Rule setStatus(String status) { this.status = status; return this; } + /** + * @since 3.6 + */ public Date getCreatedAt() { return createdAt; } + /** + * @since 3.6 + */ public Rule setCreatedAt(Date created_at) { this.createdAt = created_at; return this; } + /** + * @since 3.6 + */ public Date getUpdatedAt() { return updatedAt; } + /** + * @since 3.6 + */ public Rule setUpdatedAt(Date updatedAt) { this.updatedAt = updatedAt; return this; } + + /** + * @since 3.6 + */ + public String getLanguage() { + return language; + } + + /** + * For internal use only. + * @since 3.6 + */ + public void setLanguage(String language) { + this.language = language; + } + @Override public boolean equals(Object obj) { if (!(obj instanceof Rule)) { @@ -443,6 +480,7 @@ public final class Rule { .append("severity", priority) .append("cardinality", cardinality) .append("status", status) + .append("language", language) .toString(); } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/rules/XMLRuleParser.java b/sonar-plugin-api/src/main/java/org/sonar/api/rules/XMLRuleParser.java index d43a0e64330..19cb5876eaa 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/rules/XMLRuleParser.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/rules/XMLRuleParser.java @@ -33,7 +33,6 @@ import org.sonar.api.PropertyType; import org.sonar.api.ServerComponent; import org.sonar.api.utils.SonarException; import org.sonar.check.Cardinality; -import org.sonar.check.Status; import javax.xml.stream.XMLInputFactory; import javax.xml.stream.XMLStreamException; @@ -149,8 +148,7 @@ public final class XMLRuleParser implements ServerComponent { rule.setCardinality(Cardinality.valueOf(StringUtils.trim(cursor.collectDescendantText(false)))); } else if (StringUtils.equalsIgnoreCase("status", nodeName)) { - String value = StringUtils.trim(cursor.collectDescendantText(false)); - processStatus(rule, value); + rule.setStatus(StringUtils.trim(cursor.collectDescendantText(false))); } else if (StringUtils.equalsIgnoreCase("param", nodeName)) { processParameter(rule, cursor); @@ -225,16 +223,4 @@ public final class XMLRuleParser implements ServerComponent { throw new SonarException("Invalid property type [" + type + "]"); } - private static void processStatus(Rule rule, String value) { - try { - if (!Strings.isNullOrEmpty(value)) { - Status status = Status.valueOf(value); - rule.setStatus(status.name()); - } else { - rule.setStatus(Status.NORMAL.name()); - } - } catch (IllegalArgumentException e) { - throw new SonarException("Node can only contains : "+ Status.values(), e); - } - } } diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/rules/XMLRuleParserTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/rules/XMLRuleParserTest.java index c683949ef55..e901d88a3b1 100644 --- a/sonar-plugin-api/src/test/java/org/sonar/api/rules/XMLRuleParserTest.java +++ b/sonar-plugin-api/src/test/java/org/sonar/api/rules/XMLRuleParserTest.java @@ -123,20 +123,15 @@ public class XMLRuleParserTest { assertThat(rule.getParam("minMethodsCount"), not(nullValue())); } - @Test(expected = SonarException.class) - public void should_fail_if_status_is_unknown() { - new XMLRuleParser().parse(new StringReader("fooFooUNKNOWN")); - } - @Test public void should_read_rule_status() { List rules = new XMLRuleParser().parse(new StringReader( ""+ - "fooNORMAL"+ + "fooREADY"+ "fooBETA"+ "fooDEPRECATED"+ "")); - assertThat(rules.get(0).getStatus(), is(Status.NORMAL.name())); + assertThat(rules.get(0).getStatus(), is(Status.READY.name())); assertThat(rules.get(1).getStatus(), is(Status.BETA.name())); assertThat(rules.get(2).getStatus(), is(Status.DEPRECATED.name())); } 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 1791b36e017..72da01da89f 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 @@ -19,6 +19,7 @@ */ package org.sonar.server.startup; +import com.google.common.base.Joiner; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -47,7 +48,6 @@ import java.util.Map; public final class RegisterRules { private static final Logger LOG = LoggerFactory.getLogger(RegisterRules.class); - private final DatabaseSessionFactory sessionFactory; private final List repositories; private final RuleI18nManager ruleI18nManager; @@ -84,11 +84,11 @@ public final class RegisterRules { 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()); + " 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.enabled=false)").getResultList()); + " r WHERE r.parent IS NOT NULL AND EXISTS(FROM " + Rule.class.getSimpleName() + " p WHERE r.parent=p and p.enabled=false)").getResultList()); for (Integer deprecatedUserRuleId : deprecatedUserRuleIds) { Rule rule = session.getSingleResult(Rule.class, "id", deprecatedUserRuleId); @@ -108,6 +108,8 @@ public final class RegisterRules { 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() : Status.defaultValue().name()); rulesByKey.put(rule.getKey(), rule); } LOG.info(rulesByKey.size() + " rules"); @@ -132,11 +134,18 @@ public final class RegisterRules { if (StringUtils.isNotBlank(rule.getName()) && StringUtils.isBlank(ruleI18nManager.getName(repositoryKey, rule.getKey(), Locale.ENGLISH))) { // specific case throw new SonarException("No description found for the rule '" + rule.getName() + "' (repository: " + repositoryKey + ") because the entry 'rule." - + repositoryKey + "." + rule.getKey() + ".name' is missing from the bundle."); + + repositoryKey + "." + rule.getKey() + ".name' is missing from the bundle."); } else { throw new SonarException("The following rule (repository: " + repositoryKey + ") must have a description: " + 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); + } + } } private void updateRule(Rule persistedRule, Rule rule, DatabaseSession session) { @@ -146,7 +155,8 @@ public final class RegisterRules { persistedRule.setSeverity(rule.getSeverity()); persistedRule.setEnabled(true); persistedRule.setCardinality(rule.getCardinality()); - persistedRule.setStatus(!Strings.isNullOrEmpty(rule.getStatus()) ? rule.getStatus() : Status.NORMAL.name()); + persistedRule.setStatus(rule.getStatus()); + persistedRule.setLanguage(rule.getLanguage()); persistedRule.setUpdatedAt(new Date()); // delete deprecated params @@ -174,7 +184,7 @@ public final class RegisterRules { private void deleteDeprecatedParameters(Rule persistedRule, Rule rule, DatabaseSession session) { if (persistedRule.getParams() != null && persistedRule.getParams().size() > 0) { - for (Iterator it = persistedRule.getParams().iterator(); it.hasNext();) { + for (Iterator it = persistedRule.getParams().iterator(); it.hasNext(); ) { RuleParam persistedParam = it.next(); if (rule.getParam(persistedParam.getKey()) == null) { it.remove(); @@ -190,7 +200,6 @@ public final class RegisterRules { private void saveNewRules(Collection rules, DatabaseSession session) { for (Rule rule : rules) { rule.setEnabled(true); - rule.setStatus(Status.NORMAL.name()); rule.setCreatedAt(new Date()); session.saveWithoutFlush(rule); } diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/rule.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/rule.rb index ce8f4607f69..64782ad1d82 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/rule.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/rule.rb @@ -262,9 +262,6 @@ class Rule < ActiveRecord::Base status = options[:status] if status && !status.empty? values[:enabled] = !status.include?(STATUS_DISABLED) - - conditions << "status IN (:status)" - values[:status] = status end plugins=nil @@ -319,6 +316,19 @@ class Rule < ActiveRecord::Base priorities = remove_blank(options[:priorities]) profile = options[:profile] inheritance = options[:inheritance] + status = options[:status] + + # The status cannot be filter in the SQL query because the disabled state is not set in the status column but in the disabled column. + # For instance, a rule can be disabled and in status BETA in database, but in real it has to be considered only as disabled and must be returned when searching only for DISABLED. + if status && !status.empty? + status_list = status.split(',') + rules = rules.reject do |rule| + include_rule = rule.beta? && !status_list.include?(STATUS_BETA) || + rule.deprecated? && !status_list.include?(STATUS_DEPRECATED) || + rule.disabled? && !status_list.include?(STATUS_DISABLED) + !include_rule + end + end if profile inactive = (options[:activation]=='INACTIVE') diff --git a/sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_and_dates_to_rules.rb b/sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_language_and_dates_to_rules.rb similarity index 88% rename from sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_and_dates_to_rules.rb rename to sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_language_and_dates_to_rules.rb index 7d6952cb552..40875b69cf9 100644 --- a/sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_and_dates_to_rules.rb +++ b/sonar-server/src/main/webapp/WEB-INF/db/migrate/380_add_status_language_and_dates_to_rules.rb @@ -21,9 +21,10 @@ # # Sonar 3.6 # -class AddStatusAndDatesToRules < ActiveRecord::Migration +class AddStatusLanguageAndDatesToRules < ActiveRecord::Migration def self.up add_column 'rules', 'status', :string, :null => true, :limit => 40 + add_column 'rules', 'language', :string, :null => true, :limit => 20 add_column 'rules', 'created_at', :datetime, :null => true add_column 'rules', 'updated_at', :datetime, :null => true 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 4f38f512d2b..6f7b57b7756 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 @@ -60,7 +60,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void saveNewRepositories() { + public void should_save_new_repositories() { setupData("shared"); task.start(); @@ -72,12 +72,13 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { assertThat(first.getRepositoryKey(), is("fake")); assertThat(first.isEnabled(), is(true)); assertThat(first.getCreatedAt(), notNullValue()); - assertThat(first.getStatus(), is(Status.NORMAL.name())); + assertThat(first.getStatus(), is(Status.READY.name())); + assertThat(first.getLanguage(), is("java")); assertThat(first.getParams().size(), is(2)); } @Test - public void disableDeprecatedRepositories() { + public void should_disable_deprecated_repositories() { setupData("shared"); task.start(); @@ -91,7 +92,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void disableDeprecatedActiveRules() { + public void should_disable_deprecated_active_rules() { setupData("disableDeprecatedActiveRules"); task.start(); @@ -107,7 +108,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void disableDeprecatedActiveRuleParameters() { + public void should_disable_Deprecated_Active_Rule_Parameters() { setupData("disableDeprecatedActiveRuleParameters"); task.start(); @@ -117,7 +118,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void disableDeprecatedRules() { + public void should_disable_deprecated_rules() { setupData("disableDeprecatedRules"); task.start(); @@ -129,7 +130,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void updateRuleFields() { + public void should_update_rule_fields() { setupData("updadeRuleFields"); task.start(); @@ -145,7 +146,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void updateRuleParameters() { + public void should_update_rule_parameters() { setupData("updateRuleParameters"); task.start(); @@ -168,7 +169,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void doNotDisableUserRulesIfParentIsEnabled() { + public void should_not_disable_user_rules_if_parent_is_enabled() { setupData("doNotDisableUserRulesIfParentIsEnabled"); task.start(); @@ -177,7 +178,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void disableUserRulesIfParentIsDisabled() { + public void should_disable_user_rules_if_parent_is_disabled() { setupData("disableUserRulesIfParentIsDisabled"); task.start(); @@ -186,7 +187,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void shouldNotDisableManualRules() { + public void should_not_disable_manual_rules() { // the hardcoded repository "manual" is used for manual violations setupData("shouldNotDisableManualRules"); task.start(); @@ -196,7 +197,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { } @Test - public void volumeTesting() { + public void volume_testing() { task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new VolumeRepository()}, null); setupData("shared"); task.start(); @@ -207,7 +208,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { // SONAR-3305 @Test - public void shouldFailRuleWithoutName() throws Exception { + public void should_fail_with_rule_without_name() throws Exception { RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutNameRepository()}, ruleI18nManager); setupData("shared"); @@ -228,7 +229,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { // SONAR-3769 @Test - public void shouldFailRuleWithBlankName() throws Exception { + public void should_fail_with_rule_with_blank_name() throws Exception { RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); when(ruleI18nManager.getName(anyString(), anyString(), any(Locale.class))).thenReturn(""); task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutNameRepository()}, ruleI18nManager); @@ -245,7 +246,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { // SONAR-3305 @Test - public void shouldFailRuleWithoutDescription() throws Exception { + public void should_fail_with_rule_without_description() throws Exception { RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); when(ruleI18nManager.getName(anyString(), anyString(), any(Locale.class))).thenReturn("Name"); task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutDescriptionRepository()}, ruleI18nManager); @@ -267,7 +268,7 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { // http://jira.codehaus.org/browse/SONAR-3722 @Test - public void shouldFailRuleWithoutNameInBundle() throws Exception { + public void should_fail_with_rule_without_name_in_bundle() throws Exception { RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutDescriptionRepository()}, ruleI18nManager); setupData("shared"); @@ -281,6 +282,18 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { "because the entry 'rule.rule-without-description-repo.rule1.name' is missing from the bundle.")); } } + + // http://jira.codehaus.org/browse/SONAR-3879 + @Test + public void should_fail_with_rule_with_unknown_status() throws Exception { + task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithUnkownStatusRepository()}, null); + try { + task.start(); + fail("Rule status must be unknown"); + } catch (SonarException e) { + assertThat(e.getMessage(), containsString("The status of a rule can only contains : ")); + } + } } class FakeRepository extends RuleRepository { @@ -354,3 +367,15 @@ class VolumeRepository extends RuleRepository { return rules; } } + +class RuleWithUnkownStatusRepository extends RuleRepository { + public RuleWithUnkownStatusRepository() { + super("rule-with-unknwon-status-repo", "java"); + } + + @Override + public List createRules() { + Rule rule1 = Rule.create("fake", "rule1", "rule1").setDescription("Description").setStatus("UNKNOWN"); + return Arrays.asList(rule1); + } +} -- 2.39.5