SONAR-10310 Check correct usage of deprecated keys

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<RulesDefinition.ExtendedRepository> 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<RuleKey, SingleDeprecatedRuleKey> getDbDeprecatedKeysByOldRuleKey() {
+      return dbDeprecatedKeysById.values().stream()
+        .flatMap(Collection::stream)
+        .collect(uniqueIndex(SingleDeprecatedRuleKey::getOldRuleKeyAsRuleKey));
+    }
+
     private Set<SingleDeprecatedRuleKey> getDBDeprecatedKeysFor(RuleDefinitionDto rule) {
       return dbDeprecatedKeysById.getOrDefault(rule.getId(), emptySet());
     }
@@ -738,30 +745,74 @@ public class RegisterRules implements Startable {
   }
 
 
-  private static void verifyRuleKeyConsistency(List<RulesDefinition.ExtendedRepository> repositories) {
-    List<RulesDefinition.Rule> allRules = repositories.stream()
+  private static void verifyRuleKeyConsistency(List<RulesDefinition.ExtendedRepository> repositories, RegisterRulesContext registerRulesContext) {
+    List<RulesDefinition.Rule> definedRules = repositories.stream()
       .flatMap(r -> r.rules().stream())
       .collect(toList());
 
-    Set<RuleKey> definedRuleKeys = allRules.stream()
+    Set<RuleKey> definedRuleKeys = definedRules.stream()
       .map(r -> RuleKey.of(r.repository().key(), r.key()))
       .collect(toSet());
 
-    List<RuleKey> definedDeprecatedRuleKeys = allRules.stream()
+    List<RuleKey> definedDeprecatedRuleKeys = definedRules.stream()
       .flatMap(r -> r.deprecatedRuleKeys().stream())
       .collect(toList());
 
+    // Find duplicates in declared deprecated rule keys
     Set<RuleKey> 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<RuleKey> 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<RuleKey, SingleDeprecatedRuleKey> dbDeprecatedRuleKeysByOldRuleKey = registerRulesContext.getDbDeprecatedKeysByOldRuleKey();
+
+    Set<String> 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<String> filterInvalidDeprecatedRuleKeys(ImmutableMap<RuleKey, SingleDeprecatedRuleKey> 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<String> toString) {
+    return new Object() {
+      @Override
+      public String toString() {
+        return toString.get();
+      }
+    };
   }
 
   private static <T> Set<T> findDuplicates(Collection<T> list) {

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;

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<NewRule>... 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<RuleParamDto> 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");