From: Eric Hartmann Date: Thu, 8 Feb 2018 08:21:25 +0000 (+0100) Subject: SONAR-10310 Check correct usage of deprecated keys X-Git-Tag: 7.5~1672 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=ddc1c7b4fe9cbd9a899e5db5ab34ea547cfce615;p=sonarqube.git SONAR-10310 Check correct usage of deprecated keys --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java index af1de2cba2b..3ffb85b521f 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -119,7 +120,7 @@ public class RegisterRules implements Startable { List repositories = getRepositories(ruleDefinitionContext); RegisterRulesContext registerRulesContext = createRegisterRulesContext(dbSession); - verifyRuleKeyConsistency(repositories); + verifyRuleKeyConsistency(repositories, registerRulesContext); boolean orgsEnabled = organizationFlags.isEnabled(dbSession); for (RulesDefinition.ExtendedRepository repoDef : repositories) { @@ -245,6 +246,12 @@ public class RegisterRules implements Startable { return res; } + private ImmutableMap getDbDeprecatedKeysByOldRuleKey() { + return dbDeprecatedKeysById.values().stream() + .flatMap(Collection::stream) + .collect(uniqueIndex(SingleDeprecatedRuleKey::getOldRuleKeyAsRuleKey)); + } + private Set getDBDeprecatedKeysFor(RuleDefinitionDto rule) { return dbDeprecatedKeysById.getOrDefault(rule.getId(), emptySet()); } @@ -738,30 +745,74 @@ public class RegisterRules implements Startable { } - private static void verifyRuleKeyConsistency(List repositories) { - List allRules = repositories.stream() + private static void verifyRuleKeyConsistency(List repositories, RegisterRulesContext registerRulesContext) { + List definedRules = repositories.stream() .flatMap(r -> r.rules().stream()) .collect(toList()); - Set definedRuleKeys = allRules.stream() + Set definedRuleKeys = definedRules.stream() .map(r -> RuleKey.of(r.repository().key(), r.key())) .collect(toSet()); - List definedDeprecatedRuleKeys = allRules.stream() + List definedDeprecatedRuleKeys = definedRules.stream() .flatMap(r -> r.deprecatedRuleKeys().stream()) .collect(toList()); + // Find duplicates in declared deprecated rule keys Set duplicates = findDuplicates(definedDeprecatedRuleKeys); - if (!duplicates.isEmpty()) { - throw new IllegalStateException(format("The following deprecated rule keys are declared at least twice [%s]", - duplicates.stream().map(RuleKey::toString).collect(Collectors.joining(",")))); - } + checkState(duplicates.isEmpty(), "The following deprecated rule keys are declared at least twice [%s]", + lazyToString(() -> duplicates.stream().map(RuleKey::toString).collect(Collectors.joining(",")))); + // Find rule keys that are both deprecated and used Set intersection = intersection(new HashSet<>(definedRuleKeys), new HashSet<>(definedDeprecatedRuleKeys)).immutableCopy(); - if (!intersection.isEmpty()) { - throw new IllegalStateException(format("The following rule keys are declared both as deprecated and used key [%s]", - intersection.stream().map(RuleKey::toString).collect(Collectors.joining(",")))); - } + checkState(intersection.isEmpty(), "The following rule keys are declared both as deprecated and used key [%s]", + lazyToString(() -> intersection.stream().map(RuleKey::toString).collect(Collectors.joining(",")))); + + // Find incorrect usage of deprecated keys + ImmutableMap dbDeprecatedRuleKeysByOldRuleKey = registerRulesContext.getDbDeprecatedKeysByOldRuleKey(); + + Set incorrectRuleKeyMessage = definedRules.stream() + .flatMap(r -> filterInvalidDeprecatedRuleKeys(dbDeprecatedRuleKeysByOldRuleKey, r)) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); + + checkState(incorrectRuleKeyMessage.isEmpty(), "An incorrect state of deprecated rule keys has been detected.\n %s", + lazyToString(() -> incorrectRuleKeyMessage.stream().collect(Collectors.joining("\n")))); + } + + private static Stream filterInvalidDeprecatedRuleKeys(ImmutableMap dbDeprecatedRuleKeysByOldRuleKey, + RulesDefinition.Rule rule) { + return rule.deprecatedRuleKeys().stream() + .map(rk -> { + SingleDeprecatedRuleKey singleDeprecatedRuleKey = dbDeprecatedRuleKeysByOldRuleKey.get(rk); + if (singleDeprecatedRuleKey == null) { + // new deprecated rule key : OK + return null; + } + RuleKey parentRuleKey = RuleKey.of(rule.repository().key(), rule.key()); + if (parentRuleKey.equals(singleDeprecatedRuleKey.getNewRuleKeyAsRuleKey())) { + // same parent : OK + return null; + } + if (rule.deprecatedRuleKeys().contains(parentRuleKey)) { + // the new rule is deprecating the old parentRuleKey : OK + return null; + } + return format("The deprecated rule key [%s] was previously deprecated by [%s]. [%s] should be a deprecated key of [%s],", + rk.toString(), + singleDeprecatedRuleKey.getNewRuleKeyAsRuleKey().toString(), + singleDeprecatedRuleKey.getNewRuleKeyAsRuleKey().toString(), + RuleKey.of(rule.repository().key(), rule.key()).toString()); + }); + } + + private static Object lazyToString(Supplier toString) { + return new Object() { + @Override + public String toString() { + return toString.get(); + } + }; } private static Set findDuplicates(Collection list) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/SingleDeprecatedRuleKey.java b/server/sonar-server/src/main/java/org/sonar/server/rule/SingleDeprecatedRuleKey.java index fdfc5b93c56..1dd580f8d61 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/SingleDeprecatedRuleKey.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/SingleDeprecatedRuleKey.java @@ -78,6 +78,11 @@ class SingleDeprecatedRuleKey { return RuleKey.of(oldRepositoryKey, oldRuleKey); } + + public RuleKey getNewRuleKeyAsRuleKey() { + return RuleKey.of(newRepositoryKey, newRuleKey); + } + @CheckForNull public String getNewRuleKey() { return newRuleKey; diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesTest.java index 64cecd404e9..7c63e9255f9 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesTest.java @@ -22,14 +22,16 @@ package org.sonar.server.rule; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import java.util.Arrays; import java.util.Date; import java.util.List; import java.util.Set; +import java.util.function.Consumer; import java.util.stream.IntStream; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; import org.junit.rules.ExpectedException; +import org.junit.runner.RunWith; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.resources.Language; import org.sonar.api.resources.Languages; @@ -80,7 +82,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.sonar.api.rule.Severity.BLOCKER; import static org.sonar.api.rule.Severity.INFO; -import static org.sonar.api.server.rule.RulesDefinition.*; +import static org.sonar.api.server.rule.RulesDefinition.NewRepository; +import static org.sonar.api.server.rule.RulesDefinition.NewRule; @RunWith(DataProviderRunner.class) public class RegisterRulesTest { @@ -167,7 +170,7 @@ public class RegisterRulesTest { // register one rule execute(context -> { - RulesDefinition.NewRepository repo = context.createRepository("fake", "java"); + NewRepository repo = context.createRepository("fake", "java"); repo.createRule(ruleKey) .setName(randomAlphanumeric(5)) .setHtmlDescription(randomAlphanumeric(20)); @@ -610,6 +613,7 @@ public class RegisterRulesTest { public void do_not_update_already_removed_rules() { execute(new FakeRepositoryV1()); assertThat(dbClient.ruleDao().selectAllDefinitions(dbTester.getSession())).hasSize(2); + RuleDto rule1 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RULE_KEY1); RuleDto rule2 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RULE_KEY2); assertThat(esTester.getIds(RuleIndexDefinition.INDEX_TYPE_RULE)).containsOnly(valueOf(rule1.getId()), valueOf(rule2.getId())); @@ -846,6 +850,22 @@ public class RegisterRulesTest { }); } + @Test + public void updating_the_deprecated_to_a_new_ruleKey_should_throw_an_ISE() { + // On this new rule add a deprecated key + execute(context -> createRule(context, "javascript", "javascript", "s103", + r -> r.addDeprecatedRuleKey("javascript", "linelength"))); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("An incorrect state of deprecated rule keys has been detected.\n " + + "The deprecated rule key [javascript:linelength] was previously deprecated by [javascript:s103]. [javascript:s103] should be a deprecated key of [sonarjs:s103]," + ); + + // This rule should have been moved to another repository + execute(context -> createRule(context, "javascript", "sonarjs", "s103", + r -> r.addDeprecatedRuleKey("javascript", "linelength"))); + } + @Test public void declaring_a_rule_with_an_existing_RuleKey_still_used_should_throw_IAE() { expectedException.expect(IllegalArgumentException.class); @@ -881,7 +901,7 @@ public class RegisterRulesTest { RuleDefinitionsLoader loader = new RuleDefinitionsLoader(mock(DeprecatedRulesDefinitionLoader.class), mock(CommonRuleDefinitionsImpl.class), pluginRepository, defs); Languages languages = mock(Languages.class); - when(languages.get("java")).thenReturn(mock(Language.class)); + when(languages.get(any())).thenReturn(mock(Language.class)); reset(webServerRuleFinder); RegisterRules task = new RegisterRules(loader, qProfileRules, dbClient, ruleIndexer, activeRuleIndexer, @@ -893,6 +913,20 @@ public class RegisterRulesTest { verify(webServerRuleFinder).startCaching(); } + @SafeVarargs + private final void createRule(RulesDefinition.Context context, String language, String repositoryKey, String ruleKey, Consumer... consumers) { + NewRepository repo = context.createRepository(repositoryKey, language); + NewRule newRule = repo.createRule(ruleKey) + .setName(ruleKey) + .setHtmlDescription("Description of One") + .setSeverity(BLOCKER) + .setType(RuleType.CODE_SMELL) + .setStatus(RuleStatus.BETA); + + Arrays.stream(consumers).forEach(c -> c.accept(newRule)); + repo.done(); + } + private RuleParamDto getParam(List params, String key) { for (RuleParamDto param : params) { if (param.getName().equals(key)) { @@ -990,7 +1024,6 @@ public class RegisterRulesTest { } static class FbContribRepository implements RulesDefinition { - @Override public void define(Context context) { NewExtendedRepository repo = context.extendRepository("findbugs", "java");