From: Sébastien Lesaint Date: Mon, 9 Dec 2019 13:00:54 +0000 (+0100) Subject: SONAR-12720 add optional param comment to api/hotspot/change_status X-Git-Tag: 8.2.0.32929~199 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=9c03dbbbe8abaf7da57d1a26084bfdeb1df2458d;p=sonarqube.git SONAR-12720 add optional param comment to api/hotspot/change_status --- diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/ChangeStatusAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/ChangeStatusAction.java index 3da4564254a..1b80cb9eea0 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/ChangeStatusAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/ChangeStatusAction.java @@ -21,6 +21,7 @@ package org.sonar.server.hotspot.ws; import java.util.Date; import java.util.Objects; +import javax.annotation.Nullable; import org.sonar.api.issue.DefaultTransitions; import org.sonar.api.rules.RuleType; import org.sonar.api.server.ws.Request; @@ -36,12 +37,14 @@ import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; import org.sonar.db.issue.IssueDto; import org.sonar.server.exceptions.NotFoundException; +import org.sonar.server.issue.IssueFieldsSetter; import org.sonar.server.issue.TransitionService; import org.sonar.server.issue.ws.IssueUpdater; import org.sonar.server.user.UserSession; import static com.google.common.base.Preconditions.checkArgument; import static java.lang.String.format; +import static org.apache.commons.lang.StringUtils.trimToNull; import static org.sonar.api.issue.Issue.RESOLUTION_FIXED; import static org.sonar.api.issue.Issue.SECURITY_HOTSPOT_RESOLUTIONS; import static org.sonar.api.issue.Issue.STATUS_REVIEWED; @@ -52,17 +55,21 @@ public class ChangeStatusAction implements HotspotsWsAction { private static final String PARAM_HOTSPOT_KEY = "hotspot"; private static final String PARAM_RESOLUTION = "resolution"; private static final String PARAM_STATUS = "status"; + private static final String PARAM_COMMENT = "comment"; private final DbClient dbClient; private final UserSession userSession; private final TransitionService transitionService; + private final IssueFieldsSetter issueFieldsSetter; private final System2 system2; private final IssueUpdater issueUpdater; - public ChangeStatusAction(DbClient dbClient, UserSession userSession, TransitionService transitionService, System2 system2, IssueUpdater issueUpdater) { + public ChangeStatusAction(DbClient dbClient, UserSession userSession, TransitionService transitionService, + IssueFieldsSetter issueFieldsSetter, System2 system2, IssueUpdater issueUpdater) { this.dbClient = dbClient; this.userSession = userSession; this.transitionService = transitionService; + this.issueFieldsSetter = issueFieldsSetter; this.system2 = system2; this.issueUpdater = issueUpdater; } @@ -88,6 +95,9 @@ public class ChangeStatusAction implements HotspotsWsAction { action.createParam(PARAM_RESOLUTION) .setDescription("Resolution of the Security Hotspot when new status is " + STATUS_REVIEWED + ", otherwise must not be set.") .setPossibleValues(SECURITY_HOTSPOT_RESOLUTIONS); + action.createParam(PARAM_COMMENT) + .setDescription("Comment text.") + .setExampleValue("This is safe because user input is validated by the calling code"); } @Override @@ -105,7 +115,7 @@ public class ChangeStatusAction implements HotspotsWsAction { if (needStatusUpdate(hotspot, newStatus, newResolution)) { String transitionKey = toTransitionKey(newStatus, newResolution); - doTransition(dbSession, hotspot, transitionKey); + doTransition(dbSession, hotspot, transitionKey, trimToNull(request.param(PARAM_COMMENT))); } response.noContent(); } @@ -114,11 +124,11 @@ public class ChangeStatusAction implements HotspotsWsAction { private static String resolutionParam(Request request, String newStatus) { String resolution = request.param(PARAM_RESOLUTION); checkArgument(STATUS_REVIEWED.equals(newStatus) || resolution == null, - "Parameter '%s' must not be specified when Parameter '%s' has value '%s'", - PARAM_RESOLUTION, PARAM_STATUS, STATUS_TO_REVIEW); + "Parameter '%s' must not be specified when Parameter '%s' has value '%s'", + PARAM_RESOLUTION, PARAM_STATUS, STATUS_TO_REVIEW); checkArgument(STATUS_TO_REVIEW.equals(newStatus) || resolution != null, - "Parameter '%s' must be specified when Parameter '%s' has value '%s'", - PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED); + "Parameter '%s' must be specified when Parameter '%s' has value '%s'", + PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED); return resolution; } @@ -127,7 +137,7 @@ public class ChangeStatusAction implements HotspotsWsAction { checkArgument(projectUuid != null, "Hotspot '%s' has no project", hotspot.getKee()); ComponentDto project = dbClient.componentDao().selectByUuid(dbSession, projectUuid) - .orElseThrow(() -> new NotFoundException(format("Project with uuid '%s' does not exist", projectUuid))); + .orElseThrow(() -> new NotFoundException(format("Project with uuid '%s' does not exist", projectUuid))); userSession.checkComponentPermission(UserRole.USER, project); } @@ -145,11 +155,14 @@ public class ChangeStatusAction implements HotspotsWsAction { return DefaultTransitions.RESOLVE_AS_SAFE; } - private void doTransition(DbSession session, IssueDto issueDto, String transitionKey) { + private void doTransition(DbSession session, IssueDto issueDto, String transitionKey, @Nullable String comment) { DefaultIssue defaultIssue = issueDto.toDefaultIssue(); IssueChangeContext context = IssueChangeContext.createUser(new Date(system2.now()), userSession.getUuid()); transitionService.checkTransitionPermission(transitionKey, defaultIssue); if (transitionService.doTransition(defaultIssue, context, transitionKey)) { + if (comment != null) { + issueFieldsSetter.addComment(defaultIssue, comment, context); + } issueUpdater.saveIssueAndPreloadSearchResponseData(session, defaultIssue, context, true); } } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/ChangeStatusActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/ChangeStatusActionTest.java index de0b1162bf1..fa9d57c9e90 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/ChangeStatusActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/ChangeStatusActionTest.java @@ -24,7 +24,7 @@ import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.util.Arrays; import java.util.Date; -import java.util.Objects; +import java.util.List; import java.util.Random; import java.util.function.Consumer; import java.util.stream.Stream; @@ -32,7 +32,7 @@ import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.ArgumentMatcher; +import org.mockito.ArgumentCaptor; import org.sonar.api.issue.DefaultTransitions; import org.sonar.api.issue.Issue; import org.sonar.api.rules.RuleType; @@ -51,6 +51,7 @@ import org.sonar.db.rule.RuleTesting; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.exceptions.UnauthorizedException; +import org.sonar.server.issue.IssueFieldsSetter; import org.sonar.server.issue.TransitionService; import org.sonar.server.issue.ws.IssueUpdater; import org.sonar.server.tester.UserSessionRule; @@ -61,7 +62,7 @@ import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -77,6 +78,7 @@ import static org.sonar.db.component.ComponentTesting.newFileDto; @RunWith(DataProviderRunner.class) public class ChangeStatusActionTest { private static final Random RANDOM = new Random(); + private static final String NO_COMMENT = null; @Rule public DbTester dbTester = DbTester.create(System2.INSTANCE); @@ -87,7 +89,8 @@ public class ChangeStatusActionTest { private TransitionService transitionService = mock(TransitionService.class); private IssueUpdater issueUpdater = mock(IssueUpdater.class); private System2 system2 = mock(System2.class); - private ChangeStatusAction underTest = new ChangeStatusAction(dbClient, userSessionRule, transitionService, system2, issueUpdater); + private IssueFieldsSetter issueFieldsSetter = mock(IssueFieldsSetter.class); + private ChangeStatusAction underTest = new ChangeStatusAction(dbClient, userSessionRule, transitionService, issueFieldsSetter, system2, issueUpdater); private WsActionTester actionTester = new WsActionTester(underTest); @Test @@ -247,7 +250,7 @@ public class ChangeStatusActionTest { RuleDefinitionDto rule = newRule(ruleType); IssueDto notAHotspot = dbTester.issues().insertIssue(IssueTesting.newIssue(rule, project, file).setType(ruleType)); userSessionRule.logIn(); - TestRequest request = newRequest(notAHotspot, status, resolution); + TestRequest request = newRequest(notAHotspot, status, resolution, NO_COMMENT); assertThatThrownBy(request::execute) .isInstanceOf(NotFoundException.class) @@ -270,7 +273,7 @@ public class ChangeStatusActionTest { ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule)); - TestRequest request = newRequest(hotspot, status, resolution); + TestRequest request = newRequest(hotspot, status, resolution, NO_COMMENT); assertThatThrownBy(request::execute) .isInstanceOf(ForbiddenException.class) @@ -286,7 +289,7 @@ public class ChangeStatusActionTest { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule)); - newRequest(hotspot, status, resolution).execute().assertNoContent(); + newRequest(hotspot, status, resolution, NO_COMMENT).execute().assertNoContent(); } @Test @@ -298,7 +301,7 @@ public class ChangeStatusActionTest { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule)); - newRequest(hotspot, status, resolution).execute().assertNoContent(); + newRequest(hotspot, status, resolution, NO_COMMENT).execute().assertNoContent(); } @Test @@ -310,8 +313,9 @@ public class ChangeStatusActionTest { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule).setStatus(status).setResolution(resolution)); - newRequest(hotspot, status, resolution).execute().assertNoContent(); - verifyNoInteractions(transitionService, issueUpdater); + newRequest(hotspot, status, resolution, NO_COMMENT).execute().assertNoContent(); + + verifyNoInteractions(transitionService, issueUpdater, issueFieldsSetter); } @Test @@ -326,21 +330,27 @@ public class ChangeStatusActionTest { IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule).setStatus(STATUS_TO_REVIEW).setResolution(null)); when(transitionService.doTransition(any(), any(), any())).thenReturn(transitionDone); - newRequest(hotspot, STATUS_REVIEWED, resolution).execute().assertNoContent(); + newRequest(hotspot, STATUS_REVIEWED, resolution, NO_COMMENT).execute().assertNoContent(); + IssueChangeContext issueChangeContext = IssueChangeContext.createUser(new Date(now), userSessionRule.getUuid()); - DefaultIssueMatcher defaultIssueMatcher = new DefaultIssueMatcher(hotspot); + ArgumentCaptor defaultIssueCaptor = ArgumentCaptor.forClass(DefaultIssue.class); + verify(transitionService).checkTransitionPermission(eq(expectedTransitionKey), defaultIssueCaptor.capture()); verify(transitionService).doTransition( - argThat(defaultIssueMatcher), + defaultIssueCaptor.capture(), eq(issueChangeContext), eq(expectedTransitionKey)); if (transitionDone) { verify(issueUpdater).saveIssueAndPreloadSearchResponseData( any(DbSession.class), - argThat(defaultIssueMatcher), + defaultIssueCaptor.capture(), eq(issueChangeContext), eq(true)); + + // because it is mutated by FieldSetter and IssueUpdater, the same object must be passed to all methods + verifyAllSame3Objects(defaultIssueCaptor.getAllValues()); + verifyNoInteractions(issueFieldsSetter); } else { - verifyNoInteractions(issueUpdater); + verifyNoInteractions(issueUpdater, issueFieldsSetter); } } @@ -366,21 +376,27 @@ public class ChangeStatusActionTest { IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule).setStatus(STATUS_REVIEWED).setResolution(resolution)); when(transitionService.doTransition(any(), any(), any())).thenReturn(transitionDone); - newRequest(hotspot, STATUS_TO_REVIEW, null).execute().assertNoContent(); + newRequest(hotspot, STATUS_TO_REVIEW, null, NO_COMMENT).execute().assertNoContent(); + IssueChangeContext issueChangeContext = IssueChangeContext.createUser(new Date(now), userSessionRule.getUuid()); - DefaultIssueMatcher defaultIssueMatcher = new DefaultIssueMatcher(hotspot); + ArgumentCaptor defaultIssueCaptor = ArgumentCaptor.forClass(DefaultIssue.class); + verify(transitionService).checkTransitionPermission(eq(DefaultTransitions.RESET_AS_TO_REVIEW), defaultIssueCaptor.capture()); verify(transitionService).doTransition( - argThat(defaultIssueMatcher), + defaultIssueCaptor.capture(), eq(issueChangeContext), eq(DefaultTransitions.RESET_AS_TO_REVIEW)); if (transitionDone) { verify(issueUpdater).saveIssueAndPreloadSearchResponseData( any(DbSession.class), - argThat(defaultIssueMatcher), + defaultIssueCaptor.capture(), eq(issueChangeContext), eq(true)); + + // because it is mutated by FieldSetter and IssueUpdater, the same object must be passed to all methods + verifyAllSame3Objects(defaultIssueCaptor.getAllValues()); + verifyNoInteractions(issueFieldsSetter); } else { - verifyNoInteractions(issueUpdater); + verifyNoInteractions(issueUpdater, issueFieldsSetter); } } @@ -394,73 +410,89 @@ public class ChangeStatusActionTest { }; } - private static class DefaultIssueMatcher implements ArgumentMatcher { - private final DefaultIssue expected; + @Test + @UseDataProvider("changingStatusAndTransitionFlag") + public void persists_comment_if_hotspot_status_changes_and_transition_done(String currentStatus, @Nullable String currentResolution, + String newStatus, @Nullable String newResolution, boolean transitionDone) { + long now = RANDOM.nextInt(232_323); + when(system2.now()).thenReturn(now); + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.logIn().registerComponents(project); + ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule).setStatus(currentStatus).setResolution(currentResolution)); + when(transitionService.doTransition(any(), any(), any())).thenReturn(transitionDone); + String comment = randomAlphabetic(12); + + newRequest(hotspot, newStatus, newResolution, comment).execute().assertNoContent(); - private DefaultIssueMatcher(IssueDto issueDto) { - this.expected = issueDto.toDefaultIssue(); - } + IssueChangeContext issueChangeContext = IssueChangeContext.createUser(new Date(now), userSessionRule.getUuid()); + ArgumentCaptor defaultIssueCaptor = ArgumentCaptor.forClass(DefaultIssue.class); + verify(transitionService).doTransition(defaultIssueCaptor.capture(), eq(issueChangeContext), anyString()); + if (transitionDone) { + verify(issueFieldsSetter).addComment(defaultIssueCaptor.capture(), eq(comment), eq(issueChangeContext)); + verify(issueUpdater).saveIssueAndPreloadSearchResponseData( + any(DbSession.class), + defaultIssueCaptor.capture(), + eq(issueChangeContext), + eq(true)); - @Override - public boolean matches(DefaultIssue that) { - if (expected == that) { - return true; - } - if (that == null || expected.getClass() != that.getClass()) { - return false; - } - return expected.manualSeverity() == that.manualSeverity() && - expected.isFromExternalRuleEngine() == that.isFromExternalRuleEngine() && - expected.isNew() == that.isNew() && - expected.isCopied() == that.isCopied() && - expected.isBeingClosed() == that.isBeingClosed() && - expected.isOnDisabledRule() == that.isOnDisabledRule() && - expected.isChanged() == that.isChanged() && - expected.mustSendNotifications() == that.mustSendNotifications() && - Objects.equals(expected.key(), that.key()) && - expected.type() == that.type() && - Objects.equals(expected.componentUuid(), that.componentUuid()) && - Objects.equals(expected.componentKey(), that.componentKey()) && - Objects.equals(expected.moduleUuid(), that.moduleUuid()) && - Objects.equals(expected.moduleUuidPath(), that.moduleUuidPath()) && - Objects.equals(expected.projectUuid(), that.projectUuid()) && - Objects.equals(expected.projectKey(), that.projectKey()) && - Objects.equals(expected.ruleKey(), that.ruleKey()) && - Objects.equals(expected.language(), that.language()) && - Objects.equals(expected.severity(), that.severity()) && - Objects.equals(expected.message(), that.message()) && - Objects.equals(expected.line(), that.line()) && - Objects.equals(expected.gap(), that.gap()) && - Objects.equals(expected.effort(), that.effort()) && - Objects.equals(expected.status(), that.status()) && - Objects.equals(expected.resolution(), that.resolution()) && - Objects.equals(expected.assignee(), that.assignee()) && - Objects.equals(expected.checksum(), that.checksum()) && - Objects.equals(expected.attributes(), that.attributes()) && - Objects.equals(expected.authorLogin(), that.authorLogin()) && - Objects.equals(expected.comments(), that.comments()) && - Objects.equals(expected.tags(), that.tags()) && - Objects.equals(expected.getLocations(), that.getLocations()) && - Objects.equals(expected.creationDate(), that.creationDate()) && - Objects.equals(expected.updateDate(), that.updateDate()) && - Objects.equals(expected.closeDate(), that.closeDate()) && - Objects.equals(expected.currentChange(), that.currentChange()) && - Objects.equals(expected.changes(), that.changes()) && - Objects.equals(expected.selectedAt(), that.selectedAt()); + // because it is mutated by FieldSetter and IssueUpdater, the same object must be passed to all methods + verifyAllSame3Objects(defaultIssueCaptor.getAllValues()); + } else { + verifyNoInteractions(issueUpdater, issueFieldsSetter); } } + @DataProvider + public static Object[][] changingStatusAndTransitionFlag() { + Object[][] changingStatuses = { + {STATUS_TO_REVIEW, null, STATUS_REVIEWED, RESOLUTION_FIXED}, + {STATUS_TO_REVIEW, null, STATUS_REVIEWED, RESOLUTION_FIXED}, + {STATUS_TO_REVIEW, null, STATUS_REVIEWED, RESOLUTION_SAFE}, + {STATUS_REVIEWED, RESOLUTION_FIXED, STATUS_REVIEWED, RESOLUTION_SAFE}, + {STATUS_REVIEWED, RESOLUTION_FIXED, STATUS_TO_REVIEW, null}, + {STATUS_REVIEWED, RESOLUTION_SAFE, STATUS_REVIEWED, RESOLUTION_FIXED}, + {STATUS_REVIEWED, RESOLUTION_SAFE, STATUS_TO_REVIEW, null} + }; + return Arrays.stream(changingStatuses) + .flatMap(b -> Stream.of( + new Object[] {b[0], b[1], b[2], b[3], true}, + new Object[] {b[0], b[1], b[2], b[3], false})) + .toArray(Object[][]::new); + } + + @Test + @UseDataProvider("validStatusAndResolutions") + public void do_not_persist_comment_if_no_status_change(String status, @Nullable String resolution) { + long now = RANDOM.nextInt(232_323); + when(system2.now()).thenReturn(now); + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.logIn().registerComponents(project); + ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + IssueDto hotspot = dbTester.issues().insertIssue(newHotspot(project, file, rule).setStatus(status).setResolution(resolution)); + String comment = randomAlphabetic(12); + + newRequest(hotspot, status, resolution, comment).execute().assertNoContent(); + + verifyNoInteractions(transitionService, issueUpdater, issueFieldsSetter); + } + private static IssueDto newHotspot(ComponentDto project, ComponentDto file, RuleDefinitionDto rule) { return IssueTesting.newIssue(rule, project, file).setType(SECURITY_HOTSPOT); } - private TestRequest newRequest(IssueDto hotspot, String newStatus, @Nullable String newResolution) { + private TestRequest newRequest(IssueDto hotspot, String newStatus, @Nullable String newResolution, @Nullable String comment) { TestRequest res = actionTester.newRequest() .setParam("hotspot", hotspot.getKey()) .setParam("status", newStatus); if (newResolution != null) { res.setParam("resolution", newResolution); } + if (comment != null) { + res.setParam("comment", comment); + } return res; } @@ -477,4 +509,11 @@ public class ChangeStatusActionTest { return ruleDefinition; } + private static void verifyAllSame3Objects(List allValues) { + assertThat(allValues).hasSize(3); + assertThat(allValues.get(0)) + .isSameAs(allValues.get(1)) + .isSameAs(allValues.get(2)); + } + }