From: Simon Brandhof Date: Mon, 15 Jan 2018 14:07:06 +0000 (+0100) Subject: SONAR-10052 minor refactoring to better understand rule activation X-Git-Tag: 7.5~1759 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=774805335b51285df0f1d6308e3e52bb0f3d750a;p=sonarqube.git SONAR-10052 minor refactoring to better understand rule activation --- 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 update(DbSession dbSession, BuiltInQProfile builtIn, RulesProfileDto ruleProfile) { - LOGGER.info("Update profile {}", builtIn.getQProfileName()); + private List 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 activateAndCommit(DbSession dbSession, RuleActivation activation, QProfileDto profile) { + public void activateAndCommit(DbSession dbSession, RuleActivation activation, QProfileDto profile) { List changes = activate(dbSession, activation, profile); activeRuleIndexer.commitAndIndex(dbSession, changes); - return changes; } public List 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 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 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 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 cascadeDeactivation(DbSession dbSession, RuleActivatorContext context, RuleKey ruleKey, boolean force) { List 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 ruleParams = new HashMap<>(); + private final Map ruleParamsByKey = new HashMap<>(); private ActiveRuleDto activeRule; private ActiveRuleDto parentActiveRule; private final Map 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 ruleParamsByKeys() { - return ruleParams; + @CheckForNull + RuleParamDto getRuleParam(String key) { + return ruleParamsByKey.get(key); } - Collection ruleParams() { - return ruleParams.values(); + Collection getRuleParams() { + return ruleParamsByKey.values(); } - RuleActivatorContext setRuleParams(Collection ruleParams) { - this.ruleParams.clear(); + void setRuleParams(Collection 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 activeRuleParamsAsMap() { - return activeRuleParams; - } Map activeRuleParamsAsStringMap() { Map params = Maps.newHashMap(); @@ -194,24 +198,22 @@ class RuleActivatorContext { return params; } - RuleActivatorContext setActiveRuleParams(@Nullable Collection a) { + void setActiveRuleParams(@Nullable Collection a) { activeRuleParams.clear(); if (a != null) { for (ActiveRuleParamDto ar : a) { this.activeRuleParams.put(ar.getKey(), ar); } } - return this; } - RuleActivatorContext setParentActiveRuleParams(@Nullable Collection a) { + void setParentActiveRuleParams(@Nullable Collection 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 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); }