From d6164e9a32c80240cb36bab6812121c1bfd3b539 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Tue, 15 Sep 2020 11:07:07 -0500 Subject: [PATCH] SONAR-13782 It's not possible for a rule key to be deprecated twice --- .../org/sonar/server/rule/RegisterRules.java | 5 +- .../sonar/server/rule/RegisterRulesTest.java | 139 ++++++------------ 2 files changed, 43 insertions(+), 101 deletions(-) diff --git a/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java b/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java index 1a70591f2ff..01229777861 100644 --- a/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -817,8 +817,7 @@ public class RegisterRules implements Startable { lazyToString(() -> String.join("\n", incorrectRuleKeyMessage))); } - private static Stream filterInvalidDeprecatedRuleKeys(ImmutableMap dbDeprecatedRuleKeysByOldRuleKey, - RulesDefinition.Rule rule) { + private static Stream filterInvalidDeprecatedRuleKeys(Map dbDeprecatedRuleKeysByOldRuleKey, RulesDefinition.Rule rule) { return rule.deprecatedRuleKeys().stream() .map(rk -> { SingleDeprecatedRuleKey singleDeprecatedRuleKey = dbDeprecatedRuleKeysByOldRuleKey.get(rk); @@ -831,7 +830,7 @@ public class RegisterRules implements Startable { // same parent : OK return null; } - if (rule.deprecatedRuleKeys().contains(parentRuleKey)) { + if (rule.deprecatedRuleKeys().contains(singleDeprecatedRuleKey.getNewRuleKeyAsRuleKey())) { // the new rule is deprecating the old parentRuleKey : OK return null; } diff --git a/server/sonar-webserver-core/src/test/java/org/sonar/server/rule/RegisterRulesTest.java b/server/sonar-webserver-core/src/test/java/org/sonar/server/rule/RegisterRulesTest.java index 7cda019478b..8ebc31b8e36 100644 --- a/server/sonar-webserver-core/src/test/java/org/sonar/server/rule/RegisterRulesTest.java +++ b/server/sonar-webserver-core/src/test/java/org/sonar/server/rule/RegisterRulesTest.java @@ -72,9 +72,11 @@ import org.sonar.server.rule.index.RuleQuery; import static com.google.common.collect.Sets.newHashSet; import static java.lang.String.valueOf; +import static java.util.Collections.emptySet; import static java.util.Collections.singletonList; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.tuple; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; @@ -850,10 +852,8 @@ public class RegisterRulesTest { execute(new FindbugsRepository()); List rules = dbClient.ruleDao().selectAllDefinitions(db.getSession()); - assertThat(rules).hasSize(1); - RuleDefinitionDto result = rules.get(0); - assertThat(result.getKey()).isEqualTo(RuleKey.of("findbugs", "rule1")); - assertThat(result.getSystemTags()).isEmpty(); + assertThat(rules).hasSize(1).extracting(RuleDefinitionDto::getKey, RuleDefinitionDto::getSystemTags) + .containsOnly(tuple(RuleKey.of("findbugs", "rule1"), emptySet())); } @Test @@ -877,27 +877,13 @@ public class RegisterRulesTest { public void rules_that_deprecate_previous_rule_must_be_recorded() { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("rule1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA); + createRule(repo, "rule1"); repo.done(); }); execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA) + createRule(repo, "newKey") .addDeprecatedRuleKey("fake", "rule1") .addDeprecatedRuleKey("fake", "rule2"); repo.done(); @@ -913,27 +899,13 @@ public class RegisterRulesTest { public void rules_that_remove_deprecated_key_must_remove_records() { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("rule1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA); + createRule(repo, "rule1"); repo.done(); }); execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA) + createRule(repo, "newKey") .addDeprecatedRuleKey("fake", "rule1") .addDeprecatedRuleKey("fake", "rule2"); repo.done(); @@ -945,14 +917,7 @@ public class RegisterRulesTest { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA); + createRule(repo, "newKey"); repo.done(); }); @@ -968,24 +933,10 @@ public class RegisterRulesTest { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .addDeprecatedRuleKey("fake", "old") - .setStatus(RuleStatus.BETA); - repo.createRule("newKey2") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .addDeprecatedRuleKey("fake", "old") - .setStatus(RuleStatus.BETA); + createRule(repo, "newKey1") + .addDeprecatedRuleKey("fake", "old"); + createRule(repo, "newKey2") + .addDeprecatedRuleKey("fake", "old"); repo.done(); }); } @@ -997,24 +948,9 @@ public class RegisterRulesTest { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA); - - repo.createRule("newKey2") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .addDeprecatedRuleKey("fake", "newKey1") - .setStatus(RuleStatus.BETA); + createRule(repo, "newKey1"); + createRule(repo, "newKey2") + .addDeprecatedRuleKey("fake", "newKey1"); repo.done(); }); } @@ -1034,6 +970,18 @@ public class RegisterRulesTest { r -> r.addDeprecatedRuleKey("javascript", "linelength"))); } + @Test + public void deprecate_rule_that_deprecated_another_rule() { + execute(context -> createRule(context, "javascript", "javascript", "s103")); + execute(context -> createRule(context, "javascript", "javascript", "s104", + r -> r.addDeprecatedRuleKey("javascript", "s103"))); + + // This rule should have been moved to another repository + execute(context -> createRule(context, "javascript", "sonarjs", "s105", + r -> r.addDeprecatedRuleKey("javascript", "s103") + .addDeprecatedRuleKey("javascript", "s104"))); + } + @Test public void declaring_a_rule_with_an_existing_RuleKey_still_used_should_throw_IAE() { expectedException.expect(IllegalArgumentException.class); @@ -1041,24 +989,8 @@ public class RegisterRulesTest { execute(context -> { NewRepository repo = context.createRepository("fake", "java"); - repo.createRule("newKey1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .setStatus(RuleStatus.BETA); - - repo.createRule("newKey1") - .setName("One") - .setHtmlDescription("Description of One") - .setSeverity(BLOCKER) - .setInternalKey("config1") - .setTags("tag1", "tag2", "tag3") - .setType(RuleType.CODE_SMELL) - .addDeprecatedRuleKey("fake", "newKey1") - .setStatus(RuleStatus.BETA); + createRule(repo, "newKey1"); + createRule(repo, "newKey1"); repo.done(); }); } @@ -1081,6 +1013,17 @@ public class RegisterRulesTest { verify(webServerRuleFinder).startCaching(); } + private NewRule createRule(NewRepository repo, String key) { + return repo.createRule(key) + .setName(key + " name") + .setHtmlDescription("Description of " + key) + .setSeverity(BLOCKER) + .setInternalKey("config1") + .setTags("tag1", "tag2", "tag3") + .setType(RuleType.CODE_SMELL) + .setStatus(RuleStatus.BETA); + } + @SafeVarargs private final void createRule(RulesDefinition.Context context, String language, String repositoryKey, String ruleKey, Consumer... consumers) { NewRepository repo = context.createRepository(repositoryKey, language); -- 2.39.5