From 78b100c449f04250acfe60a4a2b8150bfb5d0e50 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Mon, 31 Mar 2014 13:39:24 +0200 Subject: [PATCH] SONAR-5056 Reindex rules in E/S when migrating requirements to rules --- .../org/sonar/server/rule/RegisterRules.java | 85 +++++++++---------- .../org/sonar/server/rule/RuleRegistry.java | 38 ++++++++- ...equirementsFromCharacteristicsToRules.java | 22 +++-- .../qualityprofile/ESActiveRuleTest.java | 2 +- .../QProfileRuleLookupTest.java | 5 +- .../sonar/server/rule/RuleRegistryTest.java | 24 +++++- ...rementsFromCharacteristicsToRulesTest.java | 9 +- 7 files changed, 124 insertions(+), 61 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java index a8dcf16c09e..41dfcd72a99 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -107,12 +107,8 @@ public class RegisterRules implements Startable { try { RulesDefinition.Context context = defLoader.load(); Buffer buffer = new Buffer(system.now()); - List characteristicDtos = characteristicDao.selectEnabledCharacteristics(); - for (CharacteristicDto characteristicDto : characteristicDtos) { - buffer.add(characteristicDto); - } selectRulesFromDb(buffer, sqlSession); - enableRuleDefinitions(context, buffer, characteristicDtos, sqlSession); + enableRuleDefinitions(context, buffer, sqlSession); List removedRules = processRemainingDbRules(buffer, sqlSession); removeActiveRulesOnStillExistingRepositories(removedRules, context); index(buffer); @@ -144,29 +140,32 @@ public class RegisterRules implements Startable { for (RuleRuleTagDto tagDto : ruleDao.selectTags(sqlSession)) { buffer.add(tagDto); } + for (CharacteristicDto characteristicDto : characteristicDao.selectEnabledCharacteristics()) { + buffer.add(characteristicDto); + } } - private void enableRuleDefinitions(RulesDefinition.Context context, Buffer buffer, List characteristicDtos, SqlSession sqlSession) { + private void enableRuleDefinitions(RulesDefinition.Context context, Buffer buffer, SqlSession sqlSession) { for (RulesDefinition.Repository repoDef : context.repositories()) { - enableRepository(buffer, sqlSession, repoDef, characteristicDtos); + enableRepository(buffer, sqlSession, repoDef); } for (RulesDefinition.ExtendedRepository extendedRepoDef : context.extendedRepositories()) { if (context.repository(extendedRepoDef.key()) == null) { LOG.warn(String.format("Extension is ignored, repository %s does not exist", extendedRepoDef.key())); } else { - enableRepository(buffer, sqlSession, extendedRepoDef, characteristicDtos); + enableRepository(buffer, sqlSession, extendedRepoDef); } } } - private void enableRepository(Buffer buffer, SqlSession sqlSession, RulesDefinition.ExtendedRepository repoDef, List characteristicDtos) { + private void enableRepository(Buffer buffer, SqlSession sqlSession, RulesDefinition.ExtendedRepository repoDef) { int count = 0; for (RulesDefinition.Rule ruleDef : repoDef.rules()) { RuleDto dto = buffer.rule(RuleKey.of(ruleDef.repository().key(), ruleDef.key())); if (dto == null) { - dto = enableAndInsert(buffer, sqlSession, ruleDef, characteristicDtos); + dto = enableAndInsert(buffer, sqlSession, ruleDef); } else { - enableAndUpdate(buffer, sqlSession, ruleDef, dto, characteristicDtos); + enableAndUpdate(buffer, sqlSession, ruleDef, dto); } buffer.markProcessed(dto); count++; @@ -177,7 +176,7 @@ public class RegisterRules implements Startable { sqlSession.commit(); } - private RuleDto enableAndInsert(Buffer buffer, SqlSession sqlSession, RulesDefinition.Rule ruleDef, List characteristicDtos) { + private RuleDto enableAndInsert(Buffer buffer, SqlSession sqlSession, RulesDefinition.Rule ruleDef) { RuleDto ruleDto = new RuleDto() .setCardinality(ruleDef.template() ? Cardinality.MULTIPLE : Cardinality.SINGLE) .setConfigKey(ruleDef.internalKey()) @@ -191,7 +190,7 @@ public class RegisterRules implements Startable { .setUpdatedAt(buffer.now()) .setStatus(ruleDef.status().name()); - CharacteristicDto characteristic = findCharacteristic(ruleDef, null, characteristicDtos); + CharacteristicDto characteristic = buffer.characteristic(ruleDef.debtSubCharacteristic(), ruleDef.repository().key(), ruleDef.key(), null); DebtRemediationFunction remediationFunction = ruleDef.debtRemediationFunction(); if (characteristic != null && remediationFunction != null) { ruleDto.setDefaultSubCharacteristicId(characteristic.getId()) @@ -218,8 +217,8 @@ public class RegisterRules implements Startable { return ruleDto; } - private void enableAndUpdate(Buffer buffer, SqlSession sqlSession, RulesDefinition.Rule ruleDef, RuleDto dto, List characteristicDtos) { - if (mergeRule(buffer, ruleDef, dto, characteristicDtos)) { + private void enableAndUpdate(Buffer buffer, SqlSession sqlSession, RulesDefinition.Rule ruleDef, RuleDto dto) { + if (mergeRule(buffer, ruleDef, dto)) { ruleDao.update(dto); } mergeParams(buffer, sqlSession, ruleDef, dto); @@ -227,7 +226,7 @@ public class RegisterRules implements Startable { buffer.markProcessed(dto); } - private boolean mergeRule(Buffer buffer, RulesDefinition.Rule def, RuleDto dto, List characteristicDtos) { + private boolean mergeRule(Buffer buffer, RulesDefinition.Rule def, RuleDto dto) { boolean changed = false; if (!StringUtils.equals(dto.getName(), def.name())) { dto.setName(def.name()); @@ -260,17 +259,17 @@ public class RegisterRules implements Startable { dto.setLanguage(def.repository().language()); changed = true; } - changed = mergeDebtDefinitions(def, dto, characteristicDtos) || changed; + CharacteristicDto characteristic = buffer.characteristic(def.debtSubCharacteristic(), def.repository().key(), def.key(), dto.getSubCharacteristicId()); + changed = mergeDebtDefinitions(def, dto, characteristic) || changed; if (changed) { dto.setUpdatedAt(buffer.now()); } return changed; } - private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, List characteristicDtos) { + private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, @Nullable CharacteristicDto characteristic) { boolean changed = false; - CharacteristicDto characteristic = findCharacteristic(def, dto.getSubCharacteristicId(), characteristicDtos); // Debt definitions are set to null if the characteristic is null or unknown boolean hasCharacteristic = characteristic != null; DebtRemediationFunction debtRemediationFunction = characteristic != null ? def.debtRemediationFunction() : null; @@ -553,34 +552,32 @@ public class RegisterRules implements Startable { void markProcessed(RuleDto ruleDto) { unprocessedRuleIds.remove(ruleDto.getId()); } - } - @CheckForNull - private CharacteristicDto findCharacteristic(RulesDefinition.Rule ruleDef, @Nullable Integer overridingCharacteristicId, List characteristicDtos) { - String key = ruleDef.debtSubCharacteristic(); - // Rule is not linked to a default characteristic or characteristic has been disabled by user, nothing to do - if (key == null) { - return null; - } - CharacteristicDto characteristicDto = findCharacteristic(key, characteristicDtos); - if (characteristicDto == null) { - // Log a warning only if rule has not been overridden by user - if (overridingCharacteristicId == null) { - LOG.warn(String.format("Characteristic '%s' has not been found on rule '%s:%s'", key, ruleDef.repository().key(), ruleDef.key())); + CharacteristicDto characteristic(@Nullable String subCharacteristic, String repo, String ruleKey, @Nullable Integer overridingCharacteristicId){ + // Rule is not linked to a default characteristic or characteristic has been disabled by user + if (subCharacteristic == null) { + return null; } - } else if (characteristicDto.getParentId() == null) { - throw MessageException.of(String.format("Rule '%s:%s' cannot be linked on the root characteristic '%s'", ruleDef.repository().key(), ruleDef.key(), key)); + CharacteristicDto characteristicDto = findCharacteristic(subCharacteristic); + if (characteristicDto == null) { + // Log a warning only if rule has not been overridden by user + if (overridingCharacteristicId == null) { + LOG.warn(String.format("Characteristic '%s' has not been found on rule '%s:%s'", subCharacteristic, repo, ruleKey)); + } + } else if (characteristicDto.getParentId() == null) { + throw MessageException.of(String.format("Rule '%s:%s' cannot be linked on the root characteristic '%s'", repo, ruleKey, subCharacteristic)); + } + return characteristicDto; } - return characteristicDto; - } - @CheckForNull - private CharacteristicDto findCharacteristic(final String key, List characteristicDtos) { - return Iterables.find(characteristicDtos, new Predicate() { - @Override - public boolean apply(CharacteristicDto input) { - return key.equals(input.getKey()); - } - }, null); + @CheckForNull + private CharacteristicDto findCharacteristic(final String key) { + return Iterables.find(characteristicsById.values(), new Predicate() { + @Override + public boolean apply(@Nullable CharacteristicDto input) { + return input != null && key.equals(input.getKey()); + } + }, null); + } } } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java index ce9c37ac918..ea7611f44ca 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java @@ -20,10 +20,12 @@ package org.sonar.server.rule; +import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList.Builder; import com.google.common.collect.Multimap; import org.apache.commons.lang.StringUtils; +import org.apache.ibatis.session.SqlSession; import org.elasticsearch.ElasticSearchException; import org.elasticsearch.common.collect.Lists; import org.elasticsearch.common.collect.Maps; @@ -40,7 +42,9 @@ import org.elasticsearch.search.SearchHits; import org.elasticsearch.search.sort.SortOrder; import org.sonar.api.rule.RuleKey; import org.sonar.api.utils.TimeProfiler; +import org.sonar.core.persistence.MyBatis; import org.sonar.core.rule.*; +import org.sonar.core.technicaldebt.db.CharacteristicDao; import org.sonar.core.technicaldebt.db.CharacteristicDto; import org.sonar.server.es.ESIndex; import org.sonar.server.es.SearchQuery; @@ -59,6 +63,7 @@ import java.util.List; import java.util.Map; import static com.google.common.collect.Lists.newArrayList; +import static com.google.common.collect.Maps.newHashMap; import static org.elasticsearch.index.query.FilterBuilders.*; import static org.sonar.api.rules.Rule.STATUS_REMOVED; @@ -75,17 +80,48 @@ public class RuleRegistry { private static final String PARAM_STATUS = "status"; private final ESIndex searchIndex; + private final MyBatis myBatis; private final RuleDao ruleDao; + private final CharacteristicDao characteristicDao; - public RuleRegistry(ESIndex searchIndex, RuleDao ruleDao) { + public RuleRegistry(ESIndex searchIndex, MyBatis myBatis, RuleDao ruleDao, CharacteristicDao characteristicDao) { this.searchIndex = searchIndex; + this.myBatis = myBatis; this.ruleDao = ruleDao; + this.characteristicDao = characteristicDao; } public void start() { searchIndex.addMappingFromClasspath(INDEX_RULES, TYPE_RULE, "/org/sonar/server/es/config/mappings/rule_mapping.json"); } + public void reindexRules() { + SqlSession sqlSession = myBatis.openSession(); + try { + Multimap paramsByRuleId = ArrayListMultimap.create(); + Multimap tagsByRuleId = ArrayListMultimap.create(); + Map characteristicsById = newHashMap(); + + for (RuleParamDto paramDto : ruleDao.selectParameters(sqlSession)) { + paramsByRuleId.put(paramDto.getRuleId(), paramDto); + } + for (RuleRuleTagDto tagDto : ruleDao.selectTags(sqlSession)) { + tagsByRuleId.put(tagDto.getRuleId(), tagDto); + } + for (CharacteristicDto characteristicDto : characteristicDao.selectEnabledCharacteristics(sqlSession)) { + characteristicsById.put(characteristicDto.getId(), characteristicDto); + } + + bulkIndexRules( + ruleDao.selectEnablesAndNonManual(sqlSession), + characteristicsById, + paramsByRuleId, + tagsByRuleId); + } finally { + sqlSession.close(); + } + } + public void bulkRegisterRules(Collection rules, Map characteristicByRule, Multimap paramsByRule, Multimap tagsByRule) { String[] ids = bulkIndexRules(rules, characteristicByRule, paramsByRule, tagsByRule); diff --git a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java index cbf1e521f53..195d873bd63 100644 --- a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java +++ b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java @@ -40,6 +40,7 @@ import org.sonar.core.technicaldebt.db.RequirementDto; import org.sonar.server.db.migrations.MassUpdater; import org.sonar.server.db.migrations.SqlUtil; import org.sonar.server.rule.RegisterRules; +import org.sonar.server.rule.RuleRegistry; import javax.annotation.CheckForNull; import javax.annotation.Nullable; @@ -68,37 +69,44 @@ public class CopyRequirementsFromCharacteristicsToRules { private final RequirementDao requirementDao; + private final RuleRegistry ruleRegistry; + /** * @param registerRules used only to be started after init of rules */ - public CopyRequirementsFromCharacteristicsToRules(Database database, RequirementDao requirementDao, ServerUpgradeStatus status, RegisterRules registerRules) { - this(database, requirementDao, status, System2.INSTANCE); + public CopyRequirementsFromCharacteristicsToRules(Database database, RequirementDao requirementDao, ServerUpgradeStatus status, RuleRegistry ruleRegistry, + RegisterRules registerRules) { + this(database, requirementDao, ruleRegistry, status, System2.INSTANCE); } @VisibleForTesting - CopyRequirementsFromCharacteristicsToRules(Database database, RequirementDao requirementDao, ServerUpgradeStatus status, System2 system2) { + CopyRequirementsFromCharacteristicsToRules(Database database, RequirementDao requirementDao, RuleRegistry ruleRegistry, ServerUpgradeStatus status, System2 system2) { this.db = database; this.system2 = system2; this.status = status; this.requirementDao = requirementDao; + this.ruleRegistry = ruleRegistry; } public void start() { - if (mustDoPurge()) { - doPurge(); + if (mustDoExecute()) { + doExecute(); } } - private boolean mustDoPurge() { + private boolean mustDoExecute() { return status.isUpgraded() && status.getInitialDbVersion() <= 520; } - private void doPurge() { + private void doExecute() { LOGGER.info("Copying requirement from characteristics to rules"); copyRequirementsFromCharacteristicsToRules(); LOGGER.info("Deleting requirements data"); removeRequirementsDataFromCharacteristics(); + + LOGGER.info("Reindex rules in E/S"); + ruleRegistry.reindexRules(); } private void copyRequirementsFromCharacteristicsToRules() { diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/ESActiveRuleTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/ESActiveRuleTest.java index d9f019634b2..ad5e0a6616b 100644 --- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/ESActiveRuleTest.java +++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/ESActiveRuleTest.java @@ -92,7 +92,7 @@ public class ESActiveRuleTest { searchIndex = new ESIndex(node, profiling); searchIndex.start(); - RuleRegistry esRule = new RuleRegistry(searchIndex, null); + RuleRegistry esRule = new RuleRegistry(searchIndex, null, null, null); esRule.start(); esActiveRule = new ESActiveRule(searchIndex, activeRuleDao, myBatis, profiling); esActiveRule.start(); diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleLookupTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleLookupTest.java index c02b3708437..0e114e9e984 100644 --- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleLookupTest.java +++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleLookupTest.java @@ -19,8 +19,6 @@ */ package org.sonar.server.qualityprofile; -import org.sonar.server.paging.Paging; - import com.github.tlrx.elasticsearch.test.EsSetup; import org.apache.commons.io.IOUtils; import org.elasticsearch.client.Requests; @@ -33,6 +31,7 @@ import org.sonar.api.rule.Severity; import org.sonar.core.profiling.Profiling; import org.sonar.server.es.ESIndex; import org.sonar.server.es.ESNode; +import org.sonar.server.paging.Paging; import org.sonar.server.rule.RuleRegistry; import org.sonar.test.TestUtils; @@ -62,7 +61,7 @@ public class QProfileRuleLookupTest { settings.setProperty("sonar.log.profilingLevel", "FULL"); ESIndex index = new ESIndex(searchNode, new Profiling(settings)); index.start(); - RuleRegistry registry = new RuleRegistry(index, null); + RuleRegistry registry = new RuleRegistry(index, null, null, null); registry.start(); ESActiveRule esActiveRule = new ESActiveRule(index, null, null, null); esActiveRule.start(); diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleRegistryTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleRegistryTest.java index 43b9164b507..3fdab92d790 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleRegistryTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleRegistryTest.java @@ -38,6 +38,7 @@ import org.sonar.api.rule.Severity; import org.sonar.core.persistence.MyBatis; import org.sonar.core.profiling.Profiling; import org.sonar.core.rule.*; +import org.sonar.core.technicaldebt.db.CharacteristicDao; import org.sonar.core.technicaldebt.db.CharacteristicDto; import org.sonar.server.es.ESIndex; import org.sonar.server.es.ESNode; @@ -51,8 +52,7 @@ import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.Maps.newHashMap; import static org.fest.assertions.Assertions.assertThat; import static org.mockito.Matchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @RunWith(MockitoJUnitRunner.class) public class RuleRegistryTest { @@ -61,11 +61,14 @@ public class RuleRegistryTest { ESIndex searchIndex; + @Mock + MyBatis myBatis; + @Mock RuleDao ruleDao; @Mock - MyBatis myBatis; + CharacteristicDao characteristicDao; @Mock SqlSession session; @@ -89,7 +92,7 @@ public class RuleRegistryTest { searchIndex = new ESIndex(node, profiling); searchIndex.start(); - registry = new RuleRegistry(searchIndex, ruleDao); + registry = new RuleRegistry(searchIndex, myBatis, ruleDao, characteristicDao); registry.start(); esSetup.execute( @@ -231,6 +234,19 @@ public class RuleRegistryTest { assertThat((List) rule2Document.get(RuleDocument.FIELD_ADMIN_TAGS)).hasSize(1); } + @Test + public void reindex_all_rules() { + SqlSession session = mock(SqlSession.class); + when(myBatis.openSession()).thenReturn(session); + + registry.reindexRules(); + + verify(ruleDao).selectEnablesAndNonManual(session); + verify(ruleDao).selectParameters(session); + verify(ruleDao).selectTags(session); + verify(characteristicDao).selectEnabledCharacteristics(session); + } + @Test public void index_and_reindex_single_rule() { RuleDto rule = new RuleDto(); diff --git a/sonar-server/src/test/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRulesTest.java b/sonar-server/src/test/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRulesTest.java index f7d48d21523..2fda3e16e20 100644 --- a/sonar-server/src/test/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRulesTest.java +++ b/sonar-server/src/test/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRulesTest.java @@ -31,8 +31,10 @@ import org.sonar.api.utils.System2; import org.sonar.core.persistence.AbstractDaoTestCase; import org.sonar.core.persistence.TestDatabase; import org.sonar.core.technicaldebt.db.RequirementDao; +import org.sonar.server.rule.RuleRegistry; import static org.fest.assertions.Assertions.assertThat; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) @@ -47,12 +49,15 @@ public class CopyRequirementsFromCharacteristicsToRulesTest extends AbstractDaoT @Mock System2 system2; + @Mock + RuleRegistry ruleRegistry; + CopyRequirementsFromCharacteristicsToRules service; @Before public void setUp() throws Exception { when(system2.now()).thenReturn(DateUtils.parseDateTime("2014-03-13T19:10:03+0100").getTime()); - service = new CopyRequirementsFromCharacteristicsToRules(db.database(), new RequirementDao(getMyBatis()), status, system2); + service = new CopyRequirementsFromCharacteristicsToRules(db.database(), new RequirementDao(getMyBatis()), ruleRegistry, status, system2); } @Test @@ -66,6 +71,7 @@ public class CopyRequirementsFromCharacteristicsToRulesTest extends AbstractDaoT service.start(); db.assertDbUnit(getClass(), "copy_requirements_from_characteristics_to_rules_result.xml", "rules"); + verify(ruleRegistry).reindexRules(); } @Test @@ -78,6 +84,7 @@ public class CopyRequirementsFromCharacteristicsToRulesTest extends AbstractDaoT service.start(); db.assertDbUnit(getClass(), "remove_requirements_data_from_characteristics_result.xml", "characteristics"); + verify(ruleRegistry).reindexRules(); } @Test -- 2.39.5