SONAR-7330 WS api/rules/update apply ES+DB pattern

diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
index d14d83c3cc468faac151f25f5e93184ba3c37b60..5010d7fb50501b48b9800fc010a6336d3ac91fed 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
@@ -96,11 +96,6 @@ public class RuleService {
     return index.terms(RuleNormalizer.RuleField.ALL_TAGS.field(), query, size);
   }
 
-  public void update(RuleUpdate update) {
-    checkPermission();
-    ruleUpdater.update(update, userSession);
-  }
-
   public RuleKey create(NewRule newRule) {
     checkPermission();
     return ruleCreator.create(newRule);

diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java
index 1d0424c1ba22ad00cc1356b4983137b0a7077f6d..d7248f5b52228ca3b2fb32ad976c44f170d42759 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdater.java
@@ -57,24 +57,18 @@ public class RuleUpdater {
   /**
    * Update manual rules and custom rules (rules instantiated from templates)
    */
-  public boolean update(RuleUpdate update, UserSession userSession) {
+  public boolean update(DbSession dbSession, RuleUpdate update, UserSession userSession) {
     if (update.isEmpty()) {
       return false;
     }
 
-    DbSession dbSession = dbClient.openSession(false);
-    try {
-      Context context = newContext(update);
-      // validate only the changes, not all the rule fields
-      apply(update, context, userSession);
-      dbClient.deprecatedRuleDao().update(dbSession, context.rule);
-      updateParameters(dbSession, update, context);
-      dbSession.commit();
-      return true;
-
-    } finally {
-      dbSession.close();
-    }
+    Context context = newContext(update);
+    // validate only the changes, not all the rule fields
+    apply(update, context, userSession);
+    dbClient.deprecatedRuleDao().update(dbSession, context.rule);
+    updateParameters(dbSession, update, context);
+    dbSession.commit();
+    return true;
   }
 
   /**

diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java
index 7a2380c10b2456c9e26e610dfa5de21442c8f59e..61b11d32750745af8a50a41a561e78dcfee3a6b7 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java
@@ -19,8 +19,12 @@
  */
 package org.sonar.server.rule.ws;
 
+import com.google.common.base.Optional;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rule.RuleStatus;
@@ -31,12 +35,18 @@ import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
 import org.sonar.api.utils.KeyValueFormat;
-import org.sonar.server.exceptions.NotFoundException;
-import org.sonar.server.rule.Rule;
-import org.sonar.server.rule.RuleService;
+import org.sonar.core.permission.GlobalPermissions;
+import org.sonar.db.DbClient;
+import org.sonar.db.DbSession;
+import org.sonar.db.rule.RuleDto;
+import org.sonar.db.rule.RuleParamDto;
 import org.sonar.server.rule.RuleUpdate;
+import org.sonar.server.rule.RuleUpdater;
+import org.sonar.server.user.UserSession;
 import org.sonarqube.ws.Rules.UpdateResponse;
 
+import static java.util.Collections.singletonList;
+import static org.sonar.server.ws.WsUtils.checkFoundWithOptional;
 import static org.sonar.server.ws.WsUtils.writeProtobuf;
 
 public class UpdateAction implements RulesWsAction {
@@ -54,12 +64,16 @@ public class UpdateAction implements RulesWsAction {
   public static final String PARAM_STATUS = "status";
   public static final String PARAMS = "params";
 
-  private final RuleService service;
-  private final RuleMapping mapping;
+  private final DbClient dbClient;
+  private final RuleUpdater ruleUpdater;
+  private final RuleMapper mapper;
+  private final UserSession userSession;
 
-  public UpdateAction(RuleService service, RuleMapping mapping) {
-    this.service = service;
-    this.mapping = mapping;
+  public UpdateAction(DbClient dbClient, RuleUpdater ruleUpdater, RuleMapper mapper, UserSession userSession) {
+    this.dbClient = dbClient;
+    this.ruleUpdater = ruleUpdater;
+    this.mapper = mapper;
+    this.userSession = userSession;
   }
 
   @Override
@@ -125,16 +139,22 @@ public class UpdateAction implements RulesWsAction {
 
   @Override
   public void handle(Request request, Response response) throws Exception {
-    RuleUpdate update = readRequest(request);
-    service.update(update);
-    UpdateResponse updateResponse = buildResponse(update.getRuleKey());
+    checkPermission();
+    DbSession dbSession = dbClient.openSession(false);
+    try {
+      RuleUpdate update = readRequest(dbSession, request);
+      ruleUpdater.update(dbSession, update, userSession);
+      UpdateResponse updateResponse = buildResponse(dbSession, update.getRuleKey());
 
-    writeProtobuf(updateResponse, request, response);
+      writeProtobuf(updateResponse, request, response);
+    } finally {
+      dbClient.closeSession(dbSession);
+    }
   }
 
-  private RuleUpdate readRequest(Request request) {
+  private RuleUpdate readRequest(DbSession dbSession, Request request) {
     RuleKey key = RuleKey.parse(request.mandatoryParam(PARAM_KEY));
-    RuleUpdate update = createRuleUpdate(key);
+    RuleUpdate update = createRuleUpdate(dbSession, key);
     readTags(request, update);
     readMarkdownNote(request, update);
     readDebt(request, update);
@@ -162,14 +182,13 @@ public class UpdateAction implements RulesWsAction {
     return update;
   }
 
-  private RuleUpdate createRuleUpdate(RuleKey key) {
-    Rule rule = service.getByKey(key);
-    if (rule == null) {
-      throw new NotFoundException("This rule does not exists : " + key);
-    }
-    if (rule.templateKey() != null) {
+  private RuleUpdate createRuleUpdate(DbSession dbSession, RuleKey key) {
+    Optional<RuleDto> optionalRule = dbClient.ruleDao().selectByKey(dbSession, key);
+    checkFoundWithOptional(optionalRule, "This rule does not exists : " + key);
+    RuleDto rule = optionalRule.get();
+    if (rule.getTemplateId() != null) {
       return RuleUpdate.createForCustomRule(key);
-    } else if (rule.isManual()) {
+    } else if (RuleKey.MANUAL_REPOSITORY_KEY.equals(rule.getRepositoryKey())) {
       return RuleUpdate.createForManualRule(key);
     } else {
       return RuleUpdate.createForPluginRule(key);
@@ -210,11 +229,31 @@ public class UpdateAction implements RulesWsAction {
     }
   }
 
-  private UpdateResponse buildResponse(RuleKey ruleKey) {
-    Rule rule = service.getNonNullByKey(ruleKey);
+  private UpdateResponse buildResponse(DbSession dbSession, RuleKey key) {
+    Optional<RuleDto> optionalRule = dbClient.ruleDao().selectByKey(dbSession, key);
+    checkFoundWithOptional(optionalRule, "Rule not found: " + key);
+    RuleDto rule = optionalRule.get();
+    List<RuleDto> templateRules = new ArrayList<>();
+    if (rule.getTemplateId() != null) {
+      Optional<RuleDto> templateRule = dbClient.ruleDao().selectById(rule.getTemplateId(), dbSession);
+      if (templateRule.isPresent()) {
+        templateRules.add(templateRule.get());
+      }
+    }
+    List<RuleParamDto> ruleParameters = dbClient.ruleDao().selectRuleParamsByRuleIds(dbSession, singletonList(rule.getId()));
     UpdateResponse.Builder responseBuilder = UpdateResponse.newBuilder();
-    responseBuilder.setRule(mapping.buildRuleResponse(rule, null /* TODO replace by SearchOptions immutable constant */));
+    SearchAction.SearchResult searchResult = new SearchAction.SearchResult()
+      .setRules(singletonList(rule))
+      .setTemplateRules(templateRules)
+      .setRuleParameters(ruleParameters)
+      .setTotal(1L);
+    responseBuilder.setRule(mapper.toWsRule(rule, searchResult, Collections.<String>emptySet()));
 
     return responseBuilder.build();
   }
+
+  private void checkPermission() {
+    userSession.checkLoggedIn();
+    userSession.checkPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN);
+  }
 }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java
index 24675023257763830117198591df5d80b6bad497..ff9194766d75fad311a6b353536e55afbffda49c 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java
@@ -35,7 +35,6 @@ import org.sonar.api.rule.Severity;
 import org.sonar.api.server.debt.DebtRemediationFunction;
 import org.sonar.api.server.rule.RuleParamType;
 import org.sonar.api.server.rule.RulesDefinition;
-import org.sonar.api.utils.MessageException;
 import org.sonar.core.permission.GlobalPermissions;
 import org.sonar.db.DbSession;
 import org.sonar.db.qualityprofile.ActiveRuleKey;
@@ -461,7 +460,7 @@ public class RegisterRulesMediumTest {
     assertThat(rule.tags()).isEmpty();
 
     // User adds tag
-    TESTER.get(RuleUpdater.class).update(RuleUpdate.createForPluginRule(RuleTesting.XOO_X1).setTags(newHashSet("tag2")), userSessionRule);
+    TESTER.get(RuleUpdater.class).update(dbSession, RuleUpdate.createForPluginRule(RuleTesting.XOO_X1).setTags(newHashSet("tag2")), userSessionRule);
     dbSession.clearCache();
     rule = ruleIndex.getByKey(RuleTesting.XOO_X1);
     assertThat(rule.systemTags()).containsOnly("tag1");

diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
index 21e1aac476c1f2b91dba3651620a6f9e4c44d6b0..544f17680f0357ecdb8c6ded30887ad42e5aecf6 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
@@ -156,41 +156,6 @@ public class RuleServiceMediumTest {
     assertThat(tags).containsOnly("sys1", "sys2");
   }
 
-  @Test
-  public void update_rule() {
-    userSessionRule.login("me").setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN);
-
-    RuleKey key = RuleKey.of("java", "S001");
-
-    dao.insert(dbSession, RuleTesting.newDto(key));
-    dbSession.commit();
-
-    RuleUpdate update = RuleUpdate.createForCustomRule(key);
-    update.setMarkdownNote("my *note*");
-    service.update(update);
-
-    dbSession.clearCache();
-
-    RuleDto rule = dao.getNullableByKey(dbSession, key);
-    assertThat(rule.getNoteData()).isEqualTo("my *note*");
-    assertThat(rule.getNoteUserLogin()).isEqualTo("me");
-  }
-
-  @Test(expected = UnauthorizedException.class)
-  public void do_not_update_if_not_granted() {
-    userSessionRule.setGlobalPermissions(GlobalPermissions.SCAN_EXECUTION);
-
-    RuleKey key = RuleKey.of("java", "S001");
-
-    dao.insert(dbSession, RuleTesting.newDto(key)
-      .setTags(Sets.newHashSet("security"))
-      .setSystemTags(Sets.newHashSet("java8", "javadoc")));
-    dbSession.commit();
-
-    RuleUpdate update = RuleUpdate.createForPluginRule(key).setMarkdownNote("my *note*");
-    service.update(update);
-  }
-
   @Test
   public void create_rule() {
     userSessionRule.login().setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN);

diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java
index 4de502c673fcfae4cd425e9dccff7770f58febac..eeb89016853807d3be2b2e424d62a4a9384ba300 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterMediumTest.java
@@ -86,7 +86,7 @@ public class RuleUpdaterMediumTest {
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY).setTags(Sets.newHashSet("java9"));
     try {
-      updater.update(update, userSessionRule);
+      updater.update(dbSession, update, userSessionRule);
       fail();
     } catch (IllegalArgumentException e) {
       assertThat(e).hasMessage("Rule with REMOVED status cannot be updated: squid:S001");
@@ -107,7 +107,7 @@ public class RuleUpdaterMediumTest {
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY);
     assertThat(update.isEmpty()).isTrue();
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
     RuleDto rule = ruleDao.getNullableByKey(dbSession, RULE_KEY);
@@ -136,7 +136,7 @@ public class RuleUpdaterMediumTest {
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY);
     update.setMarkdownNote("my *note*");
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
     RuleDto rule = ruleDao.getNullableByKey(dbSession, RULE_KEY);
@@ -159,7 +159,7 @@ public class RuleUpdaterMediumTest {
     dbSession.commit();
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY).setMarkdownNote(null);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
     RuleDto rule = ruleDao.getNullableByKey(dbSession, RULE_KEY);
@@ -179,7 +179,7 @@ public class RuleUpdaterMediumTest {
 
     // java8 is a system tag -> ignore
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY).setTags(Sets.newHashSet("bug", "java8"));
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
     RuleDto rule = ruleDao.getNullableByKey(dbSession, RULE_KEY);
@@ -199,7 +199,7 @@ public class RuleUpdaterMediumTest {
     dbSession.commit();
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY).setTags(null);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
     RuleDto rule = ruleDao.getNullableByKey(dbSession, RULE_KEY);
@@ -225,7 +225,7 @@ public class RuleUpdaterMediumTest {
     DefaultDebtRemediationFunction fn = new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.CONSTANT_ISSUE, null, "1min");
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY)
       .setDebtRemediationFunction(fn);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
     dbSession.clearCache();
 
     // verify debt is overridden
@@ -253,7 +253,7 @@ public class RuleUpdaterMediumTest {
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY)
       .setDebtRemediationFunction(new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.LINEAR, "2d", null));
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
     dbSession.clearCache();
 
     // verify debt is overridden
@@ -281,7 +281,7 @@ public class RuleUpdaterMediumTest {
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY)
       .setDebtRemediationFunction(new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.CONSTANT_ISSUE, null, "10min"));
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
     dbSession.clearCache();
 
     // verify debt is overridden
@@ -308,7 +308,7 @@ public class RuleUpdaterMediumTest {
     dbSession.commit();
 
     RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY).setDebtRemediationFunction(null);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
     dbSession.clearCache();
 
     // verify debt is coming from default values
@@ -352,7 +352,7 @@ public class RuleUpdaterMediumTest {
       .setSeverity("MAJOR")
       .setStatus(RuleStatus.READY)
       .setParameters(ImmutableMap.of("regex", "b.*"));
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
 
@@ -394,7 +394,7 @@ public class RuleUpdaterMediumTest {
       .setMarkdownDescription("New description")
       .setSeverity("MAJOR")
       .setStatus(RuleStatus.READY);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
 
@@ -437,7 +437,7 @@ public class RuleUpdaterMediumTest {
     // Update custom rule parameter 'regex', add 'message' and remove 'format'
     RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey())
       .setParameters(ImmutableMap.of("regex", "b.*", "message", "a message"));
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();
 
@@ -482,7 +482,7 @@ public class RuleUpdaterMediumTest {
       .setName("")
       .setMarkdownDescription("New desc");
     try {
-      updater.update(update, userSessionRule);
+      updater.update(dbSession, update, userSessionRule);
       fail();
     } catch (Exception e) {
       assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("The name is missing");
@@ -506,7 +506,7 @@ public class RuleUpdaterMediumTest {
       .setName("New name")
       .setMarkdownDescription("");
     try {
-      updater.update(update, userSessionRule);
+      updater.update(dbSession, update, userSessionRule);
       fail();
     } catch (Exception e) {
       assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("The description is missing");
@@ -529,7 +529,7 @@ public class RuleUpdaterMediumTest {
       .setName("New name")
       .setMarkdownDescription("New description")
       .setSeverity(Severity.CRITICAL);
-    updater.update(update, userSessionRule);
+    updater.update(dbSession, update, userSessionRule);
 
     dbSession.clearCache();