From 087629fd0dff104aca147e5155574151e29c463d Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Wed, 17 Apr 2019 17:53:41 +0200 Subject: [PATCH] SONARCLOUD-582 load descendant profiles only if changes --- .../server/qualityprofile/RuleActivator.java | 8 +- .../BuiltInQProfileUpdateImplTest.java | 98 ++++++++++++++----- 2 files changed, 76 insertions(+), 30 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java index 835f4153227..8d36ea471c0 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java @@ -123,6 +123,7 @@ public class RuleActivator { } if (isSame(change, activeRule)) { change = null; + stopCascading = true; } } @@ -359,7 +360,7 @@ public class RuleActivator { builder.setDescendantProfilesSupplier(createDescendantProfilesSupplier(dbSession)); // load rules - List rules = completeWithRules(dbSession, builder, ruleIds); + completeWithRules(dbSession, builder, ruleIds); // load org profiles. Their parents are null by nature. List profiles = db.qualityProfileDao().selectQProfilesByRuleProfile(dbSession, builtInProfile); @@ -400,7 +401,7 @@ public class RuleActivator { return builder.build(); } - private DescendantProfilesSupplier createDescendantProfilesSupplier(DbSession dbSession) { + DescendantProfilesSupplier createDescendantProfilesSupplier(DbSession dbSession) { return (parents, ruleIds) -> { Collection profiles = db.qualityProfileDao().selectDescendants(dbSession, parents); Set ruleProfileUuids = profiles.stream() @@ -413,11 +414,10 @@ public class RuleActivator { }; } - private List completeWithRules(DbSession dbSession, RuleActivationContext.Builder builder, Collection ruleIds) { + private void completeWithRules(DbSession dbSession, RuleActivationContext.Builder builder, Collection ruleIds) { List rules = db.ruleDao().selectDefinitionByIds(dbSession, ruleIds); builder.setRules(rules); builder.setRuleParams(db.ruleDao().selectRuleParamsByRuleIds(dbSession, ruleIds)); - return rules; } private void completeWithActiveRules(DbSession dbSession, RuleActivationContext.Builder builder, Collection ruleIds, Collection ruleProfileUuids) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java index 654a0b86221..f15fe281fca 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java @@ -19,6 +19,7 @@ */ package org.sonar.server.qualityprofile; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Optional; @@ -33,6 +34,7 @@ import org.sonar.api.server.profile.BuiltInQualityProfilesDefinition; import org.sonar.api.server.profile.BuiltInQualityProfilesDefinition.NewBuiltInQualityProfile; import org.sonar.api.utils.System2; import org.sonar.api.utils.internal.TestSystem2; +import org.sonar.db.DbSession; import org.sonar.db.DbTester; import org.sonar.db.qualityprofile.ActiveRuleDto; import org.sonar.db.qualityprofile.ActiveRuleParamDto; @@ -49,6 +51,7 @@ import org.sonar.server.util.TypeValidations; import static java.util.Arrays.asList; import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.groups.Tuple.tuple; import static org.mockito.Mockito.mock; @@ -249,7 +252,7 @@ public class BuiltInQProfileUpdateImplTest { } @Test - public void propagate_new_activation_to_descendant_profiles() { + public void propagate_activation_to_descendant_profiles() { RuleDefinitionDto rule = db.rules().insert(r -> r.setLanguage("xoo")); QProfileDto profile = db.qualityProfiles().insert(db.getDefaultOrganization(), @@ -270,6 +273,69 @@ public class BuiltInQProfileUpdateImplTest { assertThatRuleIsActivated(grandchildProfile, rule, changes, rule.getSeverityString(), INHERITED, emptyMap()); } + @Test + public void do_not_load_descendants_if_no_changes() { + RuleDefinitionDto rule = db.rules().insert(r -> r.setLanguage("xoo")); + + QProfileDto profile = db.qualityProfiles().insert(db.getDefaultOrganization(), + p -> p.setLanguage(rule.getLanguage()).setIsBuiltIn(true)); + QProfileDto childProfile = createChildProfile(profile); + + BuiltInQualityProfilesDefinition.Context context = new BuiltInQualityProfilesDefinition.Context(); + NewBuiltInQualityProfile newQp = context.createBuiltInQualityProfile(profile.getName(), profile.getLanguage()); + newQp.activateRule(rule.getRepositoryKey(), rule.getRuleKey()); + newQp.done(); + + // first run + BuiltInQProfile builtIn = builtInProfileRepository.create(context.profile(profile.getLanguage(), profile.getName()), rule); + List changes = underTest.update(db.getSession(), builtIn, RulesProfileDto.from(profile)); + assertThat(changes).hasSize(2).extracting(ActiveRuleChange::getType).containsOnly(ActiveRuleChange.Type.ACTIVATED); + + // second run, without any input changes + RuleActivator ruleActivatorWithoutDescendants = new RuleActivator(system2, db.getDbClient(), typeValidations, userSession) { + @Override + DescendantProfilesSupplier createDescendantProfilesSupplier(DbSession dbSession) { + return (profiles, ruleIds) -> { + throw new IllegalStateException("BOOM - descendants should not be loaded"); + }; + } + }; + changes = new BuiltInQProfileUpdateImpl(db.getDbClient(), ruleActivatorWithoutDescendants, activeRuleIndexer).update(db.getSession(), builtIn, RulesProfileDto.from(profile)); + assertThat(changes).isEmpty(); + } + + @Test + public void propagate_deactivation_to_descendant_profiles() { + RuleDefinitionDto rule = db.rules().insert(r -> r.setLanguage("xoo")); + + QProfileDto profile = db.qualityProfiles().insert(db.getDefaultOrganization(), + p -> p.setLanguage(rule.getLanguage()).setIsBuiltIn(true)); + QProfileDto childProfile = createChildProfile(profile); + QProfileDto grandChildProfile = createChildProfile(childProfile); + + BuiltInQualityProfilesDefinition.Context context = new BuiltInQualityProfilesDefinition.Context(); + NewBuiltInQualityProfile newQp = context.createBuiltInQualityProfile(profile.getName(), profile.getLanguage()); + newQp.activateRule(rule.getRepositoryKey(), rule.getRuleKey()); + newQp.done(); + + // first run to activate the rule + BuiltInQProfile builtIn = builtInProfileRepository.create(context.profile(profile.getLanguage(), profile.getName()), rule); + List changes = underTest.update(db.getSession(), builtIn, RulesProfileDto.from(profile)); + assertThat(changes).hasSize(3).extracting(ActiveRuleChange::getType).containsOnly(ActiveRuleChange.Type.ACTIVATED); + + // second run to deactivate the rule + context = new BuiltInQualityProfilesDefinition.Context(); + NewBuiltInQualityProfile updatedQp = context.createBuiltInQualityProfile(profile.getName(), profile.getLanguage()); + updatedQp.done(); + builtIn = builtInProfileRepository.create(context.profile(profile.getLanguage(), profile.getName()), rule); + changes = underTest.update(db.getSession(), builtIn, RulesProfileDto.from(profile)); + assertThat(changes).hasSize(3).extracting(ActiveRuleChange::getType).containsOnly(ActiveRuleChange.Type.DEACTIVATED); + + assertThatRuleIsDeactivated(profile, rule); + assertThatRuleIsDeactivated(childProfile, rule); + assertThatRuleIsDeactivated(grandChildProfile, rule); + } + private QProfileDto createChildProfile(QProfileDto parent) { return db.qualityProfiles().insert(db.getDefaultOrganization(), p -> p .setLanguage(parent.getLanguage()) @@ -304,31 +370,6 @@ public class BuiltInQProfileUpdateImplTest { } } - // - // @Test - // public void deactivateOnBuiltInProfile_activate_rule_on_child_profiles() { - // RuleDefinitionDto rule = createJavaRule(); - // QProfileDto profile = db.qualityProfiles().insert(db.getDefaultOrganization(), - // p -> p.setLanguage(rule.getLanguage()) - // .setIsBuiltIn(true)); - // QProfileDto childProfile = createChildProfile(profile); - // QProfileDto grandchildProfile = createChildProfile(childProfile); - // - // List changes = underTest.activateOnBuiltInRulesProfile(db.getSession(), RuleActivation.create(rule.getKey()), - // RulesProfileDto.from(profile)); - // - // assertThatRuleIsActivated(profile, rule, changes, rule.getSeverityString(), null, emptyMap()); - // assertThatRuleIsActivated(childProfile, rule, changes, rule.getSeverityString(), INHERITED, emptyMap()); - // assertThatRuleIsActivated(grandchildProfile, rule, changes, rule.getSeverityString(), INHERITED, emptyMap()); - // - // changes = underTest.deactivateOnBuiltInRulesProfile(db.getSession(), RulesProfileDto.from(profile), rule.getKey(), false); - // - // assertThat(changes).hasSize(3); - // assertThatRuleIsNotPresent(profile, rule); - // assertThatRuleIsNotPresent(childProfile, rule); - // assertThatRuleIsNotPresent(grandchildProfile, rule); - // } - private static void assertThatRuleHasParams(DbTester db, ActiveRuleDto activeRule, Tuple... expectedParams) { assertThat(db.getDbClient().activeRuleDao().selectParamsByActiveRuleId(db.getSession(), activeRule.getId())) .extracting(ActiveRuleParamDto::getKey, ActiveRuleParamDto::getValue) @@ -362,6 +403,11 @@ public class BuiltInQProfileUpdateImplTest { assertThat(activeRule.getUpdatedAt()).isEqualTo(PAST); } + private void assertThatRuleIsDeactivated(QProfileDto profile, RuleDefinitionDto rule) { + Collection activeRules = db.getDbClient().activeRuleDao().selectByRulesAndRuleProfileUuids(db.getSession(), singletonList(rule.getId()), singletonList(profile.getRulesProfileUuid())); + assertThat(activeRules).isEmpty(); + } + private static void assertThatRuleIsDeactivated(List activeRules, RuleDefinitionDto rule) { assertThat(findRule(activeRules, rule)).isEmpty(); } -- 2.39.5