From db81905fafe6e0ac098d793769ea77b42267aa66 Mon Sep 17 00:00:00 2001 From: Fabrice Bellingard Date: Fri, 9 Mar 2012 16:23:49 +0100 Subject: [PATCH] SONAR-3305 NPE thrown when no description or title on a rule => Sonar server now fails at startup if a rule has no name or no description --- .../sonar/server/startup/RegisterRules.java | 36 ++++-- .../server/startup/RegisterRulesTest.java | 119 +++++++++++++++--- 2 files changed, 125 insertions(+), 30 deletions(-) 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 254fa88da78..6c9846ef8e0 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 @@ -28,7 +28,9 @@ import org.sonar.api.rules.Rule; import org.sonar.api.rules.RuleParam; import org.sonar.api.rules.RuleRepository; import org.sonar.api.utils.Logs; +import org.sonar.api.utils.SonarException; import org.sonar.api.utils.TimeProfiler; +import org.sonar.core.i18n.RuleI18nManager; import org.sonar.jpa.session.DatabaseSessionFactory; import java.util.*; @@ -37,14 +39,16 @@ public final class RegisterRules { private DatabaseSessionFactory sessionFactory; private List repositories = Lists.newArrayList(); + private RuleI18nManager ruleI18nManager; - public RegisterRules(DatabaseSessionFactory sessionFactory, RuleRepository[] repos) { + public RegisterRules(DatabaseSessionFactory sessionFactory, RuleRepository[] repos, RuleI18nManager ruleI18nManager) { this.sessionFactory = sessionFactory; this.repositories.addAll(Arrays.asList(repos)); + this.ruleI18nManager = ruleI18nManager; } - public RegisterRules(DatabaseSessionFactory sessionFactory) { - this(sessionFactory, new RuleRepository[0]); + public RegisterRules(DatabaseSessionFactory sessionFactory, RuleI18nManager ruleI18nManager) { + this(sessionFactory, new RuleRepository[0], ruleI18nManager); } public void start() { @@ -68,12 +72,12 @@ public final class RegisterRules { 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()); + "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.enabled=false)").getResultList()); + "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()); for (Integer deprecatedUserRuleId : deprecatedUserRuleIds) { Rule rule = session.getSingleResult(Rule.class, "id", deprecatedUserRuleId); @@ -91,6 +95,7 @@ public final class RegisterRules { private void registerRepository(RuleRepository repository, DatabaseSession session) { Map rulesByKey = Maps.newHashMap(); for (Rule rule : repository.createRules()) { + validateRule(rule, repository.getKey()); rule.setRepositoryKey(repository.getKey()); rulesByKey.put(rule.getKey(), rule); } @@ -108,6 +113,15 @@ public final class RegisterRules { saveNewRules(rulesByKey.values(), session); } + private void validateRule(Rule rule, String repositoryKey) { + if (rule.getName() == null && ruleI18nManager.getName(repositoryKey, rule.getKey(), Locale.ENGLISH) == null) { + throw new SonarException("The following rule (repository: " + repositoryKey + ") must have a name: " + rule); + } + if (rule.getDescription() == null && ruleI18nManager.getDescription(repositoryKey, rule.getKey(), Locale.ENGLISH) == null) { + throw new SonarException("The following rule (repository: " + repositoryKey + ") must have a description: " + rule); + } + } + private void updateRule(Rule persistedRule, Rule rule, DatabaseSession session) { persistedRule.setName(rule.getName()); persistedRule.setConfigKey(rule.getConfigKey()); @@ -141,14 +155,14 @@ 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(); session - .createQuery("delete from " + ActiveRuleParam.class.getSimpleName() + " where ruleParam=:param") - .setParameter("param", persistedParam) - .executeUpdate(); + .createQuery("delete from " + ActiveRuleParam.class.getSimpleName() + " where ruleParam=:param") + .setParameter("param", persistedParam) + .executeUpdate(); } } } 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 b5c0b01677c..fad5083e838 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 @@ -19,24 +19,47 @@ */ package org.sonar.server.startup; -import org.junit.Test; -import org.sonar.api.rules.*; -import org.sonar.jpa.test.AbstractDbUnitTestCase; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.number.OrderingComparisons.greaterThan; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Locale; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.number.OrderingComparisons.greaterThan; -import static org.junit.Assert.*; +import org.junit.Before; +import org.junit.Test; +import org.sonar.api.rules.ActiveRule; +import org.sonar.api.rules.ActiveRuleParam; +import org.sonar.api.rules.Rule; +import org.sonar.api.rules.RuleParam; +import org.sonar.api.rules.RulePriority; +import org.sonar.api.rules.RuleRepository; +import org.sonar.api.utils.SonarException; +import org.sonar.core.i18n.RuleI18nManager; +import org.sonar.jpa.test.AbstractDbUnitTestCase; public class RegisterRulesTest extends AbstractDbUnitTestCase { + private RegisterRules task; + + @Before + public void init() { + task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}, null); + } + @Test public void saveNewRepositories() { setupData("shared"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); List result = getSession().getResults(Rule.class, "pluginName", "fake"); @@ -52,7 +75,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void disableDeprecatedRepositories() { setupData("shared"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); List rules = (List) getSession() @@ -67,7 +89,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void disableDeprecatedActiveRules() { setupData("disableDeprecatedActiveRules"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); List result = getSession().getResults(Rule.class, "pluginName", "fake"); @@ -84,7 +105,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void disableDeprecatedActiveRuleParameters() { setupData("disableDeprecatedActiveRuleParameters"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); ActiveRule arule = getSession().getSingleResult(ActiveRule.class, "id", 1); @@ -95,7 +115,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void disableDeprecatedRules() { setupData("disableDeprecatedRules"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); Rule rule = getSession().getSingleResult(Rule.class, "id", 1); @@ -108,7 +127,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void updateRuleFields() { setupData("updadeRuleFields"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); // fields have been updated with new values @@ -122,7 +140,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void updateRuleParameters() { setupData("updateRuleParameters"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); Rule rule = getSession().getSingleResult(Rule.class, "id", 1); @@ -146,7 +163,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void doNotDisableUserRulesIfParentIsEnabled() { setupData("doNotDisableUserRulesIfParentIsEnabled"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); Rule rule = getSession().getSingleResult(Rule.class, "id", 2); @@ -156,7 +172,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void disableUserRulesIfParentIsDisabled() { setupData("disableUserRulesIfParentIsDisabled"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); Rule rule = getSession().getSingleResult(Rule.class, "id", 2); @@ -167,8 +182,6 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { public void shouldNotDisableManualRules() { // the hardcoded repository "manual" is used for manual violations setupData("shouldNotDisableManualRules"); - - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new FakeRepository()}); task.start(); assertThat(getSession().getSingleResult(Rule.class, "id", 1).isEnabled(), is(true)); @@ -177,13 +190,55 @@ public class RegisterRulesTest extends AbstractDbUnitTestCase { @Test public void volumeTesting() { + task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new VolumeRepository()}, null); setupData("shared"); - RegisterRules task = new RegisterRules(getSessionFactory(), new RuleRepository[]{new VolumeRepository()}); task.start(); List result = getSession().getResults(Rule.class, "enabled", true); assertThat(result.size(), is(VolumeRepository.SIZE)); } + + // http://jira.codehaus.org/browse/SONAR-3305 + @Test + public void shouldFailRuleWithoutName() throws Exception { + RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); + task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutNameRepository()}, ruleI18nManager); + setupData("shared"); + + // the rule has no name, it should fail + try { + task.start(); + fail("Rule must have a name"); + } catch (SonarException e) { + assertThat(e.getMessage(), containsString("must have a name")); + } + + // now it is ok, the rule has a name in the English bundle + when(ruleI18nManager.getName(anyString(), anyString(), any(Locale.class))).thenReturn("Name"); + when(ruleI18nManager.getDescription(anyString(), anyString(), any(Locale.class))).thenReturn("Description"); + task.start(); + } + + // http://jira.codehaus.org/browse/SONAR-3305 + @Test + public void shouldFailRuleWithoutDescription() throws Exception { + RuleI18nManager ruleI18nManager = mock(RuleI18nManager.class); + task = new RegisterRules(getSessionFactory(), new RuleRepository[] {new RuleWithoutDescriptionRepository()}, ruleI18nManager); + setupData("shared"); + + // the rule has no name, it should fail + try { + task.start(); + fail("Rule must have a description"); + } catch (SonarException e) { + assertThat(e.getMessage(), containsString("must have a description")); + } + + // now it is ok, the rule has a name & a description in the English bundle + when(ruleI18nManager.getName(anyString(), anyString(), any(Locale.class))).thenReturn("Name"); + when(ruleI18nManager.getDescription(anyString(), anyString(), any(Locale.class))).thenReturn("Description"); + task.start(); + } } class FakeRepository extends RuleRepository { @@ -200,12 +255,37 @@ class FakeRepository extends RuleRepository { rule1.createParameter("param2").setDescription("parameter two").setDefaultValue("default value two"); Rule rule2 = Rule.create("fake", "rule2", "Two"); + rule2.setDescription("Description of Two"); rule2.setSeverity(RulePriority.INFO); return Arrays.asList(rule1, rule2); } } +class RuleWithoutNameRepository extends RuleRepository { + public RuleWithoutNameRepository() { + super("rule-without-name-repo", "java"); + } + + public List createRules() { + // Rules must not have empty name + Rule rule1 = Rule.create("fake", "rule1", null); + return Arrays.asList(rule1); + } +} + +class RuleWithoutDescriptionRepository extends RuleRepository { + public RuleWithoutDescriptionRepository() { + super("rule-without-description-repo", "java"); + } + + public List createRules() { + // Rules must not have empty description + Rule rule1 = Rule.create("fake", "rule1", "Rule 1"); + return Arrays.asList(rule1); + } +} + class VolumeRepository extends RuleRepository { static final int SIZE = 500; @@ -216,7 +296,8 @@ class VolumeRepository extends RuleRepository { public List createRules() { List rules = new ArrayList(); for (int i = 0; i < SIZE; i++) { - Rule rule = Rule.create("volume", "rule" + i, "description of " + i); + Rule rule = Rule.create("volume", "rule" + i, "name of " + i); + rule.setDescription("description of " + i); rule.setSeverity(RulePriority.BLOCKER); for (int j = 0; j < 20; j++) { rule.createParameter("param" + j); -- 2.39.5