SONAR-23184 Fix deactivated rules in a Quality Profile are activated after an upgrade

5 files changed, 144 insertions, 9 deletions

diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImplIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImplIT.java
index 37ebd8e5cc7..3dc6c004fd8 100644
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImplIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImplIT.java
@@ -309,6 +309,40 @@ class BuiltInQProfileUpdateImplIT {
     verify(qualityProfileChangeEventService).distributeRuleChangeEvent(any(), eq(changes), eq(persistedProfile.getLanguage()));
   }
 
+  @Test
+  void deactivated_rule_should_not_be_activated_in_descendant_profiles() {
+    RuleDto rule1 = db.rules().insert(r -> r.setLanguage("xoo"));
+    RuleDto rule2 = db.rules().insert(r -> r.setLanguage("xoo"));
+
+    QProfileDto parentProfile = db.qualityProfiles().insert(p -> p.setLanguage("xoo").setIsBuiltIn(true));
+    activateRuleInDb(RulesProfileDto.from(parentProfile), rule1, RulePriority.valueOf(Severity.MINOR),
+      Map.of(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.LOW));
+    activateRuleInDb(RulesProfileDto.from(parentProfile), rule2, RulePriority.valueOf(Severity.MINOR),
+      Map.of(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.LOW));
+
+    // child profile has only rule1 activated, to simulate rule2 is deactivated
+    QProfileDto childProfile = createChildProfile(parentProfile);
+    activateRuleInDb(RulesProfileDto.from(childProfile), rule1, RulePriority.valueOf(Severity.MINOR),
+      Map.of(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.LOW), INHERITED);
+
+    BuiltInQualityProfilesDefinition.Context context = new BuiltInQualityProfilesDefinition.Context();
+    NewBuiltInQualityProfile newQp = context.createBuiltInQualityProfile(parentProfile.getName(), parentProfile.getLanguage());
+    newQp.activateRule(rule1.getRepositoryKey(), rule1.getRuleKey());
+    newQp.activateRule(rule2.getRepositoryKey(), rule2.getRuleKey());
+    newQp.done();
+    BuiltInQProfile builtIn = builtInProfileRepository.create(context.profile(parentProfile.getLanguage(), parentProfile.getName()), rule1, rule2);
+    List<ActiveRuleChange> changes = underTest.update(db.getSession(), builtIn, RulesProfileDto.from(parentProfile));
+
+    assertThat(changes).hasSize(3);
+
+    List<ActiveRuleDto> parentActiveRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), RulesProfileDto.from(parentProfile));
+    assertThatRuleIsUpdated(parentActiveRules, rule1, RulePriority.valueOfInt(rule1.getSeverity()), rule1.getDefaultImpactsMap());
+    assertThatRuleIsUpdated(parentActiveRules, rule2, RulePriority.valueOfInt(rule2.getSeverity()), rule2.getDefaultImpactsMap());
+
+    List<ActiveRuleDto> childActiveRules = db.getDbClient().activeRuleDao().selectByRuleProfile(db.getSession(), RulesProfileDto.from(childProfile));
+    assertThatRuleIsUpdated(childActiveRules, rule1, RulePriority.valueOfInt(rule1.getSeverity()), rule1.getDefaultImpactsMap(), INHERITED);
+  }
+
   // SONAR-14559
   @Test
   void propagate_rule_update_to_descendant_active_rule() {
diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/RuleActivatorIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/RuleActivatorIT.java
index 7f730090ed2..363005b4f4e 100644
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/RuleActivatorIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/qualityprofile/builtin/RuleActivatorIT.java
@@ -21,6 +21,7 @@ package org.sonar.server.qualityprofile.builtin;
 
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nullable;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -65,6 +66,7 @@ import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static java.util.Map.of;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.tuple;
 import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.mock;
 import static org.sonar.api.issue.impact.SoftwareQuality.MAINTAINABILITY;
@@ -360,6 +362,83 @@ class RuleActivatorIT {
       () -> underTest.activate(session, resetRequest, context));
   }
 
