SONAR-10307 support rule renaming in RegisterRules

server/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java

@@ -24,9 +24,11 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -64,7 +66,6 @@ import org.sonar.server.qualityprofile.QProfileRules;
 import org.sonar.server.qualityprofile.index.ActiveRuleIndexer;
 import org.sonar.server.rule.index.RuleIndexer;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.Lists.newArrayList;
@@ -136,27 +137,13 @@ public class RegisterRules implements Startable {
       // to be updated. Only a single DB commit should be executed.
       ruleIndexer.commitAndIndex(dbSession, registerRulesContext.getAllModified().map(RuleDefinitionDto::getId).collect(toSet()));
       activeRuleIndexer.commitAndIndex(dbSession, changes);
+      registerRulesContext.getRenamed().forEach(e -> LOG.info("rule {} re-keyed to {}", e.getValue(), e.getKey().getKey()));
       profiler.stopDebug();
 
       webServerRuleFinder.startCaching();
     }
   }
 
-  @Override
-  public void stop() {
-    // nothing
-  }
-
-  private void persistRepositories(DbSession dbSession, List<RulesDefinition.Repository> repositories) {
-    dbClient.ruleRepositoryDao().truncate(dbSession);
-    List<RuleRepositoryDto> dtos = repositories
-      .stream()
-      .map(r -> new RuleRepositoryDto(r.key(), r.language(), r.name()))
-      .collect(MoreCollectors.toList(repositories.size()));
-    dbClient.ruleRepositoryDao().insert(dbSession, dtos);
-    dbSession.commit();
-  }
-
   private static List<RulesDefinition.ExtendedRepository> getRepositories(RulesDefinition.Context context) {
     List<RulesDefinition.ExtendedRepository> repositories = new ArrayList<>(context.repositories());
     for (RulesDefinition.ExtendedRepository extendedRepoDef : context.extendedRepositories()) {
@@ -205,6 +192,7 @@ public class RegisterRules implements Startable {
     private final Set<RuleDefinitionDto> known;
     // mutable data
     private final Set<RuleDefinitionDto> created = new HashSet<>();
+    private final Map<RuleDefinitionDto, RuleKey> renamed = new HashMap<>();
     private final Set<RuleDefinitionDto> updated = new HashSet<>();
     private final Set<RuleDefinitionDto> unchanged = new HashSet<>();
     private final Set<RuleDefinitionDto> removed = new HashSet<>();
@@ -216,12 +204,16 @@ public class RegisterRules implements Startable {
 
     private Optional<RuleDefinitionDto> getDbRuleFor(RulesDefinition.Rule ruleDef) {
       RuleKey ruleKey = RuleKey.of(ruleDef.repository().key(), ruleDef.key());
-      return Optional.ofNullable(dbRules.get(ruleKey));
+      return Stream.concat(Stream.of(ruleKey), ruleDef.deprecatedRuleKeys().stream())
+        .map(dbRules::get)
+        .filter(Objects::nonNull)
+        .findFirst();
     }
 
     private Stream<RuleDefinitionDto> getRemaining() {
       Set<RuleDefinitionDto> res = new HashSet<>(dbRules.values());
       res.removeAll(unchanged);
+      res.removeAll(renamed.keySet());
       res.removeAll(updated);
       res.removeAll(removed);
       return res.stream();
@@ -231,11 +223,16 @@ public class RegisterRules implements Startable {
       return removed.stream();
     }
 
+    public Stream<Map.Entry<RuleDefinitionDto, RuleKey>> getRenamed() {
+      return renamed.entrySet().stream();
+    }
+
     private Stream<RuleDefinitionDto> getAllModified() {
       return Stream.of(
         created.stream(),
         updated.stream(),
-        removed.stream())
+        removed.stream(),
+        renamed.keySet().stream())
         .flatMap(s -> s);
     }
 
@@ -243,6 +240,10 @@ public class RegisterRules implements Startable {
       return created.contains(ruleDefinition);
     }
 
+    private boolean isRenamed(RuleDefinitionDto ruleDefinition) {
+      return renamed.containsKey(ruleDefinition);
+    }
+
     private boolean isUpdated(RuleDefinitionDto ruleDefinition) {
       return updated.contains(ruleDefinition);
     }
@@ -252,6 +253,11 @@ public class RegisterRules implements Startable {
       created.add(ruleDefinition);
     }
 
+    private void renamed(RuleDefinitionDto ruleDefinition) {
+      ensureKnown(ruleDefinition);
+      renamed.put(ruleDefinition, ruleDefinition.getKey());
+    }
+
     private void updated(RuleDefinitionDto ruleDefinition) {
       ensureKnown(ruleDefinition);
       updated.add(ruleDefinition);
@@ -268,18 +274,29 @@ public class RegisterRules implements Startable {
     }
 
     private void ensureKnown(RuleDefinitionDto ruleDefinition) {
-      if (!known.contains(ruleDefinition)) {
-        throw new IllegalArgumentException("unknown RuleDef");
-      }
-      checkArgument(known.contains(ruleDefinition), "unknown RuleDefinitionDto");
+      checkState(known.contains(ruleDefinition), "unknown RuleDefinitionDto");
     }
   }
 
-  /**
-   * @return the id of the rule if it's just been created or if it's been updated.
-   */
+  private void persistRepositories(DbSession dbSession, List<RulesDefinition.Repository> repositories) {
+    dbClient.ruleRepositoryDao().truncate(dbSession);
+    List<RuleRepositoryDto> dtos = repositories
+      .stream()
+      .map(r -> new RuleRepositoryDto(r.key(), r.language(), r.name()))
+      .collect(MoreCollectors.toList(repositories.size()));
+    dbClient.ruleRepositoryDao().insert(dbSession, dtos);
+    dbSession.commit();
+  }
+
+  @Override
+  public void stop() {
+    // nothing
+  }
+
   private void registerRule(RegisterRulesContext recorder, RulesDefinition.Rule ruleDef,
     Map<Integer, Set<SingleDeprecatedRuleKey>> existingDeprecatedRuleKeys, DbSession session) {
+    RuleKey ruleKey = RuleKey.of(ruleDef.repository().key(), ruleDef.key());
+
     RuleDefinitionDto ruleDefinitionDto = recorder.getDbRuleFor(ruleDef)
       .orElseGet(() -> {
         RuleDefinitionDto newRule = createRuleDto(ruleDef, session);
@@ -287,6 +304,12 @@ public class RegisterRules implements Startable {
         return newRule;
       });
 
+    // we must detect renaming __before__ we modify the DTO
+    if (!ruleDefinitionDto.getKey().equals(ruleKey)) {
+      recorder.renamed(ruleDefinitionDto);
+      ruleDefinitionDto.setRuleKey(ruleKey);
+    }
+
     if (mergeRule(ruleDef, ruleDefinitionDto)) {
       recorder.updated(ruleDefinitionDto);
     }
@@ -299,7 +322,7 @@ public class RegisterRules implements Startable {
       recorder.updated(ruleDefinitionDto);
     }
 
-    if (recorder.isUpdated(ruleDefinitionDto)) {
+    if (recorder.isUpdated(ruleDefinitionDto) || recorder.isRenamed(ruleDefinitionDto)) {
       update(session, ruleDefinitionDto);
     } else if (!recorder.isCreated(ruleDefinitionDto)) {
       recorder.unchanged(ruleDefinitionDto);

server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesTest.java

@@ -19,12 +19,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.Date;
 import java.util.List;
 import java.util.Set;
 import java.util.stream.IntStream;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.sonar.api.config.internal.MapSettings;
 import org.sonar.api.resources.Language;
 import org.sonar.api.resources.Languages;
@@ -51,6 +55,7 @@ import org.sonar.db.rule.RuleDto.Scope;
 import org.sonar.db.rule.RuleParamDto;
 import org.sonar.db.rule.RuleRepositoryDto;
 import org.sonar.server.es.EsTester;
+import org.sonar.server.es.SearchIdResult;
 import org.sonar.server.es.SearchOptions;
 import org.sonar.server.organization.OrganizationFlags;
 import org.sonar.server.organization.TestOrganizationFlags;
@@ -76,6 +81,7 @@ import static org.sonar.api.rule.Severity.BLOCKER;
 import static org.sonar.api.rule.Severity.INFO;
 import static org.sonar.api.server.rule.RulesDefinition.*;
 
+@RunWith(DataProviderRunner.class)
 public class RegisterRulesTest {
 
   private static final String FAKE_PLUGIN_KEY = "unittest";
@@ -361,6 +367,177 @@ public class RegisterRulesTest {
     assertThat(ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions()).getTotal()).isEqualTo(0);
   }
 
+  @Test
+  public void update_if_rule_key_renamed_and_deprecated_key_declared() {
+    String ruleKey1 = "rule1";
+    String ruleKey2 = "rule2";
+    String repository = "fake";
+
+    when(system.now()).thenReturn(DATE1.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository, "java");
+      repo.createRule(ruleKey1)
+        .setName("Name1")
+        .setHtmlDescription("Description");
+      repo.done();
+    });
+
+    RuleDto rule1 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository, ruleKey1));
+    SearchIdResult<Integer> searchRule1 = ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions());
+    assertThat(searchRule1.getIds()).containsOnly(rule1.getId());
+    assertThat(searchRule1.getTotal()).isEqualTo(1);
+
+    when(system.now()).thenReturn(DATE2.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository, "java");
+      repo.createRule(ruleKey2)
+        .setName("Name2")
+        .setHtmlDescription("Description")
+        .addDeprecatedRuleKey(repository, ruleKey1);
+      repo.done();
+    });
+
+    // rule2 is actually rule1
+    RuleDto rule2 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository, ruleKey2));
+    assertThat(rule2.getId()).isEqualTo(rule1.getId());
+    assertThat(rule2.getName()).isEqualTo("Name2");
+    assertThat(rule2.getDescription()).isEqualTo(rule1.getDescription());
+
+    SearchIdResult<Integer> searchRule2 = ruleIndex.search(new RuleQuery().setQueryText("Name2"), new SearchOptions());
+    assertThat(searchRule2.getIds()).containsOnly(rule2.getId());
+    assertThat(searchRule2.getTotal()).isEqualTo(1);
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions()).getTotal()).isEqualTo(0);
+  }
+
+  @Test
+  public void update_if_repository_changed_and_deprecated_key_declared() {
+    String ruleKey = "rule";
+    String repository1 = "fake1";
+    String repository2 = "fake2";
+
+    when(system.now()).thenReturn(DATE1.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository1, "java");
+      repo.createRule(ruleKey)
+        .setName("Name1")
+        .setHtmlDescription("Description");
+      repo.done();
+    });
+
+    RuleDto rule1 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository1, ruleKey));
+    SearchIdResult<Integer> searchRule1 = ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions());
+    assertThat(searchRule1.getIds()).containsOnly(rule1.getId());
+    assertThat(searchRule1.getTotal()).isEqualTo(1);
+
+    when(system.now()).thenReturn(DATE2.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository2, "java");
+      repo.createRule(ruleKey)
+        .setName("Name2")
+        .setHtmlDescription("Description")
+        .addDeprecatedRuleKey(repository1, ruleKey);
+      repo.done();
+    });
+
+    // rule2 is actually rule1
+    RuleDto rule2 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository2, ruleKey));
+    assertThat(rule2.getId()).isEqualTo(rule1.getId());
+    assertThat(rule2.getName()).isEqualTo("Name2");
+    assertThat(rule2.getDescription()).isEqualTo(rule1.getDescription());
+
+    SearchIdResult<Integer> searchRule2 = ruleIndex.search(new RuleQuery().setQueryText("Name2"), new SearchOptions());
+    assertThat(searchRule2.getIds()).containsOnly(rule2.getId());
+    assertThat(searchRule2.getTotal()).isEqualTo(1);
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions()).getTotal()).isEqualTo(0);
+  }
+
+  @Test
+  @UseDataProvider("allRenamingCases")
+  public void update_if_only_renamed_and_deprecated_key_declared(String ruleKey1, String repo1, String ruleKey2, String repo2) {
+    String name = "Name1";
+    String description = "Description";
+    when(system.now()).thenReturn(DATE1.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repo1, "java");
+      repo.createRule(ruleKey1)
+        .setName(name)
+        .setHtmlDescription(description);
+      repo.done();
+    });
+
+    RuleDto rule1 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repo1, ruleKey1));
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText(name), new SearchOptions()).getIds())
+      .containsOnly(rule1.getId());
+
+    when(system.now()).thenReturn(DATE2.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repo2, "java");
+      repo.createRule(ruleKey2)
+        .setName(name)
+        .setHtmlDescription(description)
+        .addDeprecatedRuleKey(repo1, ruleKey1);
+      repo.done();
+    });
+
+    // rule2 is actually rule1
+    RuleDto rule2 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repo2, ruleKey2));
+    assertThat(rule2.getId()).isEqualTo(rule1.getId());
+    assertThat(rule2.getName()).isEqualTo(rule1.getName());
+    assertThat(rule2.getDescription()).isEqualTo(rule1.getDescription());
+
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText(name), new SearchOptions()).getIds())
+      .containsOnly(rule2.getId());
+  }
+
+  @DataProvider
+  public static Object[][] allRenamingCases() {
+    return new Object[][] {
+      {"repo1", "rule1", "repo1", "rule2"},
+      {"repo1", "rule1", "repo2", "rule1"},
+      {"repo1", "rule1", "repo2", "rule2"},
+    };
+  }
+
+  @Test
+  public void update_if_repository_and_key_changed_and_deprecated_key_declared_among_others() {
+    String ruleKey1 = "rule1";
+    String ruleKey2 = "rule2";
+    String repository1 = "fake1";
+    String repository2 = "fake2";
+
+    when(system.now()).thenReturn(DATE1.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository1, "java");
+      repo.createRule(ruleKey1)
+        .setName("Name1")
+        .setHtmlDescription("Description");
+      repo.done();
+    });
+
+    RuleDto rule1 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository1, ruleKey1));
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText("Name1"), new SearchOptions()).getIds())
+      .containsOnly(rule1.getId());
+
+    when(system.now()).thenReturn(DATE2.getTime());
+    execute((RulesDefinition) context -> {
+      RulesDefinition.NewRepository repo = context.createRepository(repository2, "java");
+      repo.createRule(ruleKey2)
+        .setName("Name2")
+        .setHtmlDescription("Description")
+        .addDeprecatedRuleKey("foo", "bar")
+        .addDeprecatedRuleKey(repository1, ruleKey1)
+        .addDeprecatedRuleKey("some", "noise");
+      repo.done();
+    });
+
+    // rule2 is actually rule1
+    RuleDto rule2 = dbClient.ruleDao().selectOrFailByKey(dbTester.getSession(), defaultOrganization, RuleKey.of(repository2, ruleKey2));
+    assertThat(rule2.getId()).isEqualTo(rule1.getId());
+
+    assertThat(ruleIndex.search(new RuleQuery().setQueryText("Name2"), new SearchOptions()).getIds())
+      .containsOnly(rule1.getId());
+  }
+
   @Test
   public void update_only_rule_description() {
     when(system.now()).thenReturn(DATE1.getTime());
@@ -552,7 +729,7 @@ public class RegisterRulesTest {
 
     List<RuleDefinitionDto> rules = dbClient.ruleDao().selectAllDefinitions(dbTester.getSession());
     Set<DeprecatedRuleKeyDto> deprecatedRuleKeys = dbClient.ruleDao().selectAllDeprecatedRuleKeys(dbTester.getSession());
-    //assertThat(rules).hasSize(1); FIXME this must be true when renaming is done
+    assertThat(rules).hasSize(1);
     assertThat(deprecatedRuleKeys).hasSize(2);
   }
 
@@ -586,7 +763,7 @@ public class RegisterRulesTest {
       repo.done();
     });
 
