SONAR-10052 minor refactoring to better understand rule activation

5 files changed, 96 insertions, 90 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java
index 25024367f00..2017931e574 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java
@@ -112,10 +112,10 @@ public class RegisterQualityProfiles {
     builtInQProfileInsert.create(dbSession, batchDbSession, builtIn);
   }
 
-  private List<ActiveRuleChange> update(DbSession dbSession, BuiltInQProfile builtIn, RulesProfileDto ruleProfile) {
-    LOGGER.info("Update profile {}", builtIn.getQProfileName());
+  private List<ActiveRuleChange> update(DbSession dbSession, BuiltInQProfile definition, RulesProfileDto dbProfile) {
+    LOGGER.info("Update profile {}", definition.getQProfileName());
 
-    return builtInQProfileUpdate.update(dbSession, builtIn, ruleProfile);
+    return builtInQProfileUpdate.update(dbSession, definition, dbProfile);
   }
 
   /**
diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
index d195c913582..bede1be50bd 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java
@@ -28,6 +28,9 @@ import javax.annotation.concurrent.Immutable;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rule.Severity;
 
+/**
+ * The request for activation.
+ */
 @Immutable
 public class RuleActivation {
 
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 e2ab8cab385..8ccc75b07a2 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
@@ -83,10 +83,9 @@ public class RuleActivator {
     return doActivate(dbSession, activation, context);
   }
 
-  public List<ActiveRuleChange> activateAndCommit(DbSession dbSession, RuleActivation activation, QProfileDto profile) {
+  public void activateAndCommit(DbSession dbSession, RuleActivation activation, QProfileDto profile) {
     List<ActiveRuleChange> changes = activate(dbSession, activation, profile);
     activeRuleIndexer.commitAndIndex(dbSession, changes);
-    return changes;
   }
 
   public List<ActiveRuleChange> activate(DbSession dbSession, RuleActivation activation, QProfileDto profile) {
@@ -100,14 +99,14 @@ public class RuleActivator {
     ActiveRuleChange change;
     boolean stopPropagation = false;
 
-    ActiveRuleDto activeRule = context.activeRule();
+    ActiveRuleDto activeRule = context.getActiveRule();
     if (activeRule == null) {
       if (activation.isReset()) {
         // ignore reset when rule is not activated
         return changes;
       }
       // new activation
-      change = new ActiveRuleChange(ActiveRuleChange.Type.ACTIVATED, context.activeRuleKey());
+      change = new ActiveRuleChange(ActiveRuleChange.Type.ACTIVATED, context.getActiveRuleKey());
       applySeverityAndParamToChange(activation, context, change);
       if (context.isCascade() || context.isSameAsParent(change)) {
         change.setInheritance(ActiveRule.Inheritance.INHERITED);
@@ -118,16 +117,16 @@ public class RuleActivator {
         // propagating to descendants, but child profile already overrides rule -> stop propagation
         return changes;
       }
-      change = new ActiveRuleChange(ActiveRuleChange.Type.UPDATED, context.activeRuleKey());
+      change = new ActiveRuleChange(ActiveRuleChange.Type.UPDATED, context.getActiveRuleKey());
       if (context.isCascade() && activeRule.getInheritance() == null) {
         // activate on child, then on parent -> mark child as overriding parent
         change.setInheritance(ActiveRule.Inheritance.OVERRIDES);
-        change.setSeverity(context.currentSeverity());
+        change.setSeverity(context.getActiveSeverityBeforeChange());
         change.setParameters(context.activeRuleParamsAsStringMap());
         stopPropagation = true;
       } else {
         applySeverityAndParamToChange(activation, context, change);
-        if (!context.isCascade() && context.parentActiveRule() != null) {
+        if (!context.isCascade() && context.getParentActiveRule() != null) {
           // override rule which is already declared on parents
           change.setInheritance(context.isSameAsParent(change) ? ActiveRule.Inheritance.INHERITED : ActiveRule.Inheritance.OVERRIDES);
         }
@@ -153,17 +152,17 @@ public class RuleActivator {
   }
 
   private void updateProfileDates(DbSession dbSession, RuleActivatorContext context) {
-    QProfileDto profile = context.getProfile();
+    QProfileDto profile = context.getOrganizationProfile();
     if (profile != null) {
-      profile.setRulesUpdatedAtAsDate(context.getInitDate());
+      profile.setRulesUpdatedAtAsDate(context.getDate());
       if (userSession.isLoggedIn()) {
-        profile.setUserUpdatedAt(context.getInitDate().getTime());
+        profile.setUserUpdatedAt(context.getDate().getTime());
       }
       db.qualityProfileDao().update(dbSession, profile);
     } else {
       // built-in profile, change rules_profiles.rules_updated_at
       RulesProfileDto rulesProfile = context.getRulesProfile();
-      rulesProfile.setRulesUpdatedAtAsDate(context.getInitDate());
+      rulesProfile.setRulesUpdatedAtAsDate(context.getDate());
       db.qualityProfileDao().update(dbSession, rulesProfile);
     }
   }
@@ -180,50 +179,50 @@ public class RuleActivator {
     if (request.isReset()) {
       // load severity and params from parent profile, else from default values
       change.setSeverity(firstNonNull(
-        context.parentSeverity(), context.defaultSeverity()));
-      for (RuleParamDto ruleParamDto : context.ruleParams()) {
+        context.getActiveParentSeverity(), context.getRule().getSeverityString()));
+      for (RuleParamDto ruleParamDto : context.getRuleParams()) {
         String paramKey = ruleParamDto.getName();
         change.setParameter(paramKey, validateParam(ruleParamDto, firstNonNull(
-          context.parentParamValue(paramKey), context.defaultParamValue(paramKey))));
+          context.getActiveParentParamValue(paramKey), context.getDefaultParamValue(paramKey))));
       }
 
-    } else if (context.activeRule() != null) {
+    } else if (context.getActiveRule() != null) {
       // already activated -> load severity and parameters from request, else keep existing ones, else from parent,
       // else from default
       change.setSeverity(firstNonNull(
         request.getSeverity(),
-        context.currentSeverity(),
-        context.parentSeverity(),
-        context.defaultSeverity()));
-      for (RuleParamDto ruleParamDto : context.ruleParams()) {
+        context.getActiveSeverityBeforeChange(),
+        context.getActiveParentSeverity(),
+        context.getRule().getSeverityString()));
+      for (RuleParamDto ruleParamDto : context.getRuleParams()) {
         String paramKey = ruleParamDto.getName();
-        String paramValue = context.hasRequestParamValue(request, paramKey) ?
+        String paramValue = context.hasRequestedParamValue(request, paramKey) ?
         // If the request contains the parameter then we're using either value from request, or parent value, or default value
           firstNonNull(
-            context.requestParamValue(request, paramKey),
-            context.parentParamValue(paramKey),
-            context.defaultParamValue(paramKey))
+            context.getRequestedParamValue(request, paramKey),
+            context.getActiveParentParamValue(paramKey),
+            context.getDefaultParamValue(paramKey))
           // If the request doesn't contain the parameter, then we're using either value in DB, or parent value, or default value
           : firstNonNull(
-            context.currentParamValue(paramKey),
-            context.parentParamValue(paramKey),
-            context.defaultParamValue(paramKey));
+            context.getActiveParamValue(paramKey),
+            context.getActiveParentParamValue(paramKey),
+            context.getDefaultParamValue(paramKey));
         change.setParameter(paramKey, validateParam(ruleParamDto, paramValue));
       }
 
-    } else if (context.activeRule() == null) {
+    } else if (context.getActiveRule() == null) {
       // not activated -> load severity and parameters from request, else from parent, else from defaults
       change.setSeverity(firstNonNull(
         request.getSeverity(),
-        context.parentSeverity(),
-        context.defaultSeverity()));
-      for (RuleParamDto ruleParamDto : context.ruleParams()) {
+        context.getActiveParentSeverity(),
+        context.getRule().getSeverityString()));
+      for (RuleParamDto ruleParamDto : context.getRuleParams()) {
         String paramKey = ruleParamDto.getName();
         change.setParameter(paramKey, validateParam(ruleParamDto,
           firstNonNull(
-            context.requestParamValue(request, paramKey),
-            context.parentParamValue(paramKey),
-            context.defaultParamValue(paramKey))));
+            context.getRequestedParamValue(request, paramKey),
+            context.getActiveParentParamValue(paramKey),
+            context.getDefaultParamValue(paramKey))));
       }
     }
   }