+  @Test
+  void testActivate_whenProfileIsNotBuiltInAndRuleUuidInPreviousBuiltinActiveRuleUuids_shouldContainNoChange() {
+    QProfileDto builtinProfile = db.qualityProfiles().insert(p -> p.setIsBuiltIn(true).setLanguage("xoo"));
+    QProfileDto customProfile = createChildProfile(builtinProfile);
+
+    RuleDto rule = db.rules().insert(r -> r.setLanguage("xoo").setSeverity(Severity.CRITICAL));
+    ActiveRuleDto builtinActiveRuleDto = activateRuleInDb(RulesProfileDto.from(builtinProfile), rule,
+      RulePriority.valueOf(Severity.CRITICAL), Map.of(MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH), null, null);
+
+    RuleActivationContext context = new RuleActivationContext.Builder()
+      .setProfiles(List.of(builtinProfile, customProfile))
+      .setBaseProfile(RulesProfileDto.from(builtinProfile))
+      .setPreviousBuiltinActiveRuleUuids(Set.of(rule.getUuid()))
+      .setDescendantProfilesSupplier((profiles, ruleUuids) -> new Result(emptyList(), emptyList(), emptyList()))
+      .setRules(singletonList(rule))
+      .setRuleParams(emptyList())
+      .setActiveRules(List.of(builtinActiveRuleDto))
+      .setActiveRuleParams(emptyList())
+      .build();
+
+    RuleActivation activation = RuleActivation.create(rule.getUuid());
+
+    List<ActiveRuleChange> result = underTest.activate(db.getSession(), activation, context);
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  void testActivate_whenProfileIsNotBuiltInAndRuleUuidNotInPreviousBuiltinActiveRuleUuids_shouldContainActivation() {
+    QProfileDto builtinProfile = db.qualityProfiles().insert(p -> p.setIsBuiltIn(true).setLanguage("xoo"));
+    QProfileDto customProfile = createChildProfile(builtinProfile);
+
+    RuleDto rule = db.rules().insert(r -> r.setLanguage("xoo").setSeverity(Severity.CRITICAL));
+    ActiveRuleDto builtinActiveRuleDto = activateRuleInDb(RulesProfileDto.from(builtinProfile), rule,
+      RulePriority.valueOf(Severity.CRITICAL), Map.of(MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH), null, null);
+
+    RuleActivationContext context = new RuleActivationContext.Builder()
+      .setProfiles(List.of(builtinProfile, customProfile))
+      .setBaseProfile(RulesProfileDto.from(builtinProfile))
+      .setPreviousBuiltinActiveRuleUuids(Set.of("another-rule-uuid"))
+      .setDescendantProfilesSupplier((profiles, ruleUuids) -> new Result(emptyList(), emptyList(), emptyList()))
+      .setRules(singletonList(rule))
+      .setRuleParams(emptyList())
+      .setActiveRules(List.of(builtinActiveRuleDto))
+      .setActiveRuleParams(emptyList())
+      .build();
+
+    RuleActivation activation = RuleActivation.create(rule.getUuid());
+
+    List<ActiveRuleChange> result = underTest.activate(db.getSession(), activation, context);
+    assertThat(result)
+      .hasSize(1)
+      .extracting(ActiveRuleChange::getRuleUuid, ActiveRuleChange::getKey)
+      .containsExactlyInAnyOrder(tuple(rule.getUuid(), ActiveRuleKey.of(customProfile, RuleKey.of(rule.getRepositoryKey(), rule.getRuleKey()))));
+  }
+
+  @Test
+  void testActivate_whenProfileIsBuiltIn_shouldContainActivation() {
+    QProfileDto profile = db.qualityProfiles().insert(p -> p.setIsBuiltIn(true).setLanguage("xoo"));
+    RuleDto rule = db.rules().insert(r -> r.setLanguage("xoo"));
+    RuleActivationContext context = new RuleActivationContext.Builder()
+      .setProfiles(List.of(profile))
+      .setBaseProfile(RulesProfileDto.from(profile))
+      .setPreviousBuiltinActiveRuleUuids(Set.of(rule.getUuid()))
+      .setDescendantProfilesSupplier((profiles, ruleUuids) -> new Result(emptyList(), emptyList(), emptyList()))
+      .setRules(singletonList(rule))
+      .setRuleParams(emptyList())
+      .setActiveRules(emptyList())
+      .setActiveRuleParams(emptyList())
+      .build();
+
+    RuleActivation activation = RuleActivation.create(rule.getUuid());
+
+    List<ActiveRuleChange> result = underTest.activate(db.getSession(), activation, context);
+    assertThat(result).hasSize(1);
+    assertThat(result.get(0).getRuleUuid()).isEqualTo(rule.getUuid());
+  }
+
   private ActiveRuleDto activateRuleInDb(RulesProfileDto ruleProfile, RuleDto rule, RulePriority severity, Map<SoftwareQuality, org.sonar.api.issue.impact.Severity> impacts,
     @Nullable Boolean prioritizedRule, @Nullable ActiveRuleInheritance inheritance) {
     ActiveRuleDto dto = new ActiveRuleDto()
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImpl.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImpl.java
index de49848fa6e..9499c538626 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImpl.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/BuiltInQProfileUpdateImpl.java
@@ -21,6 +21,7 @@ package org.sonar.server.qualityprofile.builtin;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -53,10 +54,12 @@ public class BuiltInQProfileUpdateImpl implements BuiltInQProfileUpdate {
 
   public List<ActiveRuleChange> update(DbSession dbSession, BuiltInQProfile builtInDefinition, RulesProfileDto initialRuleProfile) {
     // Keep reference to all the activated rules before update
-    Set<String> deactivatedRuleUuids = dbClient.activeRuleDao().selectByRuleProfile(dbSession, initialRuleProfile)
+    Set<String> previousBuiltinActiveRuleUuids = dbClient.activeRuleDao().selectByRuleProfile(dbSession, initialRuleProfile)
       .stream()
       .map(ActiveRuleDto::getRuleUuid)
-      .collect(Collectors.toSet());
+      .collect(Collectors.toUnmodifiableSet());
+
+    Set<String> deactivatedRuleUuids = new HashSet<>(previousBuiltinActiveRuleUuids);
 
     // all rules, including those which are removed from built-in profile
     Set<String> ruleUuids = Stream.concat(
@@ -71,7 +74,7 @@ public class BuiltInQProfileUpdateImpl implements BuiltInQProfileUpdate {
       deactivatedRuleUuids.remove(activation.getRuleUuid());
     }
 
-    RuleActivationContext context = ruleActivator.createContextForBuiltInProfile(dbSession, initialRuleProfile, ruleUuids);
+    RuleActivationContext context = ruleActivator.createContextForBuiltInProfile(dbSession, initialRuleProfile, ruleUuids, previousBuiltinActiveRuleUuids);
     List<ActiveRuleChange> changes = new ArrayList<>();
 
     changes.addAll(ruleActivator.activate(dbSession, activations, context));
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivationContext.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivationContext.java
index a2e7d50dfcf..c9f7b4b5a1f 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivationContext.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivationContext.java
@@ -25,6 +25,7 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import javax.annotation.CheckForNull;
@@ -40,6 +41,7 @@ import org.sonar.server.qualityprofile.RuleActivation;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
+import static java.util.Collections.emptySet;
 import static java.util.Objects.requireNonNull;
 import static org.sonar.core.util.stream.MoreCollectors.index;
 import static org.sonar.server.exceptions.BadRequestException.checkRequest;
@@ -54,6 +56,8 @@ public class RuleActivationContext {
 
   private final long date;
 
+  private Set<String> previousBuiltinActiveRuleUuids;
+
   // The profile that is initially targeted by the operation
   private final RulesProfileDto baseRulesProfile;
 
@@ -79,6 +83,8 @@ public class RuleActivationContext {
 
   private RuleActivationContext(Builder builder) {
     this.date = builder.date;
+    this.previousBuiltinActiveRuleUuids = builder.previousBuiltinActiveRuleUuids == null ? emptySet() :
+      builder.previousBuiltinActiveRuleUuids;
     this.descendantProfilesSupplier = builder.descendantProfilesSupplier;
 
     ListMultimap<String, RuleParamDto> paramsByRuleId = builder.ruleParams.stream().collect(index(RuleParamDto::getRuleUuid));
@@ -243,8 +249,13 @@ public class RuleActivationContext {
       .orElse(null);
   }
 
+  public Set<String> getPreviousBuiltinActiveRuleUuids() {
+    return previousBuiltinActiveRuleUuids;
+  }
+
   static final class Builder {
     private long date = System.currentTimeMillis();
+    private Set<String> previousBuiltinActiveRuleUuids;
     private RulesProfileDto baseRulesProfile;
     private Collection<RuleDto> rules;
     private Collection<RuleParamDto> ruleParams;
@@ -258,6 +269,11 @@ public class RuleActivationContext {
       return this;
     }
 
+    Builder setPreviousBuiltinActiveRuleUuids(Set<String> previousBuiltinActiveRuleUuids) {
+      this.previousBuiltinActiveRuleUuids = previousBuiltinActiveRuleUuids;
+      return this;
+    }
+
     Builder setBaseProfile(RulesProfileDto p) {
       this.baseRulesProfile = p;
       return this;
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivator.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivator.java
index 66bdcfb6c48..ee037c60776 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivator.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/qualityprofile/builtin/RuleActivator.java
@@ -116,8 +116,7 @@ public class RuleActivator {
     ActiveRuleWrapper activeRule = context.getActiveRule();
     ActiveRuleKey activeRuleKey = ActiveRuleKey.of(context.getRulesProfile(), rule.getKey());
     if (activeRule == null) {
-      if (activation.isReset()) {
-        // ignore reset when rule is not activated
+      if (activation.isReset() || skipBuiltinRuleActivation(activation, context)) {
         return changes;
       }
       change = handleNewRuleActivation(activation, context, rule, activeRuleKey);
@@ -142,9 +141,6 @@ public class RuleActivator {
     if (change != null) {
       changes.add(change);
       persist(change, context, dbSession);
-    }
-
-    if (!changes.isEmpty()) {
       updateProfileDates(dbSession, context);
     }
 
@@ -157,6 +153,12 @@ public class RuleActivator {
     return changes;
   }
 
+  private static boolean skipBuiltinRuleActivation(RuleActivation activation, RuleActivationContext context) {
+    // SONAR-23184: If the rule isn't active for a child profile and it is active for the previous parent definition of the profile,
+    // it means it was deactivated on purpose by a user - we don't want to active it.
+    return context.isCascading() && context.getPreviousBuiltinActiveRuleUuids().contains(activation.getRuleUuid());
+  }
+
   private void handleUpdatedRuleActivation(RuleActivation activation, RuleActivationContext context, ActiveRuleChange change,
     ActiveRuleWrapper activeRule) {
     if (context.isCascading() && activeRule.get().getInheritance() == null) {
@@ -515,11 +517,12 @@ public class RuleActivator {
   }
 
   public RuleActivationContext createContextForBuiltInProfile(DbSession dbSession, RulesProfileDto builtInProfile,
-    Collection<String> ruleUuids) {
+    Collection<String> ruleUuids, Set<String> previousBuiltinActiveRuleUuids) {
     checkArgument(builtInProfile.isBuiltIn(), "Rules profile with UUID %s is not built-in", builtInProfile.getUuid());
 
     RuleActivationContext.Builder builder = new RuleActivationContext.Builder();
     builder.setDescendantProfilesSupplier(createDescendantProfilesSupplier(dbSession));
+    builder.setPreviousBuiltinActiveRuleUuids(previousBuiltinActiveRuleUuids);
 
     // load rules
     completeWithRules(dbSession, builder, ruleUuids);