From 75faaab3142a29cde21e3b97981afe6faf0bde22 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Fri, 13 Dec 2019 14:48:55 +0100 Subject: [PATCH] SONAR-12721 add pullRequest and branch parameters api/hotspots/search --- .../sonar/server/hotspot/ws/SearchAction.java | 84 +++++++-- .../server/hotspot/ws/SearchActionTest.java | 169 ++++++++++++++++++ 2 files changed, 241 insertions(+), 12 deletions(-) diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/SearchAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/SearchAction.java index 7ccfed8c524..76a43a6231d 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/SearchAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/SearchAction.java @@ -22,7 +22,6 @@ package org.sonar.server.hotspot.ws; import com.google.common.collect.ImmutableSet; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -63,6 +62,7 @@ import org.sonarqube.ws.Hotspots.SearchWsResponse; import static com.google.common.base.Preconditions.checkArgument; import static java.lang.String.format; import static java.util.Collections.singleton; +import static java.util.Collections.singletonList; import static java.util.Optional.ofNullable; import static org.sonar.api.issue.Issue.RESOLUTION_FIXED; import static org.sonar.api.issue.Issue.RESOLUTION_SAFE; @@ -75,7 +75,9 @@ import static org.sonar.api.utils.Paging.forPageIndex; import static org.sonar.core.util.stream.MoreCollectors.toList; import static org.sonar.core.util.stream.MoreCollectors.uniqueIndex; import static org.sonar.server.security.SecurityStandards.fromSecurityStandards; +import static org.sonar.server.ws.KeyExamples.KEY_BRANCH_EXAMPLE_001; import static org.sonar.server.ws.KeyExamples.KEY_PROJECT_EXAMPLE_001; +import static org.sonar.server.ws.KeyExamples.KEY_PULL_REQUEST_EXAMPLE_001; import static org.sonar.server.ws.WsUtils.writeProtobuf; import static org.sonarqube.ws.WsUtils.nullToEmpty; @@ -84,6 +86,8 @@ public class SearchAction implements HotspotsWsAction { private static final String PARAM_STATUS = "status"; private static final String PARAM_RESOLUTION = "resolution"; private static final String PARAM_HOTSPOTS = "hotspots"; + private static final String PARAM_BRANCH = "branch"; + private static final String PARAM_PULL_REQUEST = "pullRequest"; private final DbClient dbClient; private final UserSession userSession; @@ -113,6 +117,14 @@ public class SearchAction implements HotspotsWsAction { "Key of the project. This parameter is required unless %s is provided.", PARAM_HOTSPOTS)) .setExampleValue(KEY_PROJECT_EXAMPLE_001); + action.createParam(PARAM_BRANCH) + .setDescription("Branch key") + .setExampleValue(KEY_BRANCH_EXAMPLE_001) + .setInternal(true); + action.createParam(PARAM_PULL_REQUEST) + .setDescription("Pull request id") + .setExampleValue(KEY_PULL_REQUEST_EXAMPLE_001) + .setInternal(true); action.createParam(PARAM_HOTSPOTS) .setDescription(format( "Comma-separated list of Security Hotspot keys. This parameter is required unless %s is provided.", @@ -151,38 +163,64 @@ public class SearchAction implements HotspotsWsAction { Set hotspotKeys = hotspotKeysList == null ? ImmutableSet.of() : hotspotKeysList.stream().collect(MoreCollectors.toSet(hotspotKeysList.size())); return new WsRequest( request.mandatoryParamAsInt(PAGE), request.mandatoryParamAsInt(PAGE_SIZE), - request.param(PARAM_PROJECT_KEY), hotspotKeys, + request.param(PARAM_PROJECT_KEY), request.param(PARAM_BRANCH), request.param(PARAM_PULL_REQUEST), + hotspotKeys, request.param(PARAM_STATUS), request.param(PARAM_RESOLUTION)); } private static void validateParameters(WsRequest wsRequest) { + Optional projectKey = wsRequest.getProjectKey(); + Optional branch = wsRequest.getBranch(); + Optional pullRequest = wsRequest.getPullRequest(); + Set hotspotKeys = wsRequest.getHotspotKeys(); + checkArgument( - wsRequest.getProjectKey().isPresent() || !wsRequest.getHotspotKeys().isEmpty(), + projectKey.isPresent() || !hotspotKeys.isEmpty(), "A value must be provided for either parameter '%s' or parameter '%s'", PARAM_PROJECT_KEY, PARAM_HOTSPOTS); - checkArgument(!wsRequest.getStatus().isPresent() || wsRequest.getHotspotKeys().isEmpty(), + checkArgument( + !branch.isPresent() || projectKey.isPresent(), + "Parameter '%s' must be used with parameter '%s'", PARAM_BRANCH, PARAM_PROJECT_KEY); + checkArgument( + !pullRequest.isPresent() || projectKey.isPresent(), + "Parameter '%s' must be used with parameter '%s'", PARAM_PULL_REQUEST, PARAM_PROJECT_KEY); + checkArgument( + !(branch.isPresent() && pullRequest.isPresent()), + "Only one of parameters '%s' and '%s' can be provided", PARAM_BRANCH, PARAM_PULL_REQUEST); + + Optional status = wsRequest.getStatus(); + Optional resolution = wsRequest.getResolution(); + checkArgument(!status.isPresent() || hotspotKeys.isEmpty(), "Parameter '%s' can't be used with parameter '%s'", PARAM_STATUS, PARAM_HOTSPOTS); - checkArgument(!wsRequest.getResolution().isPresent() || wsRequest.getHotspotKeys().isEmpty(), + checkArgument(!resolution.isPresent() || hotspotKeys.isEmpty(), "Parameter '%s' can't be used with parameter '%s'", PARAM_RESOLUTION, PARAM_HOTSPOTS); - wsRequest.getResolution().ifPresent( - resolution -> checkArgument(wsRequest.getStatus().filter(STATUS_REVIEWED::equals).isPresent(), + resolution.ifPresent( + r -> checkArgument(status.filter(STATUS_REVIEWED::equals).isPresent(), "Value '%s' of parameter '%s' can only be provided if value of parameter '%s' is '%s'", - resolution, PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED)); + r, PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED)); } private Optional getAndValidateProject(DbSession dbSession, WsRequest wsRequest) { return wsRequest.getProjectKey().map(projectKey -> { - ComponentDto project = dbClient.componentDao().selectByKey(dbSession, projectKey) + ComponentDto project = getProject(dbSession, projectKey, wsRequest.getBranch(), wsRequest.getPullRequest()) .filter(t -> Scopes.PROJECT.equals(t.scope()) && Qualifiers.PROJECT.equals(t.qualifier())) .filter(ComponentDto::isEnabled) - .filter(t -> t.getMainBranchProjectUuid() == null) .orElseThrow(() -> new NotFoundException(format("Project '%s' not found", projectKey))); userSession.checkComponentPermission(UserRole.USER, project); return project; }); } + private Optional getProject(DbSession dbSession, String projectKey, Optional branch, Optional pullRequest) { + if (branch.isPresent()) { + return dbClient.componentDao().selectByKeyAndBranch(dbSession, projectKey, branch.get()); + } else if (pullRequest.isPresent()) { + return dbClient.componentDao().selectByKeyAndPullRequest(dbSession, projectKey, pullRequest.get()); + } + return dbClient.componentDao().selectByKey(dbSession, projectKey); + } + private SearchResponseData searchHotspots(WsRequest wsRequest, DbSession dbSession, Optional project, Set hotspotKeys) { SearchResponse result = doIndexSearch(wsRequest, project, hotspotKeys); List issueKeys = Arrays.stream(result.getHits().getHits()) @@ -213,8 +251,15 @@ public class SearchAction implements HotspotsWsAction { .sort(IssueQuery.SORT_HOTSPOTS) .asc(true); project.ifPresent(p -> { - builder.projectUuids(Collections.singletonList(p.uuid())); builder.organizationUuid(p.getOrganizationUuid()); + + if (p.getMainBranchProjectUuid() == null) { + builder.mainBranch(true); + builder.projectUuids(singletonList(p.uuid())); + } else { + builder.branchUuid(p.projectUuid()); + builder.mainBranch(false); + } }); if (!hotspotKeys.isEmpty()) { builder.issueKeys(hotspotKeys); @@ -317,14 +362,21 @@ public class SearchAction implements HotspotsWsAction { private final int page; private final int index; private final String projectKey; + private final String branch; + private final String pullRequest; private final Set hotspotKeys; private final String status; private final String resolution; - private WsRequest(int page, int index, @Nullable String projectKey, Set hotspotKeys, @Nullable String status, @Nullable String resolution) { + private WsRequest(int page, int index, + @Nullable String projectKey, @Nullable String branch, @Nullable String pullRequest, + Set hotspotKeys, + @Nullable String status, @Nullable String resolution) { this.page = page; this.index = index; this.projectKey = projectKey; + this.branch = branch; + this.pullRequest = pullRequest; this.hotspotKeys = hotspotKeys; this.status = status; this.resolution = resolution; @@ -342,6 +394,14 @@ public class SearchAction implements HotspotsWsAction { return ofNullable(projectKey); } + Optional getBranch() { + return ofNullable(branch); + } + + Optional getPullRequest() { + return ofNullable(pullRequest); + } + Set getHotspotKeys() { return hotspotKeys; } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/SearchActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/SearchActionTest.java index c893a97923f..30193c11263 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/SearchActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/SearchActionTest.java @@ -45,6 +45,7 @@ import org.sonar.api.utils.System2; import org.sonar.api.web.UserRole; import org.sonar.db.DbClient; import org.sonar.db.DbTester; +import org.sonar.db.component.BranchType; import org.sonar.db.component.ComponentDto; import org.sonar.db.component.ComponentTesting; import org.sonar.db.issue.IssueDto; @@ -123,6 +124,40 @@ public class SearchActionTest { .hasMessage("A value must be provided for either parameter 'projectKey' or parameter 'hotspots'"); } + @Test + public void fail_with_IAE_if_parameter_branch_is_used_without_parameter_projectKey() { + TestRequest request = actionTester.newRequest() + .setParam("hotspots", randomAlphabetic(2)) + .setParam("branch", randomAlphabetic(1)); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'branch' must be used with parameter 'projectKey'"); + } + + @Test + public void fail_with_IAE_if_parameter_pullRequest_is_used_without_parameter_projectKey() { + TestRequest request = actionTester.newRequest() + .setParam("hotspots", randomAlphabetic(2)) + .setParam("pullRequest", randomAlphabetic(1)); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'pullRequest' must be used with parameter 'projectKey'"); + } + + @Test + public void fail_with_IAE_if_both_parameters_pullRequest_and_branch_are_provided() { + TestRequest request = actionTester.newRequest() + .setParam("projectKey", randomAlphabetic(2)) + .setParam("branch", randomAlphabetic(1)) + .setParam("pullRequest", randomAlphabetic(1)); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Only one of parameters 'branch' and 'pullRequest' can be provided"); + } + @Test @UseDataProvider("badStatuses") public void fails_with_IAE_if_status_parameter_is_neither_TO_REVIEW_or_REVIEWED(String badStatus) { @@ -325,6 +360,8 @@ public class SearchActionTest { SearchWsResponse response = newRequest(project) .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()).isEmpty(); } @Test @@ -421,6 +458,54 @@ public class SearchActionTest { .containsOnly(project2.getKey(), file2.getKey()); } + @Test + public void returns_hotspot_of_branch_or_pullRequest() { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + indexPermissions(); + ComponentDto branch = dbTester.components().insertProjectBranch(project); + ComponentDto pullRequest = dbTester.components().insertProjectBranch(project, t -> t.setBranchType(BranchType.PULL_REQUEST)); + ComponentDto fileProject = dbTester.components().insertComponent(newFileDto(project)); + ComponentDto fileBranch = dbTester.components().insertComponent(newFileDto(branch)); + ComponentDto filePR = dbTester.components().insertComponent(newFileDto(pullRequest)); + IssueDto[] hotspotProject = IntStream.range(0, 1 + RANDOM.nextInt(10)) + .mapToObj(i -> { + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + return insertHotspot(project, fileProject, rule); + }) + .toArray(IssueDto[]::new); + IssueDto[] hotspotBranch = IntStream.range(0, 1 + RANDOM.nextInt(10)) + .mapToObj(i -> { + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + return insertHotspot(branch, fileBranch, rule); + }) + .toArray(IssueDto[]::new); + IssueDto[] hotspotPR = IntStream.range(0, 1 + RANDOM.nextInt(10)) + .mapToObj(i -> { + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + return insertHotspot(pullRequest, filePR, rule); + }) + .toArray(IssueDto[]::new); + indexIssues(); + + SearchWsResponse responseProject = newRequest(project) + .executeProtobuf(SearchWsResponse.class); + SearchWsResponse responseBranch = newRequest(branch) + .executeProtobuf(SearchWsResponse.class); + SearchWsResponse responsePR = newRequest(pullRequest) + .executeProtobuf(SearchWsResponse.class); + + assertThat(responseProject.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsExactlyInAnyOrder(Arrays.stream(hotspotProject).map(IssueDto::getKey).toArray(String[]::new)); + assertThat(responseBranch.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsExactlyInAnyOrder(Arrays.stream(hotspotBranch).map(IssueDto::getKey).toArray(String[]::new)); + assertThat(responsePR.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsExactlyInAnyOrder(Arrays.stream(hotspotPR).map(IssueDto::getKey).toArray(String[]::new)); + } + @Test public void returns_hotpots_with_any_status_if_no_status_nor_resolution_parameter() { ComponentDto project = dbTester.components().insertPublicProject(); @@ -687,16 +772,92 @@ public class SearchActionTest { assertThat(actualProject.getName()).isEqualTo(project.name()); assertThat(actualProject.getLongName()).isEqualTo(project.longName()); assertThat(actualProject.hasPath()).isFalse(); + assertThat(actualProject.hasBranch()).isFalse(); + assertThat(actualProject.hasPullRequest()).isFalse(); Component actualDirectory = componentByKey.get(directory.getKey()); assertThat(actualDirectory.getQualifier()).isEqualTo(directory.qualifier()); assertThat(actualDirectory.getName()).isEqualTo(directory.name()); assertThat(actualDirectory.getLongName()).isEqualTo(directory.longName()); assertThat(actualDirectory.getPath()).isEqualTo(directory.path()); + assertThat(actualDirectory.hasBranch()).isFalse(); + assertThat(actualDirectory.hasPullRequest()).isFalse(); Component actualFile = componentByKey.get(file.getKey()); assertThat(actualFile.getQualifier()).isEqualTo(file.qualifier()); assertThat(actualFile.getName()).isEqualTo(file.name()); assertThat(actualFile.getLongName()).isEqualTo(file.longName()); assertThat(actualFile.getPath()).isEqualTo(file.path()); + assertThat(actualFile.hasBranch()).isFalse(); + assertThat(actualFile.hasPullRequest()).isFalse(); + } + + @Test + public void returns_branch_field_of_components_of_branch() { + ComponentDto project = dbTester.components().insertPublicProject(); + ComponentDto branch = dbTester.components().insertProjectBranch(project); + userSessionRule.registerComponents(project, branch); + indexPermissions(); + ComponentDto directory = dbTester.components().insertComponent(newDirectory(branch, "donut/acme")); + ComponentDto file = dbTester.components().insertComponent(newFileDto(branch)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + IssueDto fileHotspot = insertHotspot(branch, file, rule); + IssueDto dirHotspot = insertHotspot(branch, directory, rule); + IssueDto projectHotspot = insertHotspot(branch, branch, rule); + indexIssues(); + + SearchWsResponse response = newRequest(branch) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsOnly(fileHotspot.getKey(), dirHotspot.getKey(), projectHotspot.getKey()); + assertThat(response.getComponentsList()) + .extracting(Component::getKey) + .containsOnly(project.getKey(), directory.getKey(), file.getKey()); + Map componentByKey = response.getComponentsList().stream().collect(uniqueIndex(Component::getKey)); + Component actualProject = componentByKey.get(project.getKey()); + assertThat(actualProject.getBranch()).isEqualTo(branch.getBranch()); + assertThat(actualProject.hasPullRequest()).isFalse(); + Component actualDirectory = componentByKey.get(directory.getKey()); + assertThat(actualDirectory.getBranch()).isEqualTo(branch.getBranch()); + assertThat(actualDirectory.hasPullRequest()).isFalse(); + Component actualFile = componentByKey.get(file.getKey()); + assertThat(actualFile.getBranch()).isEqualTo(branch.getBranch()); + assertThat(actualFile.hasPullRequest()).isFalse(); + } + + @Test + public void returns_pullRequest_field_of_components_of_pullRequest() { + ComponentDto project = dbTester.components().insertPublicProject(); + ComponentDto pullRequest = dbTester.components().insertProjectBranch(project, t -> t.setBranchType(BranchType.PULL_REQUEST)); + userSessionRule.registerComponents(project, pullRequest); + indexPermissions(); + ComponentDto directory = dbTester.components().insertComponent(newDirectory(pullRequest, "donut/acme")); + ComponentDto file = dbTester.components().insertComponent(newFileDto(pullRequest)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + IssueDto fileHotspot = insertHotspot(pullRequest, file, rule); + IssueDto dirHotspot = insertHotspot(pullRequest, directory, rule); + IssueDto projectHotspot = insertHotspot(pullRequest, pullRequest, rule); + indexIssues(); + + SearchWsResponse response = newRequest(pullRequest) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsOnly(fileHotspot.getKey(), dirHotspot.getKey(), projectHotspot.getKey()); + assertThat(response.getComponentsList()) + .extracting(Component::getKey) + .containsOnly(project.getKey(), directory.getKey(), file.getKey()); + Map componentByKey = response.getComponentsList().stream().collect(uniqueIndex(Component::getKey)); + Component actualProject = componentByKey.get(project.getKey()); + assertThat(actualProject.hasBranch()).isFalse(); + assertThat(actualProject.getPullRequest()).isEqualTo(pullRequest.getPullRequest()); + Component actualDirectory = componentByKey.get(directory.getKey()); + assertThat(actualDirectory.hasBranch()).isFalse(); + assertThat(actualDirectory.getPullRequest()).isEqualTo(pullRequest.getPullRequest()); + Component actualFile = componentByKey.get(file.getKey()); + assertThat(actualFile.hasBranch()).isFalse(); + assertThat(actualFile.getPullRequest()).isEqualTo(pullRequest.getPullRequest()); } @Test @@ -918,6 +1079,14 @@ public class SearchActionTest { private TestRequest newRequest(ComponentDto project, @Nullable String status, @Nullable String resolution) { TestRequest res = actionTester.newRequest() .setParam("projectKey", project.getKey()); + String branch = project.getBranch(); + if (branch != null) { + res.setParam("branch", branch); + } + String pullRequest = project.getPullRequest(); + if (pullRequest != null) { + res.setParam("pullRequest", pullRequest); + } if (status != null) { res.setParam("status", status); } -- 2.39.5