SONAR-10473 Keep severity up-to-date on built-in QP

zhuomingliang · Mar 14, 2018 · 4df6727 · 4df6727
1 parent b7497de
commit 4df6727
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 45 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
@@ -158,70 +158,64 @@ private void updateProfileDates(DbSession dbSession, RuleActivationContext conte
   }
 
   /**
-   * Severity and parameter values are :
-   * 1. defined by end-user
-   * 2. else inherited from parent profile
-   * 3. else defined by rule defaults
-   * <p/>
-   * On custom rules, it's always rule parameters that are used
+   * Update severity and params
    */
   private void applySeverityAndParamToChange(RuleActivation request, RuleActivationContext context, ActiveRuleChange change) {
     RuleWrapper rule = context.getRule();
     ActiveRuleWrapper activeRule = context.getActiveRule();
     ActiveRuleWrapper parentActiveRule = context.getParentActiveRule();
 
+    // First apply severity
+    String severity;
     if (request.isReset()) {
-      // load severity and params from parent profile, else from default values
-      change.setSeverity(firstNonNull(
+      // load severity from parent profile, else from default values
+      severity = firstNonNull(
         parentActiveRule != null ? parentActiveRule.get().getSeverityString() : null,
-        rule.get().getSeverityString()));
-
-      for (RuleParamDto ruleParamDto : rule.getParams()) {
-        String paramKey = ruleParamDto.getName();
-        change.setParameter(paramKey, validateParam(ruleParamDto, firstNonNull(
-          parentActiveRule != null ? parentActiveRule.getParamValue(paramKey) : null,
-          rule.getParamDefaultValue(paramKey))));
-      }
-
-    } else if (activeRule != null) {
-      // already activated -> load severity and parameters from request, else keep existing ones, else from parent,
-      // else from default
-      change.setSeverity(firstNonNull(
+        rule.get().getSeverityString());
+    } else if (context.getRulesProfile().isBuiltIn()) {
+      // for builtin quality profiles, the severity from profile, when null use the default severity of the rule
+      severity = firstNonNull(request.getSeverity(), rule.get().getSeverityString());
+    } else {
+      // load severity from request, else keep existing one (if already activated), else from parent, else from default
+      severity = firstNonNull(
         request.getSeverity(),
-        activeRule.get().getSeverityString(),
+        activeRule == null ? null : activeRule.get().getSeverityString(),
         parentActiveRule != null ? parentActiveRule.get().getSeverityString() : null,
-        rule.get().getSeverityString()));
-      for (RuleParamDto ruleParamDto : rule.getParams()) {
-        String paramKey = ruleParamDto.getName();
+        rule.get().getSeverityString());
+    }
+    change.setSeverity(severity);
+
+    // Apply param values
+    for (RuleParamDto ruleParamDto : rule.getParams()) {
+      String paramKey = ruleParamDto.getName();
+      String paramValue;
+      if (request.isReset()) {
+        // load params from parent profile, else from default values
+        paramValue = firstNonNull(
+          parentActiveRule != null ? parentActiveRule.getParamValue(paramKey) : null,
+          rule.getParamDefaultValue(paramKey));
+      } else if (context.getRulesProfile().isBuiltIn()) {
+        // use the value defined in the profile definition, else the rule default value
+        paramValue = firstNonNull(
+          context.getRequestedParamValue(request, paramKey),
+          rule.getParamDefaultValue(paramKey));
+      } else {
         String parentValue = parentActiveRule != null ? parentActiveRule.getParamValue(paramKey) : null;
-        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
+        String activeRuleValue = activeRule == null ? null : activeRule.getParamValue(paramKey);
+        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.getRequestedParamValue(request, paramKey),
             parentValue,
             rule.getParamDefaultValue(paramKey))
           // If the request doesn't contain the parameter, then we're using either value in DB, or parent value, or default value
           : firstNonNull(
-            activeRule.getParamValue(paramKey),
-            parentValue,
-            rule.getParamDefaultValue(paramKey));
-        change.setParameter(paramKey, validateParam(ruleParamDto, paramValue));
+          activeRuleValue,
+          parentValue,
+          rule.getParamDefaultValue(paramKey));
       }
 
-    } else {
-      // not activated -> load severity and parameters from request, else from parent, else from defaults
-      change.setSeverity(firstNonNull(
-        request.getSeverity(),
-        parentActiveRule != null ? parentActiveRule.get().getSeverityString() : null,
-        rule.get().getSeverityString()));
-      for (RuleParamDto ruleParamDto : rule.getParams()) {
-        String paramKey = ruleParamDto.getName();
-        change.setParameter(paramKey, validateParam(ruleParamDto,
-          firstNonNull(
-            context.getRequestedParamValue(request, paramKey),
-            parentActiveRule != null ? parentActiveRule.getParamValue(paramKey) : null,
-            rule.getParamDefaultValue(paramKey))));
-      }
+      change.setParameter(paramKey, validateParam(ruleParamDto, paramValue));
     }
   }
 

