From: Sébastien Lesaint Date: Fri, 2 Feb 2018 16:30:29 +0000 (+0100) Subject: SONAR-10307 refactor RegisterRules implementation X-Git-Tag: 7.5~1686 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=b2b9d1a5bf8d3adbf8a601bc47a583ce7048d0e5;p=sonarqube.git SONAR-10307 refactor RegisterRules implementation --- 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 fffb9360be4..da13e5fd6ee 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 @@ -19,17 +19,18 @@ */ package org.sonar.server.rule; -import com.google.common.collect.Iterables; +import com.google.common.collect.ImmutableMap; +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.Collection; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.annotation.Nullable; import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.StringUtils; @@ -63,12 +64,14 @@ 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; import static com.google.common.collect.Sets.difference; import static java.lang.String.format; import static java.util.Collections.emptySet; -import static java.util.stream.Collectors.toSet; +import static org.sonar.core.util.stream.MoreCollectors.toSet; /** * Register rules at server startup @@ -107,48 +110,31 @@ public class RegisterRules implements Startable { public void start() { Profiler profiler = Profiler.create(LOG).startInfo("Register rules"); try (DbSession dbSession = dbClient.openSession(false)) { - Map allRules = loadRules(dbSession); - List ruleIdsToIndex = new ArrayList<>(); + RulesDefinition.Context ruleDefinitionContext = defLoader.load(); + List repositories = getRepositories(ruleDefinitionContext); + RegisterRulesContext registerRulesContext = createRegisterRulesContext(dbSession); Map> existingDeprecatedRuleKeys = loadDeprecatedRuleKeys(dbSession); - RulesDefinition.Context context = defLoader.load(); - - List repositories = getRepositories(context); - boolean orgsEnabled = organizationFlags.isEnabled(dbSession); - for (RulesDefinition.ExtendedRepository repoDef : repositories) { if (languages.get(repoDef.language()) != null) { for (RulesDefinition.Rule ruleDef : repoDef.rules()) { - RuleKey ruleKey = RuleKey.of(ruleDef.repository().key(), ruleDef.key()); - - if (ruleDef.template() && orgsEnabled) { - RuleDefinitionDto ruleDefinition = allRules.get(ruleKey); - if (ruleDefinition != null && ruleDefinition.getStatus() == RuleStatus.REMOVED) { - LOG.debug("Template rule {} kept removed, because organizations are enabled.", ruleKey); - allRules.remove(ruleKey); - } else { - LOG.info("Template rule {} will not be imported, because organizations are enabled.", ruleKey); - } + if (noTemplateRuleWithOrganizationsEnabled(registerRulesContext, orgsEnabled, ruleDef)) { continue; } - - registerRule(ruleDef, allRules, existingDeprecatedRuleKeys, dbSession) - .ifPresent(ruleIdsToIndex::add); + registerRule(registerRulesContext, ruleDef, existingDeprecatedRuleKeys, dbSession); } dbSession.commit(); } } - - List removedRules = processRemainingDbRules(allRules.values(), dbSession); - List changes = removeActiveRulesOnStillExistingRepositories(dbSession, removedRules, context); + processRemainingDbRules(registerRulesContext, dbSession); + List changes = removeActiveRulesOnStillExistingRepositories(dbSession, registerRulesContext, repositories); dbSession.commit(); - ruleIdsToIndex.addAll(removedRules.stream().map(RuleDefinitionDto::getId).collect(Collectors.toList())); - persistRepositories(dbSession, context.repositories()); + persistRepositories(dbSession, ruleDefinitionContext.repositories()); // FIXME lack of resiliency, active rules index is corrupted if rule index fails // to be updated. Only a single DB commit should be executed. - ruleIndexer.commitAndIndex(dbSession, ruleIdsToIndex); + ruleIndexer.commitAndIndex(dbSession, registerRulesContext.getAllModified().map(RuleDefinitionDto::getId).collect(toSet())); activeRuleIndexer.commitAndIndex(dbSession, changes); profiler.stopDebug(); @@ -171,77 +157,156 @@ public class RegisterRules implements Startable { dbSession.commit(); } - /** - * @return the id of the rule if it's just been created or if it's been updated. - */ - private Optional registerRule(RulesDefinition.Rule ruleDef, Map allRules, - Map> existingDeprecatedRuleKeys, DbSession session) { - RuleKey ruleKey = RuleKey.of(ruleDef.repository().key(), ruleDef.key()); - - RuleDefinitionDto existingRule = allRules.remove(ruleKey); - boolean newRule; - RuleDefinitionDto rule; - if (existingRule == null) { - rule = createRuleDto(ruleDef, session); - newRule = true; + private static List getRepositories(RulesDefinition.Context context) { + List repositories = new ArrayList<>(context.repositories()); + for (RulesDefinition.ExtendedRepository extendedRepoDef : context.extendedRepositories()) { + if (context.repository(extendedRepoDef.key()) == null) { + LOG.warn(format("Extension is ignored, repository %s does not exist", extendedRepoDef.key())); + } else { + repositories.add(extendedRepoDef); + } + } + return repositories; + } + + private RegisterRulesContext createRegisterRulesContext(DbSession dbSession) { + Map allRules = dbClient.ruleDao().selectAllDefinitions(dbSession) + .stream() + .collect(MoreCollectors.uniqueIndex(RuleDefinitionDto::getKey)); + return new RegisterRulesContext(allRules); + } + + private Map> loadDeprecatedRuleKeys(DbSession dbSession) { + return dbClient.ruleDao().selectAllDeprecatedRuleKeys(dbSession) + .stream() + .map(SingleDeprecatedRuleKey::from) + .collect(Collectors.groupingBy(SingleDeprecatedRuleKey::getRuleId, Collectors.toSet())); + } + + private static boolean noTemplateRuleWithOrganizationsEnabled(RegisterRulesContext registerRulesContext, boolean orgsEnabled, RulesDefinition.Rule ruleDef) { + if (!ruleDef.template() || !orgsEnabled) { + return false; + } + + Optional dbRule = registerRulesContext.getDbRuleFor(ruleDef); + if (dbRule.isPresent() && dbRule.get().getStatus() == RuleStatus.REMOVED) { + RuleDefinitionDto dto = dbRule.get(); + LOG.debug("Template rule {} kept removed, because organizations are enabled.", dto.getKey()); + registerRulesContext.removed(dto); } else { - rule = existingRule; - newRule = false; + LOG.info("Template rule {} will not be imported, because organizations are enabled.", RuleKey.of(ruleDef.repository().key(), ruleDef.key())); } + return true; + } + + private static class RegisterRulesContext { + // initial immutable data + private final Map dbRules; + private final Set known; + // mutable data + private final Set created = new HashSet<>(); + private final Set updated = new HashSet<>(); + private final Set unchanged = new HashSet<>(); + private final Set removed = new HashSet<>(); - boolean executeUpdate = false; - if (mergeRule(ruleDef, rule)) { - executeUpdate = true; + private RegisterRulesContext(Map dbRules) { + this.dbRules = ImmutableMap.copyOf(dbRules); + this.known = ImmutableSet.copyOf(dbRules.values()); } - if (mergeDebtDefinitions(ruleDef, rule)) { - executeUpdate = true; + private Optional getDbRuleFor(RulesDefinition.Rule ruleDef) { + RuleKey ruleKey = RuleKey.of(ruleDef.repository().key(), ruleDef.key()); + return Optional.ofNullable(dbRules.get(ruleKey)); } - if (mergeTags(ruleDef, rule)) { - executeUpdate = true; + private Stream getRemaining() { + Set res = new HashSet<>(dbRules.values()); + res.removeAll(unchanged); + res.removeAll(updated); + res.removeAll(removed); + return res.stream(); } - if (executeUpdate) { - update(session, rule); + private Stream getRemoved() { + return removed.stream(); } - mergeParams(ruleDef, rule, session); - updateDeprecatedKeys(ruleDef, rule, existingDeprecatedRuleKeys, session); - if (newRule || executeUpdate) { - return Optional.of(rule.getId()); + private Stream getAllModified() { + return Stream.of( + created.stream(), + updated.stream(), + removed.stream()) + .flatMap(s -> s); } - return Optional.empty(); - } - private Map loadRules(DbSession session) { - Map rules = new HashMap<>(); - for (RuleDefinitionDto rule : dbClient.ruleDao().selectAllDefinitions(session)) { - rules.put(rule.getKey(), rule); + private boolean isCreated(RuleDefinitionDto ruleDefinition) { + return created.contains(ruleDefinition); } - return rules; - } - private Map> loadDeprecatedRuleKeys(DbSession dbSession) { - return dbClient.ruleDao().selectAllDeprecatedRuleKeys(dbSession) - .stream() - .map(SingleDeprecatedRuleKey::from) - .collect(Collectors.groupingBy(SingleDeprecatedRuleKey::getRuleId, toSet())); - } + private boolean isUpdated(RuleDefinitionDto ruleDefinition) { + return updated.contains(ruleDefinition); + } - private List getRepositories(RulesDefinition.Context context) { - List repositories = new ArrayList<>(); - for (RulesDefinition.Repository repoDef : context.repositories()) { - repositories.add(repoDef); + private void created(RuleDefinitionDto ruleDefinition) { + checkState(!known.contains(ruleDefinition), "known RuleDefinitionDto can't be created"); + created.add(ruleDefinition); } - for (RulesDefinition.ExtendedRepository extendedRepoDef : context.extendedRepositories()) { - if (context.repository(extendedRepoDef.key()) == null) { - LOG.warn(format("Extension is ignored, repository %s does not exist", extendedRepoDef.key())); - } else { - repositories.add(extendedRepoDef); + + private void updated(RuleDefinitionDto ruleDefinition) { + ensureKnown(ruleDefinition); + updated.add(ruleDefinition); + } + + private void removed(RuleDefinitionDto ruleDefinition) { + ensureKnown(ruleDefinition); + removed.add(ruleDefinition); + } + + private void unchanged(RuleDefinitionDto ruleDefinition) { + ensureKnown(ruleDefinition); + unchanged.add(ruleDefinition); + } + + private void ensureKnown(RuleDefinitionDto ruleDefinition) { + if (!known.contains(ruleDefinition)) { + throw new IllegalArgumentException("unknown RuleDef"); } + checkArgument(known.contains(ruleDefinition), "unknown RuleDefinitionDto"); } - return repositories; + } + + /** + * @return the id of the rule if it's just been created or if it's been updated. + */ + private void registerRule(RegisterRulesContext recorder, RulesDefinition.Rule ruleDef, + Map> existingDeprecatedRuleKeys, DbSession session) { + RuleDefinitionDto ruleDefinitionDto = recorder.getDbRuleFor(ruleDef) + .orElseGet(() -> { + RuleDefinitionDto newRule = createRuleDto(ruleDef, session); + recorder.created(newRule); + return newRule; + }); + + if (mergeRule(ruleDef, ruleDefinitionDto)) { + recorder.updated(ruleDefinitionDto); + } + + if (mergeDebtDefinitions(ruleDef, ruleDefinitionDto)) { + recorder.updated(ruleDefinitionDto); + } + + if (mergeTags(ruleDef, ruleDefinitionDto)) { + recorder.updated(ruleDefinitionDto); + } + + if (recorder.isUpdated(ruleDefinitionDto)) { + update(session, ruleDefinitionDto); + } else if (!recorder.isCreated(ruleDefinitionDto)) { + recorder.unchanged(ruleDefinitionDto); + } + + mergeParams(ruleDef, ruleDefinitionDto, session); + updateDeprecatedKeys(ruleDef, ruleDefinitionDto, existingDeprecatedRuleKeys, session); } private RuleDefinitionDto createRuleDto(RulesDefinition.Rule ruleDef, DbSession session) { @@ -452,22 +517,6 @@ public class RegisterRules implements Startable { return changed; } - private static boolean mergeTags(RulesDefinition.Rule ruleDef, RuleDefinitionDto dto) { - boolean changed = false; - - if (RuleStatus.REMOVED == ruleDef.status()) { - dto.setSystemTags(emptySet()); - changed = true; - } else if (dto.getSystemTags().size() != ruleDef.tags().size() || - !dto.getSystemTags().containsAll(ruleDef.tags())) { - dto.setSystemTags(ruleDef.tags()); - // FIXME this can't be implemented easily with organization support: remove end-user tags that are now declared as system - // RuleTagHelper.applyTags(dto, ImmutableSet.copyOf(dto.getTags())); - changed = true; - } - return changed; - } - private void updateDeprecatedKeys(RulesDefinition.Rule ruleDef, RuleDefinitionDto rule, Map> existingDeprecatedKeysById, DbSession dbSession) { @@ -493,37 +542,51 @@ public class RegisterRules implements Startable { .setCreatedAt(system2.now()))); } - private List processRemainingDbRules(Collection existingRules, DbSession session) { + private static boolean mergeTags(RulesDefinition.Rule ruleDef, RuleDefinitionDto dto) { + boolean changed = false; + + if (RuleStatus.REMOVED == ruleDef.status()) { + dto.setSystemTags(emptySet()); + changed = true; + } else if (dto.getSystemTags().size() != ruleDef.tags().size() || + !dto.getSystemTags().containsAll(ruleDef.tags())) { + dto.setSystemTags(ruleDef.tags()); + // FIXME this can't be implemented easily with organization support: remove end-user tags that are now declared as system + // RuleTagHelper.applyTags(dto, ImmutableSet.copyOf(dto.getTags())); + changed = true; + } + return changed; + } + + private void processRemainingDbRules(RegisterRulesContext recorder, DbSession dbSession) { // custom rules check status of template, so they must be processed at the end List customRules = newArrayList(); - List removedRules = newArrayList(); - for (RuleDefinitionDto rule : existingRules) { + recorder.getRemaining().forEach(rule -> { if (rule.isCustomRule()) { customRules.add(rule); } else if (rule.getStatus() != RuleStatus.REMOVED) { - removeRule(session, removedRules, rule); + removeRule(dbSession, recorder, rule); } - } + }); for (RuleDefinitionDto customRule : customRules) { Integer templateId = customRule.getTemplateId(); checkNotNull(templateId, "Template id of the custom rule '%s' is null", customRule); - Optional template = dbClient.ruleDao().selectDefinitionById(templateId, session); + Optional template = dbClient.ruleDao().selectDefinitionById(templateId, dbSession); if (template.isPresent() && template.get().getStatus() != RuleStatus.REMOVED) { if (updateCustomRuleFromTemplateRule(customRule, template.get())) { - update(session, customRule); + update(dbSession, customRule); } } else { - removeRule(session, removedRules, customRule); + removeRule(dbSession, recorder, customRule); } } - session.commit(); - return removedRules; + dbSession.commit(); } - private void removeRule(DbSession session, List removedRules, RuleDefinitionDto rule) { + private void removeRule(DbSession session, RegisterRulesContext recorder, RuleDefinitionDto rule) { LOG.info(format("Disable rule %s", rule.getKey())); rule.setStatus(RuleStatus.REMOVED); rule.setSystemTags(emptySet()); @@ -531,8 +594,8 @@ public class RegisterRules implements Startable { // FIXME resetting the tags for all organizations must be handled a different way // rule.setTags(Collections.emptySet()); // update(session, rule.getMetadata()); - removedRules.add(rule); - if (removedRules.size() % 100 == 0) { + recorder.removed(rule); + if (recorder.getRemoved().count() % 100 == 0) { session.commit(); } } @@ -589,19 +652,22 @@ public class RegisterRules implements Startable { * The side effect of this approach is that extended repositories will not be managed the same way. * If an extended repository do not exists anymore, then related active rules will be removed. */ - private List removeActiveRulesOnStillExistingRepositories(DbSession dbSession, Collection removedRules, RulesDefinition.Context context) { - List repositoryKeys = newArrayList(Iterables.transform(context.repositories(), RulesDefinition.Repository::key)); + private List removeActiveRulesOnStillExistingRepositories(DbSession dbSession, RegisterRulesContext recorder, + List context) { + List repositoryKeys = context.stream() + .map(RulesDefinition.ExtendedRepository::key) + .collect(MoreCollectors.toList(context.size())); List changes = new ArrayList<>(); Profiler profiler = Profiler.create(Loggers.get(getClass())); - for (RuleDefinitionDto rule : removedRules) { + recorder.getRemoved().forEach(rule -> { // SONAR-4642 Remove active rules only when repository still exists if (repositoryKeys.contains(rule.getRepositoryKey())) { profiler.start(); changes.addAll(qProfileRules.deleteRule(dbSession, rule)); profiler.stopDebug(format("Remove active rule for rule %s", rule.getKey())); } - } + }); return changes; }