From ea57408183fd266e9f35bf7b608ae1e89b5c799b Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 6 Jun 2014 14:03:19 +0200 Subject: [PATCH] SONAR-5362 Update RulUpdater to be able to update custom rule --- .../java/org/sonar/server/rule/NewRule.java | 25 ++-- .../org/sonar/server/rule/RegisterRules.java | 2 +- .../org/sonar/server/rule/RuleCreator.java | 2 +- .../org/sonar/server/rule/RuleUpdate.java | 113 +++++++++++++++++- .../org/sonar/server/rule/RuleUpdater.java | 74 ++++++++++++ .../sonar/server/rule/ws/CreateAction.java | 2 +- .../sonar/server/issue/ws/example-search.json | 1 + .../sonar/server/issue/ws/example-show.json | 2 +- .../server/rule/RuleCreatorMediumTest.java | 22 ++-- .../server/rule/RuleServiceMediumTest.java | 2 +- .../server/rule/RuleUpdaterMediumTest.java | 46 ++++++- .../server/rule/ws/CreateActionTest.java | 4 +- 12 files changed, 257 insertions(+), 38 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/rule/NewRule.java b/sonar-server/src/main/java/org/sonar/server/rule/NewRule.java index 86be3dcdc55..c5f11cb43a0 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/NewRule.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/NewRule.java @@ -30,19 +30,10 @@ import java.util.Map; public class NewRule { - private boolean isTemplate; private RuleKey templateKey; private String name, htmlDescription, severity; private RuleStatus status; - private final Map params = Maps.newHashMap(); - - public boolean isTemplate() { - return isTemplate; - } - - public void setIsTemplate(boolean template) { - this.isTemplate = template; - } + private final Map parameters = Maps.newHashMap(); @CheckForNull public RuleKey templateKey() { @@ -97,18 +88,18 @@ public class NewRule { return this; } - public Map params() { - return params; + public Map parameters() { + return parameters; } @CheckForNull - public String param(final String paramKey) { - return params.get(paramKey); + public String parameter(final String paramKey) { + return parameters.get(paramKey); } - public NewRule setParams(Map params) { - this.params.clear(); - this.params.putAll(params); + public NewRule setParameters(Map params) { + this.parameters.clear(); + this.parameters.putAll(params); return this; } } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java index 937b431d11e..1cf13c34b69 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -49,6 +49,7 @@ import org.sonar.server.search.action.IndexAction; import org.sonar.server.search.action.KeyIndexAction; import javax.annotation.Nullable; + import java.util.*; import static com.google.common.collect.Lists.newArrayList; @@ -345,7 +346,6 @@ public class RegisterRules implements Startable { ruleDto.setDefaultRemediationCoefficient(parent.getDefaultRemediationCoefficient()); ruleDto.setDefaultRemediationOffset(parent.getDefaultRemediationOffset()); ruleDto.setEffortToFixDescription(parent.getEffortToFixDescription()); - // TODO update tags ? dbClient.ruleDao().update(session, ruleDto); toBeRemoved = false; } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java index f497c54a2ce..3f3d9d03761 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java @@ -99,7 +99,7 @@ public class RuleCreator implements ServerComponent { dbClient.ruleDao().insert(dbSession, ruleDto); for (RuleParamDto templateRuleParamDto : dbClient.ruleDao().findRuleParamsByRuleKey(dbSession, templateRuleDto.getKey())) { - String newRuleParam = newRule.param(templateRuleParamDto.getName()); + String newRuleParam = newRule.parameter(templateRuleParamDto.getName()); if (newRuleParam == null) { throw new IllegalArgumentException(String.format("The parameter '%s' has not been set", templateRuleParamDto.getName())); } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java index 479e55202b4..ad0eccdee74 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java @@ -19,12 +19,16 @@ */ package org.sonar.server.rule; +import com.google.common.collect.Maps; import org.apache.commons.lang.StringUtils; import org.sonar.api.rule.RuleKey; +import org.sonar.api.rule.RuleStatus; import org.sonar.api.server.debt.DebtRemediationFunction; import javax.annotation.CheckForNull; import javax.annotation.Nullable; + +import java.util.Map; import java.util.Set; public class RuleUpdate { @@ -33,12 +37,18 @@ public class RuleUpdate { private final RuleKey ruleKey; - private boolean changeTags = false, changeMarkdownNote = false, changeDebtSubCharacteristic = false, changeDebtRemediationFunction = false; + private boolean changeTags = false, changeMarkdownNote = false, changeDebtSubCharacteristic = false, changeDebtRemediationFunction = false, + changeName = false, changeDescription = false, changeSeverity = false, changeStatus = false, changeParameters = false; + private boolean isCustomRule = false; private Set tags; private String markdownNote; private String debtSubCharacteristicKey; private DebtRemediationFunction debtRemediationFunction; + private String name, htmlDescription, severity; + private RuleStatus status; + private final Map parameters = Maps.newHashMap(); + public RuleUpdate(RuleKey ruleKey) { this.ruleKey = ruleKey; } @@ -101,6 +111,67 @@ public class RuleUpdate { return this; } + public String getName() { + return name; + } + + public RuleUpdate setName(@Nullable String name) { + checkCustomRule(); + this.name = name; + this.changeName = true; + return this; + } + + public String getHtmlDescription() { + return htmlDescription; + } + + public RuleUpdate setHtmlDescription(@Nullable String htmlDescription) { + checkCustomRule(); + this.htmlDescription = htmlDescription; + this.changeDescription = true; + return this; + } + + public String getSeverity() { + return severity; + } + + public RuleUpdate setSeverity(String severity) { + checkCustomRule(); + this.severity = severity; + this.changeSeverity = true; + return this; + } + + public RuleStatus getStatus() { + return status; + } + + public RuleUpdate setStatus(RuleStatus status) { + checkCustomRule(); + this.status = status; + this.changeStatus = true; + return this; + } + + public RuleUpdate setParameters(Map params) { + checkCustomRule(); + this.parameters.clear(); + this.parameters.putAll(params); + this.changeParameters = true; + return this; + } + + public Map getParameters() { + return parameters; + } + + @CheckForNull + public String parameter(final String paramKey) { + return parameters.get(paramKey); + } + public boolean isChangeTags() { return changeTags; } @@ -117,7 +188,45 @@ public class RuleUpdate { return changeDebtRemediationFunction; } + public boolean isChangeName() { + return changeName; + } + + public boolean isChangeDescription() { + return changeDescription; + } + + public boolean isChangeSeverity() { + return changeSeverity; + } + + public boolean isChangeStatus() { + return changeStatus; + } + + public boolean isChangeParameters() { + return changeParameters; + } + public boolean isEmpty() { - return !changeMarkdownNote && !changeTags && !changeDebtSubCharacteristic && !changeDebtRemediationFunction; + return !changeMarkdownNote && !changeTags && !changeDebtSubCharacteristic && !changeDebtRemediationFunction && + !changeName && !changeDescription && !changeSeverity && !changeStatus && !changeParameters; + } + + private void checkCustomRule(){ + if (!isCustomRule) { + throw new IllegalStateException("Not a custom rule"); + } } + + public static RuleUpdate createForRule(RuleKey ruleKey) { + return new RuleUpdate(ruleKey); + } + + public static RuleUpdate createForCustomRule(RuleKey ruleKey) { + RuleUpdate ruleUpdate = new RuleUpdate(ruleKey); + ruleUpdate.isCustomRule = true; + return ruleUpdate; + } + } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java index 4f812fc22a9..27a393a4402 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java @@ -19,23 +19,29 @@ */ package org.sonar.server.rule; +import com.google.common.base.Strings; import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.builder.EqualsBuilder; import org.sonar.api.ServerComponent; import org.sonar.api.rule.RuleStatus; +import org.sonar.api.rule.Severity; import org.sonar.api.server.debt.DebtRemediationFunction; import org.sonar.api.utils.System2; import org.sonar.core.persistence.DbSession; import org.sonar.core.rule.RuleDto; +import org.sonar.core.rule.RuleParamDto; import org.sonar.core.technicaldebt.db.CharacteristicDto; import org.sonar.server.db.DbClient; import org.sonar.server.user.UserSession; import java.util.Collections; import java.util.Date; +import java.util.List; import java.util.Set; +import static com.google.common.collect.Lists.newArrayList; + public class RuleUpdater implements ServerComponent { private final DbClient dbClient; @@ -57,6 +63,9 @@ public class RuleUpdater implements ServerComponent { // validate only the changes, not all the rule fields apply(update, context, userSession); dbClient.ruleDao().update(dbSession, context.rule); + for (RuleParamDto ruleParamDto : context.parameters) { + dbClient.ruleDao().updateRuleParam(dbSession, context.rule, ruleParamDto); + } dbSession.commit(); return true; @@ -76,6 +85,7 @@ public class RuleUpdater implements ServerComponent { if (RuleStatus.REMOVED == context.rule.getStatus()) { throw new IllegalArgumentException("Rule with REMOVED status cannot be updated: " + change.getRuleKey()); } + context.parameters = dbClient.ruleDao().findRuleParamsByRuleKey(dbSession, change.getRuleKey()); String subCharacteristicKey = change.getDebtSubCharacteristicKey(); if (subCharacteristicKey != null && @@ -100,6 +110,21 @@ public class RuleUpdater implements ServerComponent { } private void apply(RuleUpdate update, Context context, UserSession userSession) { + if (update.isChangeName()) { + updateName(update, context); + } + if (update.isChangeDescription()) { + updateDescription(update, context); + } + if (update.isChangeSeverity()) { + updateSeverity(update, context); + } + if (update.isChangeStatus()) { + updateStatus(update, context); + } + if (update.isChangeParameters()) { + updateParameters(update, context); + } if (update.isChangeMarkdownNote()) { updateMarkdownNote(update, context, userSession); } @@ -115,6 +140,54 @@ public class RuleUpdater implements ServerComponent { } } + private void updateName(RuleUpdate update, Context context) { + String name = update.getName(); + if (Strings.isNullOrEmpty(name)) { + throw new IllegalArgumentException("The name is missing"); + } else { + context.rule.setName(name); + } + } + + private void updateDescription(RuleUpdate update, Context context) { + String description = update.getHtmlDescription(); + if (Strings.isNullOrEmpty(description)) { + throw new IllegalArgumentException("The description is missing"); + } else { + context.rule.setDescription(description); + } + } + + private void updateSeverity(RuleUpdate update, Context context) { + String severity = update.getSeverity(); + if (Strings.isNullOrEmpty(severity) || !Severity.ALL.contains(severity)) { + throw new IllegalArgumentException("The severity is invalid"); + } else { + context.rule.setSeverity(severity); + } + } + + private void updateStatus(RuleUpdate update, Context context) { + RuleStatus status = update.getStatus(); + if (status == null) { + throw new IllegalArgumentException("The status is missing"); + } else { + context.rule.setStatus(status); + } + } + + private void updateParameters(RuleUpdate update, Context context) { + // All parameters have to be updated + for (RuleParamDto ruleParamDto : context.parameters) { + String value = update.parameter(ruleParamDto.getName()); + if (Strings.isNullOrEmpty(value)) { + throw new IllegalArgumentException(String.format("The parameter '%s' has not been set", ruleParamDto.getName())); + } else { + ruleParamDto.setDefaultValue(value); + } + } + } + private void updateTags(RuleUpdate update, Context context) { Set tags = update.getTags(); if (tags == null || tags.isEmpty()) { @@ -206,6 +279,7 @@ public class RuleUpdater implements ServerComponent { */ private static class Context { private RuleDto rule; + private List parameters = newArrayList(); private CharacteristicDto newCharacteristic; } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java b/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java index 4041f136190..7dc0e090158 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java @@ -101,7 +101,7 @@ public class CreateAction implements RequestHandler { .setStatus(RuleStatus.valueOf(request.mandatoryParam(PARAM_STATUS))); String params = request.param(PARAMS); if (params != null) { - newRule.setParams(KeyValueFormat.parse(params)); + newRule.setParameters(KeyValueFormat.parse(params)); } service.create(newRule); } diff --git a/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-search.json b/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-search.json index df05a415c09..ff8974c9fc5 100644 --- a/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-search.json +++ b/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-search.json @@ -19,6 +19,7 @@ "message": "'3' is a magic number.", "line": 530, "author": "Developer 1", + "debt": "2h1min", "creationDate": "2013-05-13T17:55:39+0200", "updateDate": "2013-05-13T17:55:39+0200", "comments": [ diff --git a/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-show.json b/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-show.json index f0a4edc287c..4173dca0314 100644 --- a/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-show.json +++ b/sonar-server/src/main/resources/org/sonar/server/issue/ws/example-show.json @@ -8,7 +8,7 @@ "status": "OPEN", "severity": "MAJOR", "author": "Developer 1", - "debt": "2min", + "debt": "2 minutes", "creationDate": "2014-02-14T23:04:29+0100", "fCreationDate": "Feb 14, 2014 11:04 PM", "updateDate": "2014-03-04T23:03:44+0100", diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleCreatorMediumTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleCreatorMediumTest.java index ef23891cbdd..9be2392ec2e 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleCreatorMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleCreatorMediumTest.java @@ -76,7 +76,7 @@ public class RuleCreatorMediumTest { .setHtmlDescription("Some description") .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); RuleKey customRuleKey = creator.create(newRule); dbSession.clearCache(); @@ -99,6 +99,14 @@ public class RuleCreatorMediumTest { List params = db.ruleDao().findRuleParamsByRuleKey(dbSession, customRuleKey); assertThat(params).hasSize(1); + + RuleParamDto param = params.get(0); + // From template rule + assertThat(param.getName()).isEqualTo("regex"); + assertThat(param.getDescription()).isEqualTo("Reg ex"); + assertThat(param.getType()).isEqualTo("STRING"); + // From user + assertThat(param.getDefaultValue()).isEqualTo("a.*"); } @Test @@ -112,7 +120,7 @@ public class RuleCreatorMediumTest { .setHtmlDescription("Some description") .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); @@ -133,7 +141,7 @@ public class RuleCreatorMediumTest { .setName("My custom") .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); @@ -154,7 +162,7 @@ public class RuleCreatorMediumTest { .setName("My custom") .setHtmlDescription("Some description") .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); @@ -176,7 +184,7 @@ public class RuleCreatorMediumTest { .setHtmlDescription("Some description") .setSeverity("INVALID") .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); @@ -197,7 +205,7 @@ public class RuleCreatorMediumTest { .setName("My custom") .setHtmlDescription("Some description") .setSeverity(Severity.MAJOR) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); @@ -222,7 +230,7 @@ public class RuleCreatorMediumTest { .setHtmlDescription("Some description") .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); try { creator.create(newRule); diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java index 260e60d20f2..60ac8b0430a 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java @@ -150,7 +150,7 @@ public class RuleServiceMediumTest { .setHtmlDescription("Some description") .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) - .setParams(ImmutableMap.of("regex", "a.*")); + .setParameters(ImmutableMap.of("regex", "a.*")); RuleKey customRuleKey = service.create(newRule); dbSession.clearCache(); diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java index 70e65f64b54..d9516d28cd6 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java @@ -19,6 +19,7 @@ */ package org.sonar.server.rule; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import org.junit.After; @@ -29,9 +30,9 @@ import org.sonar.api.rule.RuleKey; import org.sonar.api.rule.RuleStatus; import org.sonar.api.server.debt.DebtRemediationFunction; import org.sonar.api.server.debt.internal.DefaultDebtRemediationFunction; -import org.sonar.api.utils.System2; import org.sonar.core.persistence.DbSession; import org.sonar.core.rule.RuleDto; +import org.sonar.core.rule.RuleParamDto; import org.sonar.core.technicaldebt.db.CharacteristicDto; import org.sonar.server.db.DbClient; import org.sonar.server.debt.DebtTesting; @@ -40,11 +41,11 @@ import org.sonar.server.tester.ServerTester; import org.sonar.server.user.MockUserSession; import org.sonar.server.user.UserSession; +import java.util.List; import java.util.Set; import static org.fest.assertions.Assertions.assertThat; import static org.fest.assertions.Fail.fail; -import static org.mockito.Mockito.mock; public class RuleUpdaterMediumTest { @@ -55,7 +56,6 @@ public class RuleUpdaterMediumTest { DbClient db = tester.get(DbClient.class); DbSession dbSession; - System2 system = mock(System2.class); RuleUpdater updater = tester.get(RuleUpdater.class); int reliabilityId, softReliabilityId, hardReliabilityId; @@ -168,8 +168,8 @@ public class RuleUpdaterMediumTest { public void set_tags() throws Exception { // insert db db.ruleDao().insert(dbSession, RuleTesting.newDto(RULE_KEY) - .setTags(Sets.newHashSet("security")) - .setSystemTags(Sets.newHashSet("java8", "javadoc"))); + .setTags(Sets.newHashSet("security")) + .setSystemTags(Sets.newHashSet("java8", "javadoc"))); dbSession.commit(); // java8 is a system tag -> ignore @@ -325,4 +325,40 @@ public class RuleUpdaterMediumTest { db.debtCharacteristicDao().insert(hardReliability, dbSession); hardReliabilityId = hardReliability.getId(); } + + @Test + public void update_custom_rule() throws Exception { + insertDebtCharacteristics(dbSession); + RuleDto ruledto = db.ruleDao().insert(dbSession, RuleTesting.newDto(RULE_KEY) + .setName("Old name") + .setDescription("Old description") + .setSeverity("MINOR") + .setStatus(RuleStatus.BETA) + ); + RuleParamDto ruleParamDto = RuleParamDto.createFor(ruledto).setName("regex").setType("STRING").setDescription("Reg ex").setDefaultValue(".*"); + db.ruleDao().addRuleParam(dbSession, ruledto, ruleParamDto); + + dbSession.commit(); + + RuleUpdate update = RuleUpdate.createForCustomRule(RULE_KEY) + .setName("New name") + .setHtmlDescription("New description") + .setSeverity("MAJOR") + .setStatus(RuleStatus.READY) + .setParameters(ImmutableMap.of("regex", "a.*")); + updater.update(update, UserSession.get()); + + // verify db + dbSession.clearCache(); + RuleDto rule = db.ruleDao().getNullableByKey(dbSession, RULE_KEY); + assertThat(rule.getName()).isEqualTo("New name"); + assertThat(rule.getDescription()).isEqualTo("New description"); + assertThat(rule.getSeverityString()).isEqualTo("MAJOR"); + assertThat(rule.getStatus()).isEqualTo(RuleStatus.READY); + + List params = db.ruleDao().findRuleParamsByRuleKey(dbSession, RULE_KEY); + assertThat(params).hasSize(1); + RuleParamDto param = params.get(0); + assertThat(param.getDefaultValue()).isEqualTo("a.*"); + } } diff --git a/sonar-server/src/test/java/org/sonar/server/rule/ws/CreateActionTest.java b/sonar-server/src/test/java/org/sonar/server/rule/ws/CreateActionTest.java index ff3ecf74392..444844952d7 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/ws/CreateActionTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/ws/CreateActionTest.java @@ -73,7 +73,7 @@ public class CreateActionTest { assertThat(newRule.htmlDescription()).isEqualTo("Description"); assertThat(newRule.severity()).isEqualTo("MAJOR"); assertThat(newRule.status()).isEqualTo(RuleStatus.BETA); - assertThat(newRule.params()).hasSize(1); - assertThat(newRule.params().get("key")).isEqualTo("value"); + assertThat(newRule.parameters()).hasSize(1); + assertThat(newRule.parameters().get("key")).isEqualTo("value"); } } -- 2.39.5