SONAR-5137 do not propagate activation on descendants if they already activate the same rule

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<String,String> m) {
+    parameters.clear();
+    parameters.putAll(m);
+    return this;
+  }
+
   @Override
   public Map<String, String> getDetails() {
     ImmutableMap.Builder<String, String> details = ImmutableMap.<String, String>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<ActiveRuleChange> 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<ActiveRuleChange> cascadeActivation(DbSession session, RuleActivation activation) {
     List<ActiveRuleChange> 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<String, String> activeRuleParamsAsStringMap() {
+    Map<String, String> params = Maps.newHashMap();
+    for (Map.Entry<String, ActiveRuleParamDto> param : activeRuleParams.entrySet()) {
+      params.put(param.getKey(), param.getValue().getValue());
+    }
+    return params;
+  }
+
   Map<String, String> parentActiveRuleParamsAsStringMap() {
     Map<String, String> params = Maps.newHashMap();
     for (Map.Entry<String, ActiveRuleParamDto> 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,15 +469,48 @@ 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();
 
@@ -521,6 +554,37 @@ public class RuleActivatorMediumTest {
   }
 
   @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++) {