From c92cbc70428ff46873cbad56b274901238028c50 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Wed, 11 Jun 2014 16:26:01 +0200 Subject: [PATCH] SONAR-5137 do not propagate activation on descendants if they already activate the same rule --- .../qualityprofile/ActiveRuleChange.java | 6 ++ .../server/qualityprofile/RuleActivator.java | 66 ++++++++++------- .../qualityprofile/RuleActivatorContext.java | 8 +++ .../RuleActivatorMediumTest.java | 70 ++++++++++++++++++- 4 files changed, 122 insertions(+), 28 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java index 2932c90b43a..da012888e9b 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java @@ -106,6 +106,12 @@ public class ActiveRuleChange implements Loggable { return this; } + public ActiveRuleChange setParameters(Map m) { + parameters.clear(); + parameters.putAll(m); + return this; + } + @Override public Map getDetails() { ImmutableMap.Builder details = ImmutableMap.builder(); diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java index 7fe1b275f36..0187ceb3644 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java @@ -106,46 +106,45 @@ public class RuleActivator implements ServerComponent { context.verifyForActivation(); List changes = Lists.newArrayList(); ActiveRuleChange change; + boolean stopPropagation = false; + if (context.activeRule() == null) { + // new activation change = new ActiveRuleChange(ActiveRuleChange.Type.ACTIVATED, activation.getKey()); - - //Rules crated by default Inheritance if (activation.isCascade() || context.isSameAsParent(activation)) { change.setInheritance(ActiveRule.Inheritance.INHERITED); } + applySeverityAndParamToChange(activation, context, change); + } else { - change = new ActiveRuleChange(ActiveRuleChange.Type.UPDATED, activation.getKey()); - // Stop propagation on overriding profiles + // already activated + if (activation.isCascade() && context.activeRule().doesOverride()) { + // propagating to descendants, but child profile already overrides rule -> stop propagation return changes; } - - //Updates on rule that exists with a valid parent switch them to OVERRIDE - if (!activation.isCascade() && context.parentProfile() != null) { - change.setInheritance(context.isSameAsParent(activation) ? ActiveRule.Inheritance.INHERITED : ActiveRule.Inheritance.OVERRIDES); + change = new ActiveRuleChange(ActiveRuleChange.Type.UPDATED, activation.getKey()); + if (activation.isCascade() && context.activeRule().getInheritance() == null) { + // activate on child, then on parent -> mark child as overriding parent + change.setInheritance(ActiveRule.Inheritance.OVERRIDES); + change.setSeverity(context.activeRule().getSeverityString()); + change.setParameters(context.activeRuleParamsAsStringMap()); + stopPropagation = true; + } else { + applySeverityAndParamToChange(activation, context, change); + if (!activation.isCascade() && context.parentProfile() != null) { + // override rule which is already declared on parents + change.setInheritance(context.isSameAsParent(activation) ? ActiveRule.Inheritance.INHERITED : ActiveRule.Inheritance.OVERRIDES); + } } } - // Severity and parameter values are : - // 1. defined by end-user - // 2. else inherited from parent profile - // 3. else defined by rule defaults - change.setSeverity(StringUtils.defaultIfEmpty(activation.getSeverity(), context.defaultSeverity())); - for (RuleParamDto ruleParamDto : context.ruleParams()) { - String value = StringUtils.defaultIfEmpty( - activation.getParameters().get(ruleParamDto.getName()), - context.defaultParam(ruleParamDto.getName())); - verifyParam(ruleParamDto, value); - change.setParameter(ruleParamDto.getName(), StringUtils.defaultIfEmpty(value, ruleParamDto.getDefaultValue())); - } - changes.add(change); - // TODO filter changes without any differences - persist(change, context, dbSession); - // Execute the cascade on the child if NOT overrides - changes.addAll(cascadeActivation(dbSession, activation)); + if (!stopPropagation) { + changes.addAll(cascadeActivation(dbSession, activation)); + } if (!changes.isEmpty()) { log.write(dbSession, Log.Type.ACTIVE_RULE, changes); @@ -155,6 +154,23 @@ public class RuleActivator implements ServerComponent { return changes; } + /** + * Severity and parameter values are : + * 1. defined by end-user + * 2. else inherited from parent profile + * 3. else defined by rule defaults + */ + private void applySeverityAndParamToChange(RuleActivation activation, RuleActivatorContext context, ActiveRuleChange change) { + change.setSeverity(StringUtils.defaultIfEmpty(activation.getSeverity(), context.defaultSeverity())); + for (RuleParamDto ruleParamDto : context.ruleParams()) { + String value = StringUtils.defaultIfEmpty( + activation.getParameters().get(ruleParamDto.getName()), + context.defaultParam(ruleParamDto.getName())); + verifyParam(ruleParamDto, value); + change.setParameter(ruleParamDto.getName(), StringUtils.defaultIfEmpty(value, ruleParamDto.getDefaultValue())); + } + } + private List cascadeActivation(DbSession session, RuleActivation activation) { List changes = Lists.newArrayList(); diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java index f4f279fb1b4..f6a48fc20af 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java @@ -108,6 +108,14 @@ class RuleActivatorContext { return activeRuleParams; } + Map activeRuleParamsAsStringMap() { + Map params = Maps.newHashMap(); + for (Map.Entry param : activeRuleParams.entrySet()) { + params.put(param.getKey(), param.getValue().getValue()); + } + return params; + } + Map parentActiveRuleParamsAsStringMap() { Map params = Maps.newHashMap(); for (Map.Entry param : parentActiveRuleParams.entrySet()) { diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java index f894d5b42da..16c1e339a35 100644 --- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java @@ -469,14 +469,47 @@ public class RuleActivatorMediumTest { // change on parent -> do not propagate on children because they're overriding values activation = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1)); activation.setSeverity(Severity.CRITICAL); - activation.setParameter("max", "10"); + activation.setParameter("max", "9"); + ruleActivator.activate(activation); + dbSession.clearCache(); + verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.CRITICAL, null, ImmutableMap.of("max", "9")); + verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.OVERRIDES, ImmutableMap.of("max", "8")); + verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "8")); + + // reset on parent (use default severity and params) -> do not propagate on children because they're overriding values + activation = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1)); ruleActivator.activate(activation); dbSession.clearCache(); - verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.CRITICAL, null, ImmutableMap.of("max", "10")); + verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.MINOR, null, ImmutableMap.of("max", "10")); verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.OVERRIDES, ImmutableMap.of("max", "8")); verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "8")); } + @Test + public void active_on_parent_a_rule_already_activated_on_child() throws Exception { + createChildProfiles(); + + // activate on child profile + RuleActivation activation = new RuleActivation(ActiveRuleKey.of(XOO_CHILD_PROFILE_KEY, XOO_RULE_1)); + activation.setSeverity(Severity.INFO); + activation.setParameter("max", "7"); + ruleActivator.activate(activation); + verifyZeroActiveRules(XOO_PROFILE_KEY); + verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, null, ImmutableMap.of("max", "7")); + verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7")); + + // active the same rule on root profile -> mark the child profile as OVERRIDES + activation = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1)); + activation.setSeverity(Severity.MAJOR); + activation.setParameter("max", "8"); + ruleActivator.activate(activation); + dbSession.clearCache(); + verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.MAJOR, null, ImmutableMap.of("max", "8")); + verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, ActiveRuleDto.OVERRIDES, ImmutableMap.of("max", "7")); + verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7")); + } + + @Test public void do_not_override_on_child_if_same_values() throws Exception { createChildProfiles(); @@ -520,6 +553,37 @@ public class RuleActivatorMediumTest { verifyZeroActiveRules(XOO_GRAND_CHILD_PROFILE_KEY); } + @Test + public void propagate_deactivation_even_on_child_overrides() throws Exception { + createChildProfiles(); + + // activate on root profile + RuleActivation activation = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1)); + activation.setSeverity(Severity.INFO); + activation.setParameter("max", "7"); + ruleActivator.activate(activation); + verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.INFO, null, ImmutableMap.of("max", "7")); + verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7")); + verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.INFO, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7")); + + // override on child + activation = new RuleActivation(ActiveRuleKey.of(XOO_CHILD_PROFILE_KEY, XOO_RULE_1)); + activation.setSeverity(Severity.BLOCKER); + activation.setParameter("max", "8"); + ruleActivator.activate(activation); + dbSession.clearCache(); + verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.INFO, null, ImmutableMap.of("max", "7")); + verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.OVERRIDES, ImmutableMap.of("max", "8")); + verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "8")); + + // deactivate on parent -> do not propagate on children because they're overriding values + ruleActivator.deactivate(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1)); + dbSession.clearCache(); + verifyZeroActiveRules(XOO_PROFILE_KEY); + verifyZeroActiveRules(XOO_CHILD_PROFILE_KEY); + verifyZeroActiveRules(XOO_GRAND_CHILD_PROFILE_KEY); + } + @Test public void do_not_deactivate_inherited_or_overridden_rule() throws Exception { createChildProfiles(); @@ -617,7 +681,7 @@ public class RuleActivatorMediumTest { } @Test - public void bulk_activate_lot_of_rules() { + public void mass_activation() { // Generate more rules than the search's max limit int bulkSize = QueryOptions.MAX_LIMIT + 10; for (int i = 0; i < bulkSize; i++) { -- 2.39.5