-    //assertThat(dbClient.ruleDao().selectAllDefinitions(dbTester.getSession())).hasSize(1); FIXME this must be true when renaming is done
+    assertThat(dbClient.ruleDao().selectAllDefinitions(dbTester.getSession())).hasSize(1);
     Set<DeprecatedRuleKeyDto> deprecatedRuleKeys = dbClient.ruleDao().selectAllDeprecatedRuleKeys(dbTester.getSession());
     assertThat(deprecatedRuleKeys).hasSize(2);
 
@@ -603,7 +780,7 @@ public class RegisterRulesTest {
       repo.done();
     });
 
-    //assertThat(dbClient.ruleDao().selectAllDefinitions(dbTester.getSession())).hasSize(1); FIXME this must be true when renaming is done
+    assertThat(dbClient.ruleDao().selectAllDefinitions(dbTester.getSession())).hasSize(1);
     deprecatedRuleKeys = dbClient.ruleDao().selectAllDeprecatedRuleKeys(dbTester.getSession());
     assertThat(deprecatedRuleKeys).hasSize(0);
   }

tests/src/test/java/org/sonarqube/tests/qualityProfile/BuiltInQualityProfilesNotificationTest.java

@@ -136,7 +136,7 @@ public class BuiltInQualityProfilesNotificationTest {
       .containsSubsequence(
         "The following built-in profiles have been updated:",
         "\"Basic\" - Foo: " + url + "/profiles/changelog?language=foo&name=Basic&since=", "&to=",
-        " 3 new rules",
+        " 1 new rule",
         " 3 rules have been updated",
         " 1 rule removed",
         "This is a good time to review your quality profiles and update them to benefit from the latest evolutions: " + url + "/profiles")