diff --git a/...r-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java b/...r-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQProfileUpdateImplTest.java
@@ -21,6 +21,7 @@
 
 import java.util.List;
 import java.util.Optional;
+import org.assertj.core.groups.Tuple;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -32,8 +33,10 @@
 import org.sonar.api.utils.internal.TestSystem2;
 import org.sonar.db.DbTester;
 import org.sonar.db.qualityprofile.ActiveRuleDto;
+import org.sonar.db.qualityprofile.ActiveRuleParamDto;
 import org.sonar.db.qualityprofile.RulesProfileDto;
 import org.sonar.db.rule.RuleDefinitionDto;
+import org.sonar.db.rule.RuleParamDto;
 import org.sonar.server.qualityprofile.index.ActiveRuleIndexer;
 import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.util.IntegerTypeValidation;
@@ -42,10 +45,12 @@
 
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.groups.Tuple.tuple;
 import static org.mockito.Mockito.mock;
 import static org.sonar.api.rules.RulePriority.BLOCKER;
 import static org.sonar.api.rules.RulePriority.CRITICAL;
 import static org.sonar.api.rules.RulePriority.MAJOR;
+import static org.sonar.api.rules.RulePriority.MINOR;
 import static org.sonar.db.qualityprofile.QualityProfileTesting.newRuleProfileDto;
 
 public class BuiltInQProfileUpdateImplTest {
@@ -187,6 +192,56 @@ public void activate_deactivate_and_update_three_rules_at_the_same_time() {
     assertThatProfileIsMarkedAsUpdated(persistedProfile);
   }
 
+  // SONAR-10473
+  @Test
+  public void activate_rule_on_built_in_profile_resets_severity_to_default_if_not_overridden() {
+    RuleDefinitionDto rule = db.rules().insert(r -> r.setSeverity(Severity.MAJOR).setLanguage("xoo"));
+
+    BuiltInQualityProfilesDefinition.Context context = new BuiltInQualityProfilesDefinition.Context();
+    NewBuiltInQualityProfile newQp = context.createBuiltInQualityProfile("Sonar way", "xoo");
+    newQp.activateRule(rule.getRepositoryKey(), rule.getRuleKey());
+    newQp.done();
+    BuiltInQProfile builtIn = builtInProfileRepository.create(context.profile("xoo", "Sonar way"), rule);
+    underTest.update(db.getSession(), builtIn, persistedProfile);
+
+    List<ActiveRuleDto> activeRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), persistedProfile);
+    assertThatRuleIsNewlyActivated(activeRules, rule, MAJOR);
+
+    // emulate an upgrade of analyzer that changes the default severity of the rule
+    rule.setSeverity(Severity.MINOR);
+    db.rules().update(rule);
+
+    underTest.update(db.getSession(), builtIn, persistedProfile);
+    activeRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), persistedProfile);
+    assertThatRuleIsNewlyActivated(activeRules, rule, MINOR);
+  }
+
+  @Test
+  public void activate_rule_on_built_in_profile_resets_params_to_default_if_not_overridden() {
+    RuleDefinitionDto rule = db.rules().insert(r -> r.setLanguage("xoo"));
+    RuleParamDto ruleParam = db.rules().insertRuleParam(rule, p -> p.setName("min").setDefaultValue("10"));
+
+    BuiltInQualityProfilesDefinition.Context context = new BuiltInQualityProfilesDefinition.Context();
+    NewBuiltInQualityProfile newQp = context.createBuiltInQualityProfile("Sonar way", "xoo");
+    newQp.activateRule(rule.getRepositoryKey(), rule.getRuleKey());
+    newQp.done();
+    BuiltInQProfile builtIn = builtInProfileRepository.create(context.profile("xoo", "Sonar way"), rule);
+    underTest.update(db.getSession(), builtIn, persistedProfile);
+
+    List<ActiveRuleDto> activeRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), persistedProfile);
+    assertThat(activeRules).hasSize(1);
+    assertThatRuleHasParams(db, activeRules.get(0), tuple("min", "10"));
+
+    // emulate an upgrade of analyzer that changes the default value of parameter min
+    ruleParam.setDefaultValue("20");
+    db.getDbClient().ruleDao().updateRuleParam(db.getSession(), rule, ruleParam);
+
+    underTest.update(db.getSession(), builtIn, persistedProfile);
+    activeRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), persistedProfile);
+    assertThat(activeRules).hasSize(1);
+    assertThatRuleHasParams(db, activeRules.get(0), tuple("min", "20"));
+  }
+
 //  @Test
 //  public void propagate_new_activation_to_profiles() {
 //    RuleDefinitionDto rule = createJavaRule();
@@ -229,6 +284,12 @@ public void activate_deactivate_and_update_three_rules_at_the_same_time() {
 //    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)
+      .containsExactlyInAnyOrder(expectedParams);
+  }
+
   private static void assertThatRuleIsNewlyActivated(List<ActiveRuleDto> activeRules, RuleDefinitionDto rule, RulePriority severity) {
     ActiveRuleDto activeRule = findRule(activeRules, rule).get();