SONAR-4525 Prevent bulk actions used for issues planning and assignment from doing unnecessary SQL requests

5 files changed, 98 insertions, 65 deletions

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java b/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
index 808d8690f75..2b4a7ea07fb 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
@@ -37,12 +37,11 @@ import java.util.Map;
 public class AssignAction extends Action implements ServerComponent {
 
   public static final String KEY = "assign";
+  public static final String VERIFIED_ASSIGNEE = "verifiedAssignee";
 
   private final UserFinder userFinder;
   private final IssueUpdater issueUpdater;
 
-  private User assignee;
-
   public AssignAction(UserFinder userFinder, IssueUpdater issueUpdater) {
     super(KEY);
     this.userFinder = userFinder;
@@ -53,26 +52,32 @@ public class AssignAction extends Action implements ServerComponent {
   @Override
   public boolean verify(Map<String, Object> properties, List<Issue> issues, UserSession userSession){
     String assignee = assigneeValue(properties);
-    if (!Strings.isNullOrEmpty(assignee) && getOrSelectUser(assignee) == null) {
-      throw new IllegalArgumentException("Unknown user: " + assignee);
+    if(!Strings.isNullOrEmpty(assignee)) {
+      User user = selectUser(assignee);
+      if (user == null) {
+        throw new IllegalArgumentException("Unknown user: " + assignee);
+      }
+      properties.put(VERIFIED_ASSIGNEE, user);
+    } else {
+      properties.put(VERIFIED_ASSIGNEE, null);
     }
     return true;
   }
 
   @Override
   public boolean execute(Map<String, Object> properties, Context context) {
-    String assignee = assigneeValue(properties);
-    return issueUpdater.assign((DefaultIssue) context.issue(), getOrSelectUser(assignee), context.issueChangeContext());
+    if(!properties.containsKey(VERIFIED_ASSIGNEE)) {
+      throw new IllegalArgumentException("Assignee is missing from the execution parameters");
+    }
+    User assignee = (User) properties.get(VERIFIED_ASSIGNEE);
+    return issueUpdater.assign((DefaultIssue) context.issue(), assignee, context.issueChangeContext());
   }
 
   private String assigneeValue(Map<String, Object> properties) {
     return (String) properties.get("assignee");
   }
 
-  private User getOrSelectUser(String assigneeKey) {
-    if(assignee == null) {
-      assignee = userFinder.findByLogin(assigneeKey);
-    }
-    return assignee;
+  private User selectUser(String assigneeKey) {
+    return userFinder.findByLogin(assigneeKey);
   }
 }
\ No newline at end of file
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
index dc9253a3910..7fbbc7efc42 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
@@ -87,9 +87,9 @@ public class IssueBulkChangeService {
 
   private List<Action> getActionsToApply(IssueBulkChangeQuery issueBulkChangeQuery, List<Issue> issues, UserSession userSession) {
     List<Action> bulkActions = newArrayList();
-    for (String actionName : issueBulkChangeQuery.actions()) {
-      Action action = getAction(actionName);
-      if (action.verify(issueBulkChangeQuery.properties(actionName), issues, userSession)) {
+    for (String actionKey : issueBulkChangeQuery.actions()) {
+      Action action = getAction(actionKey);
+      if (action.verify(issueBulkChangeQuery.properties(actionKey), issues, userSession)) {
         bulkActions.add(action);
       }
     }
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java b/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
index 73f2b444627..4c4cd470e72 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
@@ -36,6 +36,7 @@ import java.util.Map;
 public class PlanAction extends Action implements ServerComponent {
 
   public static final String KEY = "plan";
+  public static final String VERIFIED_ACTION_PLAN = "verifiedActionPlan";
 
   private final ActionPlanService actionPlanService;
   private final IssueUpdater issueUpdater;
@@ -56,17 +57,19 @@ public class PlanAction extends Action implements ServerComponent {
         throw new IllegalArgumentException("Unknown action plan: " + actionPlanValue);
       }
       verifyIssuesAreAllRelatedOnActionPlanProject(issues, actionPlan);
+      properties.put(VERIFIED_ACTION_PLAN, actionPlan);
+    } else {
+      properties.put(VERIFIED_ACTION_PLAN, null);
     }
     return true;
   }
 
   @Override
   public boolean execute(Map<String, Object> properties, Context context) {
-    ActionPlan actionPlan = null;
-    String actionPlanValue = planValue(properties);
-    if (!Strings.isNullOrEmpty(actionPlanValue)) {
-      actionPlan = selectActionPlan(actionPlanValue, UserSession.get());
+    if(!properties.containsKey(VERIFIED_ACTION_PLAN)) {
+      throw new IllegalArgumentException("Action plan is missing from the execution parameters");
     }
+    ActionPlan actionPlan = (ActionPlan) properties.get(VERIFIED_ACTION_PLAN);
     return issueUpdater.plan((DefaultIssue) context.issue(), actionPlan, context.issueChangeContext());
   }
 
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java b/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
index 7c5d7d43bb0..c21512bd652 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
@@ -21,7 +21,9 @@
 package org.sonar.server.issue;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.internal.DefaultIssue;
 import org.sonar.api.issue.internal.IssueChangeContext;
@@ -48,72 +50,84 @@ public class AssignActionTest {
   private final UserFinder userFinder = mock(UserFinder.class);
   private IssueUpdater issueUpdater = mock(IssueUpdater.class);
 
+  @Rule
+  public ExpectedException throwable = ExpectedException.none();
+
   @Before
-  public void before(){
+  public void before() {
     action = new AssignAction(userFinder, issueUpdater);
   }
 
   @Test
-  public void should_execute(){
-    String assignee = "arthur";
+  public void should_execute() {
+    User assignee = new DefaultUser();
 
     Map<String, Object> properties = newHashMap();
-    properties.put("assignee", assignee);
+    properties.put(AssignAction.VERIFIED_ASSIGNEE, assignee);
     DefaultIssue issue = mock(DefaultIssue.class);
 
     Action.Context context = mock(Action.Context.class);
     when(context.issue()).thenReturn(issue);
 
-    User user = new DefaultUser();
-    when(userFinder.findByLogin(assignee)).thenReturn(user);
+    action.execute(properties, context);
+    verify(issueUpdater).assign(eq(issue), eq(assignee), any(IssueChangeContext.class));
+  }
+
+  @Test
+  public void should_fail_if_assignee_is_not_verified() throws Exception {
+    throwable.expect(IllegalArgumentException.class);
+    throwable.expectMessage("Assignee is missing from the execution parameters");
+
+    Map<String, Object> properties = newHashMap();
+
+    Action.Context context = mock(Action.Context.class);
 
     action.execute(properties, context);
-    verify(issueUpdater).assign(eq(issue), eq(user), any(IssueChangeContext.class));
   }
 
   @Test
-  public void should_verify_assignee_exists(){
+  public void should_verify_assignee_exists() {
     String assignee = "arthur";
     Map<String, Object> properties = newHashMap();
     properties.put("assignee", assignee);
 
+    User user = new DefaultUser().setLogin(assignee);
+
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC"));
-    when(userFinder.findByLogin(assignee)).thenReturn(new DefaultUser());
+    when(userFinder.findByLogin(assignee)).thenReturn(user);
     assertThat(action.verify(properties, issues, mock(UserSession.class))).isTrue();
+    assertThat(properties.get(AssignAction.VERIFIED_ASSIGNEE)).isEqualTo(user);
   }
 
   @Test
-  public void should_fail_if_assignee_does_not_exists(){
+  public void should_fail_if_assignee_does_not_exists() {
+    throwable.expect(IllegalArgumentException.class);
+    throwable.expectMessage("Unknown user: arthur");
+
     String assignee = "arthur";
     Map<String, Object> properties = newHashMap();
     properties.put("assignee", assignee);
 
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC"));
     when(userFinder.findByLogin(assignee)).thenReturn(null);
-    try {
-      action.verify(properties, issues, mock(UserSession.class));
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown user: arthur");
-    }
+    action.verify(properties, issues, mock(UserSession.class));
   }
 
   @Test
-  public void should_fail_if_assignee_is_empty(){
+  public void should_unassign_if_assignee_is_empty() {
     String assignee = "";
     Map<String, Object> properties = newHashMap();
     properties.put("assignee", assignee);
 
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC"));
-    when(userFinder.findByLogin(assignee)).thenReturn(null);
-    try {
-      action.verify(properties, issues, mock(UserSession.class));
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown user: ");
-    }
+    action.verify(properties, issues, mock(UserSession.class));
+    assertThat(action.verify(properties, issues, mock(UserSession.class))).isTrue();
+    assertThat(properties.containsKey(AssignAction.VERIFIED_ASSIGNEE)).isTrue();
+    assertThat(properties.get(AssignAction.VERIFIED_ASSIGNEE)).isNull();
   }
 
   @Test
-  public void should_support_only_unresolved_issues(){
+  public void should_support_only_unresolved_issues() {
     assertThat(action.supports(new DefaultIssue().setResolution(null))).isTrue();
     assertThat(action.supports(new DefaultIssue().setResolution(Issue.RESOLUTION_FIXED))).isFalse();
   }
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java b/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
index a1a5fcded8d..04c89753eeb 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
@@ -21,7 +21,9 @@
 package org.sonar.server.issue;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.issue.ActionPlan;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.internal.DefaultIssue;
@@ -47,6 +49,9 @@ public class PlanActionTest {
   private ActionPlanService actionPlanService = mock(ActionPlanService.class);
   private IssueUpdater issueUpdater = mock(IssueUpdater.class);
 
+  @Rule
+  public ExpectedException throwable = ExpectedException.none();
+
   @Before
   public void before(){
     action = new PlanAction(actionPlanService, issueUpdater);
@@ -54,24 +59,33 @@ public class PlanActionTest {
 
   @Test
   public void should_execute(){
-    String planKey = "ABCD";
+    ActionPlan actionPlan = new DefaultActionPlan();
     Map<String, Object> properties = newHashMap();
-    properties.put("plan", planKey);
+    properties.put(PlanAction.VERIFIED_ACTION_PLAN, actionPlan);
     DefaultIssue issue = mock(DefaultIssue.class);
 
     Action.Context context = mock(Action.Context.class);
     when(context.issue()).thenReturn(issue);
 
-    ActionPlan actionPlan = new DefaultActionPlan();
-    when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(actionPlan);
-
     action.execute(properties, context);
     verify(issueUpdater).plan(eq(issue), eq(actionPlan), any(IssueChangeContext.class));
   }
 
   @Test
+  public void should_fail_on_unverified_action_plan() throws Exception {
+    throwable.expect(IllegalArgumentException.class);
+    throwable.expectMessage("Action plan is missing from the execution parameters");
+
+    Map<String, Object> properties = newHashMap();
+    Action.Context context = mock(Action.Context.class);
+
+    action.execute(properties, context);
+  }
+
+  @Test
   public void should_execute_on_null_action_plan(){
     Map<String, Object> properties = newHashMap();
+    properties.put(PlanAction.VERIFIED_ACTION_PLAN, null);
     DefaultIssue issue = mock(DefaultIssue.class);
 
     Action.Context context = mock(Action.Context.class);
@@ -86,55 +100,52 @@ public class PlanActionTest {
     String planKey = "ABCD";
     Map<String, Object> properties = newHashMap();
     properties.put("plan", planKey);
+    ActionPlan actionPlan = new DefaultActionPlan().setProjectKey("struts");
 
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC").setProjectKey("struts"));
-    when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(new DefaultActionPlan().setProjectKey("struts"));
+    when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(actionPlan);
     assertThat(action.verify(properties, issues, mock(UserSession.class))).isTrue();
+    assertThat(properties.get(PlanAction.VERIFIED_ACTION_PLAN)).isEqualTo(actionPlan);
   }
 
   @Test
-  public void should_fail_if_action_plan_does_not_exists(){
+  public void should_fail_if_action_plan_does_not_exist(){
+    throwable.expect(IllegalArgumentException.class);
+    throwable.expectMessage("Unknown action plan: ABCD");
+
     String planKey = "ABCD";
     Map<String, Object> properties = newHashMap();
     properties.put("plan", planKey);
 
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC").setProjectKey("struts"));
     when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(null);
-    try {
-      action.verify(properties, issues, mock(UserSession.class));
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown action plan: ABCD");
-    }
+    action.verify(properties, issues, mock(UserSession.class));
   }
 
   @Test
-  public void should_fail_if_action_plan_is_empty(){
+  public void should_unplan_if_action_plan_is_empty() throws Exception {
     String planKey = "";
     Map<String, Object> properties = newHashMap();
     properties.put("plan", planKey);
 
     List<Issue> issues = newArrayList((Issue) new DefaultIssue().setKey("ABC").setProjectKey("struts"));
-    when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(null);
-    try {
-      action.verify(properties, issues, mock(UserSession.class));
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown action plan: ");
-    }
+    action.verify(properties, issues, mock(UserSession.class));
+    assertThat(properties.containsKey(PlanAction.VERIFIED_ACTION_PLAN)).isTrue();
+    assertThat(properties.get(PlanAction.VERIFIED_ACTION_PLAN)).isNull();
   }
 
   @Test
   public void should_verify_issues_are_on_the_same_project(){
+    throwable.expect(IllegalArgumentException.class);
+    throwable.expectMessage("Issues are not all related to the action plan project: struts");
+
     String planKey = "ABCD";
     Map<String, Object> properties = newHashMap();
     properties.put("plan", planKey);
 
     when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(new DefaultActionPlan().setProjectKey("struts"));
     List<Issue> issues = newArrayList(new DefaultIssue().setKey("ABC").setProjectKey("struts"), (Issue) new DefaultIssue().setKey("ABE").setProjectKey("mybatis"));
-    try {
-      action.verify(properties, issues, mock(UserSession.class));
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Issues are not all related to the action plan project: struts");
-    }
+    action.verify(properties, issues, mock(UserSession.class));
   }
 
   @Test