From f5b00283602faef90f1f1a3c36a3754cb0bf847b Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Vilain Date: Wed, 14 Aug 2013 13:55:34 +0200 Subject: [PATCH] SONAR-4525 Prevent bulk actions used for issues planning and assignment from doing unnecessary SQL requests --- .../org/sonar/server/issue/AssignAction.java | 27 +++++---- .../server/issue/IssueBulkChangeService.java | 6 +- .../org/sonar/server/issue/PlanAction.java | 11 ++-- .../sonar/server/issue/AssignActionTest.java | 60 ++++++++++++------- .../sonar/server/issue/PlanActionTest.java | 59 ++++++++++-------- 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 properties, List 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 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 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 getActionsToApply(IssueBulkChangeQuery issueBulkChangeQuery, List issues, UserSession userSession) { List 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 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 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 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 properties = newHashMap(); properties.put("assignee", assignee); + User user = new DefaultUser().setLogin(assignee); + List 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 properties = newHashMap(); properties.put("assignee", assignee); List 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 properties = newHashMap(); properties.put("assignee", assignee); List 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 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 properties = newHashMap(); + Action.Context context = mock(Action.Context.class); + + action.execute(properties, context); + } + @Test public void should_execute_on_null_action_plan(){ Map 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 properties = newHashMap(); properties.put("plan", planKey); + ActionPlan actionPlan = new DefaultActionPlan().setProjectKey("struts"); List 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 properties = newHashMap(); properties.put("plan", planKey); List 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 properties = newHashMap(); properties.put("plan", planKey); List 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 properties = newHashMap(); properties.put("plan", planKey); when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(new DefaultActionPlan().setProjectKey("struts")); List 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 -- 2.39.5