SONAR-5007 optimize changes of rule activation

5 files changed, 69 insertions, 13 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 59650b8ecdd..f783170a5c2 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
@@ -19,6 +19,7 @@
  */
 package org.sonar.server.qualityprofile;
 
+import com.google.common.base.Objects;
 import com.google.common.collect.Maps;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.core.activity.ActivityLog;
@@ -41,12 +42,9 @@ public class ActiveRuleChange implements ActivityLog {
   private ActiveRule.Inheritance inheritance = null;
   private Map<String, String> parameters = Maps.newHashMap();
 
-  private long start;
-
   private ActiveRuleChange(Type type, ActiveRuleKey key) {
     this.type = type;
     this.key = key;
-    this.start = System.currentTimeMillis();
   }
 
   public ActiveRuleKey getKey() {
@@ -123,4 +121,15 @@ public class ActiveRuleChange implements ActivityLog {
   public static ActiveRuleChange createFor(Type type, ActiveRuleKey key) {
     return new ActiveRuleChange(type, key);
   }
+
+  @Override
+  public String toString() {
+    return Objects.toStringHelper(this)
+      .add("type", type)
+      .add("key", key)
+      .add("severity", severity)
+      .add("inheritance", inheritance)
+      .add("parameters", parameters)
+      .toString();
+  }
 }
diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
index e7c856962f5..e6c293cf88d 100644
--- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
+++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
@@ -57,7 +57,7 @@ public class RuleActivation {
   /**
    * For internal use
    */
-  boolean isReset() {
+  boolean useDefaults() {
     return severity == null && parameters.isEmpty();
   }
 
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 a4fc4d8cfeb..318ed039670 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
@@ -124,15 +124,20 @@ public class RuleActivator implements ServerComponent {
         stopPropagation = true;
       } else {
         applySeverityAndParamToChange(activation, context, change);
-        if (!activation.isCascade() && context.parentProfile() != null) {
+        if (!activation.isCascade() && context.parentActiveRule() != null) {
           // override rule which is already declared on parents
           change.setInheritance(context.isSameAsParent(activation) ? ActiveRule.Inheritance.INHERITED : ActiveRule.Inheritance.OVERRIDES);
         }
       }
+      if (context.isSame(change)) {
+        change = null;
+      }
     }
 
-    changes.add(change);
-    persist(change, context, dbSession);
+    if (change != null) {
+      changes.add(change);
+      persist(change, context, dbSession);
+    }
 
     if (!stopPropagation) {
       changes.addAll(cascadeActivation(dbSession, activation));
@@ -339,7 +344,7 @@ public class RuleActivator implements ServerComponent {
         } catch (BadRequestException e) {
           // other exceptions stop the bulk activation
           result.incrementFailed();
-          // TODO result.addMessage
+          result.getErrors().add(e.errors());
         }
       }
       dbSession.commit();
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 1ab2b1ef8c3..82d1d86827b 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
@@ -77,8 +77,8 @@ class RuleActivatorContext {
   }
 
   @CheckForNull
-  QualityProfileDto parentProfile() {
-    return parentProfile;
+  ActiveRuleDto parentActiveRule() {
+    return parentActiveRule;
   }
 
   RuleActivatorContext setParentProfile(@Nullable QualityProfileDto p) {
@@ -171,7 +171,7 @@ class RuleActivatorContext {
     if (parentActiveRule == null) {
       return false;
     }
-    if (activation.isReset()) {
+    if (activation.useDefaults()) {
       return true;
     }
     if (StringUtils.equals(activation.getSeverity(), parentActiveRule.getSeverityString())) {
@@ -180,6 +180,22 @@ class RuleActivatorContext {
     return false;
   }
 
+  boolean isSame(ActiveRuleChange change) {
+    if (change.getInheritance()!=null && !change.getInheritance().name().equals(activeRule.getInheritance())) {
+      return false;
+    }
+    if (change.getSeverity()!=null && !change.getSeverity().equals(activeRule.getSeverityString())) {
+      return false;
+    }
+    for (Map.Entry<String, String> changeParam : change.getParameters().entrySet()) {
+      ActiveRuleParamDto param = activeRuleParams.get(changeParam.getKey());
+      if (param != null && !StringUtils.equals(changeParam.getValue(), param.getValue())) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   void verifyForActivation() {
     if (RuleStatus.REMOVED == rule.getStatus()) {
       throw new BadRequestException("Rule was removed: " + rule.getKey());
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 31c2ecb28da..15fc453695b 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
@@ -194,6 +194,22 @@ public class RuleActivatorMediumTest {
   }
 
   @Test
+  public void ignore_activation_without_changes() throws Exception {
+    // initial activation
+    ActiveRuleKey activeRuleKey = ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1);
+    RuleActivation activation = new RuleActivation(activeRuleKey);
+    activation.setSeverity(Severity.BLOCKER);
+    ruleActivator.activate(activation);
+    dbSession.clearCache();
+
+    // update with exactly the same severity and params
+    RuleActivation update = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1));
+    update.setSeverity(Severity.BLOCKER);
+    List<ActiveRuleChange> changes = ruleActivator.activate(update);
+    assertThat(changes).isEmpty();
+  }
+
+  @Test
   public void revert_activation_to_default_severity_and_parameters() throws Exception {
     // initial activation
     ActiveRuleKey activeRuleKey = ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1);
@@ -396,7 +412,7 @@ public class RuleActivatorMediumTest {
 
   // INHERITANCE OF PROFILES
   @Test
-  public void activate_on_child_profile() throws Exception {
+  public void activate_on_child_profile_but_not_on_parent() throws Exception {
     createChildProfiles();
 
     // activate on child profile, but not on root
@@ -408,6 +424,15 @@ public class RuleActivatorMediumTest {
     verifyZeroActiveRules(XOO_PROFILE_KEY);
     verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, null, ImmutableMap.of("max", "7"));
     verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7"));
+
+    // update severity on child
+    activation = new RuleActivation(ActiveRuleKey.of(XOO_CHILD_PROFILE_KEY, XOO_RULE_1));
+    activation.setSeverity(Severity.MINOR);
+    activation.setParameter("max", "77");
+    ruleActivator.activate(activation);
+    verifyZeroActiveRules(XOO_PROFILE_KEY);
+    verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.MINOR, null, ImmutableMap.of("max", "77"));
+    verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.MINOR, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "77"));
   }
 
   @Test
@@ -418,8 +443,9 @@ public class RuleActivatorMediumTest {
     RuleActivation activation = new RuleActivation(ActiveRuleKey.of(XOO_PROFILE_KEY, XOO_RULE_1));
     activation.setSeverity(Severity.BLOCKER);
     activation.setParameter("max", "7");
-    ruleActivator.activate(activation);
+    List<ActiveRuleChange> changes = ruleActivator.activate(activation);
 
+    assertThat(changes).hasSize(3);
     verifyOneActiveRule(XOO_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, null, ImmutableMap.of("max", "7"));
     verifyOneActiveRule(XOO_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7"));
     verifyOneActiveRule(XOO_GRAND_CHILD_PROFILE_KEY, XOO_RULE_1, Severity.BLOCKER, ActiveRuleDto.INHERITED, ImmutableMap.of("max", "7"));