From: Sébastien Lesaint Date: Fri, 2 Feb 2018 16:40:28 +0000 (+0100) Subject: SONAR-10307 support rule renaming in RegisterRules X-Git-Tag: 7.5~1684 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=a8fcb745fb59b4ba2e11bc43f5b57f7366eacb0c;p=sonarqube.git SONAR-10307 support rule renaming in RegisterRules --- 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 da13e5fd6ee..0becfdeca37 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 @@ -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 repositories) { - dbClient.ruleRepositoryDao().truncate(dbSession); - List 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 getRepositories(RulesDefinition.Context context) { List repositories = new ArrayList<>(context.repositories()); for (RulesDefinition.ExtendedRepository extendedRepoDef : context.extendedRepositories()) { @@ -205,6 +192,7 @@ public class RegisterRules implements Startable { private final Set known; // mutable data private final Set created = new HashSet<>(); + private final Map renamed = new HashMap<>(); private final Set updated = new HashSet<>(); private final Set unchanged = new HashSet<>(); private final Set removed = new HashSet<>(); @@ -216,12 +204,16 @@ public class RegisterRules implements Startable { private Optional 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 getRemaining() { Set 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> getRenamed() { + return renamed.entrySet().stream(); + } + private Stream 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 repositories) { + dbClient.ruleRepositoryDao().truncate(dbSession); + List 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> 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); 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 2b53a3020b8..fbcf86f59f9 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 @@ -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 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 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 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 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 rules = dbClient.ruleDao().selectAllDefinitions(dbTester.getSession()); Set 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 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); } diff --git a/tests/src/test/java/org/sonarqube/tests/qualityProfile/BuiltInQualityProfilesNotificationTest.java b/tests/src/test/java/org/sonarqube/tests/qualityProfile/BuiltInQualityProfilesNotificationTest.java index 3f81750e2db..042430e63b2 100644 --- a/tests/src/test/java/org/sonarqube/tests/qualityProfile/BuiltInQualityProfilesNotificationTest.java +++ b/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")