@@ -250,8 +249,9 @@ public class RuleActivator {
   }
 
   protected List<QProfileDto> getChildren(DbSession session, RuleActivatorContext context) {
-    if (context.getProfile() != null) {
-      return db.qualityProfileDao().selectChildren(session, context.getProfile());
+    QProfileDto orgProfile = context.getOrganizationProfile();
+    if (orgProfile != null) {
+      return db.qualityProfileDao().selectChildren(session, orgProfile);
     }
     return db.qualityProfileDao().selectChildrenOfBuiltInRulesProfile(session, context.getRulesProfile());
   }
@@ -290,7 +290,7 @@ public class RuleActivator {
     dao.insert(dbSession, activeRule);
     for (Map.Entry<String, String> param : change.getParameters().entrySet()) {
       if (param.getValue() != null) {
-        ActiveRuleParamDto paramDto = ActiveRuleParamDto.createFor(context.ruleParamsByKeys().get(param.getKey()));
+        ActiveRuleParamDto paramDto = ActiveRuleParamDto.createFor(context.getRuleParam(param.getKey()));
         paramDto.setValue(param.getValue());
         dao.insertParam(dbSession, activeRule, paramDto);
       }
@@ -300,7 +300,7 @@ public class RuleActivator {
 
   private ActiveRuleDto doUpdate(ActiveRuleChange change, RuleActivatorContext context, DbSession dbSession) {
     ActiveRuleDao dao = db.activeRuleDao();
-    ActiveRuleDto activeRule = context.activeRule();
+    ActiveRuleDto activeRule = context.getActiveRule();
     if (activeRule != null) {
       String severity = change.getSeverity();
       if (severity != null) {
@@ -314,11 +314,11 @@ public class RuleActivator {
       dao.update(dbSession, activeRule);
 
       for (Map.Entry<String, String> param : change.getParameters().entrySet()) {
-        ActiveRuleParamDto activeRuleParamDto = context.activeRuleParamsAsMap().get(param.getKey());
+        ActiveRuleParamDto activeRuleParamDto = context.getActiveParamBeforeChange(param.getKey());
         if (activeRuleParamDto == null) {
           // did not exist
           if (param.getValue() != null) {
-            activeRuleParamDto = ActiveRuleParamDto.createFor(context.ruleParamsByKeys().get(param.getKey()));
+            activeRuleParamDto = ActiveRuleParamDto.createFor(context.getRuleParam(param.getKey()));
             activeRuleParamDto.setValue(param.getValue());
             dao.insertParam(dbSession, activeRule, activeRuleParamDto);
           }
@@ -386,7 +386,7 @@ public class RuleActivator {
   private List<ActiveRuleChange> cascadeDeactivation(DbSession dbSession, RuleActivatorContext context, RuleKey ruleKey, boolean force) {
     List<ActiveRuleChange> changes = new ArrayList<>();
     ActiveRuleChange change;
-    ActiveRuleDto activeRuleDto = context.activeRule();
+    ActiveRuleDto activeRuleDto = context.getActiveRule();
     if (activeRuleDto == null) {
       return changes;
     }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java
index 5b5f855ab83..c14bfb3d47a 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java
@@ -43,9 +43,9 @@ class RuleActivatorContext {
 
   private final QProfileDto profile;
   private final RulesProfileDto rulesProfile;
-  private final Date initDate = new Date();
+  private final Date now = new Date();
   private RuleDefinitionDto rule;
-  private final Map<String, RuleParamDto> ruleParams = new HashMap<>();
+  private final Map<String, RuleParamDto> ruleParamsByKey = new HashMap<>();
   private ActiveRuleDto activeRule;
   private ActiveRuleDto parentActiveRule;
   private final Map<String, ActiveRuleParamDto> activeRuleParams = new HashMap<>();
@@ -65,8 +65,11 @@ class RuleActivatorContext {
     this.isCascade = false;
   }
 
+  /**
+   * The organization profile that is being updated. Null if updating built-in profile.
+   */
   @CheckForNull
-  QProfileDto getProfile() {
+  QProfileDto getOrganizationProfile() {
     return profile;
   }
 
@@ -74,15 +77,15 @@ class RuleActivatorContext {
     return rulesProfile;
   }
 
-  boolean isBuiltIn() {
-    return profile == null;
-  }
-
+  /**
+   * Whether activation is being applied on a descendant profile ({@code true}) or
+   * on the target profile ({@code false}).
+   */
   boolean isCascade() {
     return isCascade;
   }
 
-  ActiveRuleKey activeRuleKey() {
+  ActiveRuleKey getActiveRuleKey() {
     return ActiveRuleKey.of(rulesProfile, rule.getKey());
   }
 
@@ -95,30 +98,35 @@ class RuleActivatorContext {
     return this;
   }
 
-
-
-  Date getInitDate() {
-    return initDate;
+  /**
+   * Date of request for activation.
+   */
+  Date getDate() {
+    return now;
   }
 
-  Map<String, RuleParamDto> ruleParamsByKeys() {
-    return ruleParams;
+  @CheckForNull
+  RuleParamDto getRuleParam(String key) {
+    return ruleParamsByKey.get(key);
   }
 
-  Collection<RuleParamDto> ruleParams() {
-    return ruleParams.values();
+  Collection<RuleParamDto> getRuleParams() {
+    return ruleParamsByKey.values();
   }
 
-  RuleActivatorContext setRuleParams(Collection<RuleParamDto> ruleParams) {
-    this.ruleParams.clear();
+  void setRuleParams(Collection<RuleParamDto> ruleParams) {
+    this.ruleParamsByKey.clear();
     for (RuleParamDto ruleParam : ruleParams) {
-      this.ruleParams.put(ruleParam.getName(), ruleParam);
+      this.ruleParamsByKey.put(ruleParam.getName(), ruleParam);
     }
-    return this;
   }
 
+  /**
+   * The active rule as it was before applying the current activation.
+   * Return null if the rule was not activated.
+   */
   @CheckForNull
-  ActiveRuleDto activeRule() {
+  ActiveRuleDto getActiveRule() {
     return activeRule;
   }
 
@@ -128,63 +136,59 @@ class RuleActivatorContext {
   }
 
   @CheckForNull
-  ActiveRuleDto parentActiveRule() {
+  ActiveRuleDto getParentActiveRule() {
     return parentActiveRule;
   }
 
-  RuleActivatorContext setParentActiveRule(@Nullable ActiveRuleDto a) {
+  void setParentActiveRule(@Nullable ActiveRuleDto a) {
     this.parentActiveRule = a;
-    return this;
   }
 
   @CheckForNull
-  String requestParamValue(RuleActivation request, String key) {
+  String getRequestedParamValue(RuleActivation request, String key) {
     if (rule.isCustomRule()) {
       return null;
     }
     return request.getParameter(key);
   }
 
-  boolean hasRequestParamValue(RuleActivation request, String key) {
+  boolean hasRequestedParamValue(RuleActivation request, String key) {
     return request.hasParameter(key);
   }
 
   @CheckForNull
-  String currentParamValue(String key) {
+  String getActiveParamValue(String key) {
     ActiveRuleParamDto param = activeRuleParams.get(key);
     return param != null ? param.getValue() : null;
   }
 
   @CheckForNull
-  String parentParamValue(String key) {
+  ActiveRuleParamDto getActiveParamBeforeChange(String key) {
+    return activeRuleParams.get(key);
+  }
+
+  @CheckForNull
+  String getActiveParentParamValue(String key) {
     ActiveRuleParamDto param = parentActiveRuleParams.get(key);
     return param != null ? param.getValue() : null;
   }
 
   @CheckForNull
-  String defaultParamValue(String key) {
-    RuleParamDto param = ruleParams.get(key);
+  String getDefaultParamValue(String key) {
+    RuleParamDto param = ruleParamsByKey.get(key);
     return param != null ? param.getDefaultValue() : null;
   }
 
   @CheckForNull
-  String currentSeverity() {
+  String getActiveSeverityBeforeChange() {
     return activeRule != null ? activeRule.getSeverityString() : null;
   }
 
   @CheckForNull
-  String parentSeverity() {
+  String getActiveParentSeverity() {
     return parentActiveRule != null ? parentActiveRule.getSeverityString() : null;
   }
 
-  @CheckForNull
-  String defaultSeverity() {
-    return rule.getSeverityString();
-  }
-
-  Map<String, ActiveRuleParamDto> activeRuleParamsAsMap() {
-    return activeRuleParams;
-  }
 
   Map<String, String> activeRuleParamsAsStringMap() {
     Map<String, String> params = Maps.newHashMap();
@@ -194,24 +198,22 @@ class RuleActivatorContext {
     return params;
   }
 
-  RuleActivatorContext setActiveRuleParams(@Nullable Collection<ActiveRuleParamDto> a) {
+  void setActiveRuleParams(@Nullable Collection<ActiveRuleParamDto> a) {
     activeRuleParams.clear();
     if (a != null) {
       for (ActiveRuleParamDto ar : a) {
         this.activeRuleParams.put(ar.getKey(), ar);
       }
     }
-    return this;
   }
 
-  RuleActivatorContext setParentActiveRuleParams(@Nullable Collection<ActiveRuleParamDto> a) {
+  void setParentActiveRuleParams(@Nullable Collection<ActiveRuleParamDto> a) {
     parentActiveRuleParams.clear();
     if (a != null) {
       for (ActiveRuleParamDto ar : a) {
         this.parentActiveRuleParams.put(ar.getKey(), ar);
       }
     }
-    return this;
   }
 
   /**
@@ -225,7 +227,7 @@ class RuleActivatorContext {
       return false;
     }
     for (Map.Entry<String, String> entry : change.getParameters().entrySet()) {
-      if (entry.getValue() != null && !entry.getValue().equals(parentParamValue(entry.getKey()))) {
+      if (entry.getValue() != null && !entry.getValue().equals(getActiveParentParamValue(entry.getKey()))) {
         return false;
       }
     }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java
index a96205609f8..044a9e0f432 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java
@@ -59,8 +59,9 @@ public class RuleActivatorContextFactory {
     initRule(ruleKey, context, dbSession);
     initActiveRules(context.getRulesProfile(), ruleKey, context, dbSession, false);
 
-    if (context.getProfile() != null && context.getProfile().getParentKee() != null) {
-      QProfileDto parent = db.qualityProfileDao().selectByUuid(dbSession, context.getProfile().getParentKee());
+    QProfileDto orgProfile = context.getOrganizationProfile();
+    if (orgProfile != null && orgProfile.getParentKee() != null) {
+      QProfileDto parent = db.qualityProfileDao().selectByUuid(dbSession, orgProfile.getParentKee());
       if (parent != null) {
         initActiveRules(RulesProfileDto.from(parent), ruleKey, context, dbSession, true);
       }