From 545e6dd086f886f6ab178eeb8c5103ba4b74d4f1 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Wed, 11 Dec 2019 17:26:07 +0100 Subject: [PATCH] SONAR-12797 add hotspots parameter to api/hotspots/search --- .../sonar/server/hotspot/ws/SearchAction.java | 91 +++++++++---- .../server/hotspot/ws/SearchActionTest.java | 122 ++++++++++++++---- 2 files changed, 163 insertions(+), 50 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 88456006569..7ccfed8c524 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 @@ -19,6 +19,7 @@ */ package org.sonar.server.hotspot.ws; +import com.google.common.collect.ImmutableSet; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -43,6 +44,7 @@ import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.Paging; import org.sonar.api.web.UserRole; +import org.sonar.core.util.stream.MoreCollectors; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; @@ -81,6 +83,7 @@ public class SearchAction implements HotspotsWsAction { private static final String PARAM_PROJECT_KEY = "projectKey"; private static final String PARAM_STATUS = "status"; private static final String PARAM_RESOLUTION = "resolution"; + private static final String PARAM_HOTSPOTS = "hotspots"; private final DbClient dbClient; private final UserSession userSession; @@ -106,17 +109,23 @@ public class SearchAction implements HotspotsWsAction { action.addPagingParams(100); action.createParam(PARAM_PROJECT_KEY) - .setDescription("Key of the project") - .setExampleValue(KEY_PROJECT_EXAMPLE_001) - .setRequired(true); + .setDescription(format( + "Key of the project. This parameter is required unless %s is provided.", + PARAM_HOTSPOTS)) + .setExampleValue(KEY_PROJECT_EXAMPLE_001); + action.createParam(PARAM_HOTSPOTS) + .setDescription(format( + "Comma-separated list of Security Hotspot keys. This parameter is required unless %s is provided.", + PARAM_PROJECT_KEY)) + .setExampleValue(KEY_PROJECT_EXAMPLE_001); action.createParam(PARAM_STATUS) - .setDescription("If provided, only Security Hotspots with the specified status are returned.") + .setDescription("If '%s' is provided, only Security Hotspots with the specified status are returned.", PARAM_PROJECT_KEY) .setPossibleValues(STATUS_TO_REVIEW, STATUS_REVIEWED) .setRequired(false); action.createParam(PARAM_RESOLUTION) .setDescription(format( - "If provided and if status is '%s', only Security Hotspots with the specified resolution are returned.", - STATUS_REVIEWED)) + "If '%s' is provided and if status is '%s', only Security Hotspots with the specified resolution are returned.", + PARAM_PROJECT_KEY, STATUS_REVIEWED)) .setPossibleValues(RESOLUTION_FIXED, RESOLUTION_SAFE) .setRequired(false); // FIXME add response example and test it @@ -126,10 +135,11 @@ public class SearchAction implements HotspotsWsAction { @Override public void handle(Request request, Response response) throws Exception { WsRequest wsRequest = toWsRequest(request); + validateParameters(wsRequest); try (DbSession dbSession = dbClient.openSession(false)) { - ComponentDto project = validateRequest(dbSession, wsRequest); + Optional project = getAndValidateProject(dbSession, wsRequest); - SearchResponseData searchResponseData = searchHotspots(wsRequest, dbSession, project); + SearchResponseData searchResponseData = searchHotspots(wsRequest, dbSession, project, wsRequest.getHotspotKeys()); loadComponents(dbSession, searchResponseData); loadRules(dbSession, searchResponseData); writeProtobuf(formatResponse(searchResponseData), request, response); @@ -137,30 +147,44 @@ public class SearchAction implements HotspotsWsAction { } private static WsRequest toWsRequest(Request request) { + List hotspotKeysList = request.paramAsStrings(PARAM_HOTSPOTS); + Set hotspotKeys = hotspotKeysList == null ? ImmutableSet.of() : hotspotKeysList.stream().collect(MoreCollectors.toSet(hotspotKeysList.size())); return new WsRequest( request.mandatoryParamAsInt(PAGE), request.mandatoryParamAsInt(PAGE_SIZE), - request.mandatoryParam(PARAM_PROJECT_KEY), + request.param(PARAM_PROJECT_KEY), hotspotKeys, request.param(PARAM_STATUS), request.param(PARAM_RESOLUTION)); } - private ComponentDto validateRequest(DbSession dbSession, WsRequest wsRequest) { + private static void validateParameters(WsRequest wsRequest) { + checkArgument( + wsRequest.getProjectKey().isPresent() || !wsRequest.getHotspotKeys().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(), + "Parameter '%s' can't be used with parameter '%s'", PARAM_STATUS, PARAM_HOTSPOTS); + checkArgument(!wsRequest.getResolution().isPresent() || wsRequest.getHotspotKeys().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(), "Value '%s' of parameter '%s' can only be provided if value of parameter '%s' is '%s'", - resolution, PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED) - ); - - ComponentDto project = dbClient.componentDao().selectByKey(dbSession, wsRequest.getProjectKey()) - .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", wsRequest.getProjectKey()))); - userSession.checkComponentPermission(UserRole.USER, project); - return project; + resolution, PARAM_RESOLUTION, PARAM_STATUS, STATUS_REVIEWED)); } - private SearchResponseData searchHotspots(WsRequest wsRequest, DbSession dbSession, ComponentDto project) { - SearchResponse result = doIndexSearch(wsRequest, project); + private Optional getAndValidateProject(DbSession dbSession, WsRequest wsRequest) { + return wsRequest.getProjectKey().map(projectKey -> { + ComponentDto project = dbClient.componentDao().selectByKey(dbSession, projectKey) + .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 SearchResponseData searchHotspots(WsRequest wsRequest, DbSession dbSession, Optional project, Set hotspotKeys) { + SearchResponse result = doIndexSearch(wsRequest, project, hotspotKeys); List issueKeys = Arrays.stream(result.getHits().getHits()) .map(SearchHit::getId) .collect(toList(result.getHits().getHits().length)); @@ -183,13 +207,18 @@ public class SearchAction implements HotspotsWsAction { .collect(Collectors.toList()); } - private SearchResponse doIndexSearch(WsRequest wsRequest, ComponentDto project) { + private SearchResponse doIndexSearch(WsRequest wsRequest, Optional project, Set hotspotKeys) { IssueQuery.Builder builder = IssueQuery.builder() - .projectUuids(Collections.singletonList(project.uuid())) - .organizationUuid(project.getOrganizationUuid()) .types(singleton(RuleType.SECURITY_HOTSPOT.name())) .sort(IssueQuery.SORT_HOTSPOTS) .asc(true); + project.ifPresent(p -> { + builder.projectUuids(Collections.singletonList(p.uuid())); + builder.organizationUuid(p.getOrganizationUuid()); + }); + if (!hotspotKeys.isEmpty()) { + builder.issueKeys(hotspotKeys); + } wsRequest.getStatus().ifPresent(status -> builder.resolved(STATUS_REVIEWED.equals(status))); wsRequest.getResolution().ifPresent(resolution -> builder.resolutions(singleton(resolution))); @@ -288,13 +317,15 @@ public class SearchAction implements HotspotsWsAction { private final int page; private final int index; private final String projectKey; + private final Set hotspotKeys; private final String status; private final String resolution; - private WsRequest(int page, int index, String projectKey, @Nullable String status, @Nullable String resolution) { + private WsRequest(int page, int index, @Nullable String projectKey, Set hotspotKeys, @Nullable String status, @Nullable String resolution) { this.page = page; this.index = index; this.projectKey = projectKey; + this.hotspotKeys = hotspotKeys; this.status = status; this.resolution = resolution; } @@ -307,8 +338,12 @@ public class SearchAction implements HotspotsWsAction { return index; } - String getProjectKey() { - return projectKey; + Optional getProjectKey() { + return ofNullable(projectKey); + } + + Set getHotspotKeys() { + return hotspotKeys; } Optional getStatus() { 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 e0cf3bd3611..c893a97923f 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 @@ -25,6 +25,7 @@ import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -32,7 +33,6 @@ import java.util.Map; import java.util.Random; import java.util.Set; import java.util.function.Consumer; -import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -70,6 +70,8 @@ import org.sonarqube.ws.Hotspots.Component; import org.sonarqube.ws.Hotspots.SearchWsResponse; import static java.util.Collections.singleton; +import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; @@ -113,12 +115,12 @@ public class SearchActionTest { } @Test - public void fails_with_IAE_if_parameter_projectKey_is_missing() { + public void fails_with_IAE_if_parameters_projectKey_and_hotspots_are_missing() { TestRequest request = actionTester.newRequest(); assertThatThrownBy(request::execute) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("The 'projectKey' parameter is missing"); + .hasMessage("A value must be provided for either parameter 'projectKey' or parameter 'hotspots'"); } @Test @@ -144,6 +146,18 @@ public class SearchActionTest { .toArray(Object[][]::new); } + @Test + @UseDataProvider("validStatusesAndResolutions") + public void fail_with_IAE_if_parameter_status_is_specified_with_hotspots_parameter(String status, @Nullable String notUsed) { + TestRequest request = actionTester.newRequest() + .setParam("hotspots", randomAlphabetic(12)) + .setParam("status", status); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'status' can't be used with parameter 'hotspots'"); + } + @Test @UseDataProvider("badResolutions") public void fails_with_IAE_if_resolution_parameter_is_neither_FIXED_nor_SAFE(String badResolution) { @@ -183,6 +197,18 @@ public class SearchActionTest { .hasMessage("Value '" + resolution + "' of parameter 'resolution' can only be provided if value of parameter 'status' is 'REVIEWED'"); } + @Test + @UseDataProvider("fixedOrSafeResolution") + public void fails_with_IAE_if_resolution_is_provided_with_hotspots_parameter(String resolution) { + TestRequest request = actionTester.newRequest() + .setParam("hotspots", randomAlphabetic(13)) + .setParam("resolution", resolution); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'resolution' can't be used with parameter 'hotspots'"); + } + @DataProvider public static Object[][] fixedOrSafeResolution() { return new Object[][] { @@ -265,7 +291,7 @@ public class SearchActionTest { IssueDto[] hotspots = IntStream.range(0, 1 + RANDOM.nextInt(10)) .mapToObj(i -> { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); - return dbTester.issues().insert(rule, project, file, t -> t.setType(SECURITY_HOTSPOT)); + return insertHotspot(project, file, rule); }) .toArray(IssueDto[]::new); indexIssues(); @@ -317,7 +343,7 @@ public class SearchActionTest { IssueDto[] hotspots = IntStream.range(0, 1 + RANDOM.nextInt(10)) .mapToObj(i -> { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); - return dbTester.issues().insert(rule, project, fileWithHotspot, t -> t.setType(SECURITY_HOTSPOT)); + return insertHotspot(project, fileWithHotspot, rule); }) .toArray(IssueDto[]::new); indexIssues(); @@ -341,7 +367,7 @@ public class SearchActionTest { IssueDto[] hotspots = IntStream.range(0, 1 + RANDOM.nextInt(10)) .mapToObj(i -> { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); - return dbTester.issues().insert(rule, project, project, t -> t.setType(SECURITY_HOTSPOT)); + return insertHotspot(project, project, rule); }) .toArray(IssueDto[]::new); indexIssues(); @@ -368,8 +394,8 @@ public class SearchActionTest { IssueDto[] hotspots2 = IntStream.range(0, 1 + RANDOM.nextInt(10)) .mapToObj(i -> { RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); - dbTester.issues().insert(rule, project1, file1, t -> t.setType(SECURITY_HOTSPOT)); - return dbTester.issues().insert(rule, project2, file2, t -> t.setType(SECURITY_HOTSPOT)); + insertHotspot(project1, file1, rule); + return insertHotspot(project2, file2, rule); }) .toArray(IssueDto[]::new); indexIssues(); @@ -379,7 +405,7 @@ public class SearchActionTest { assertThat(responseProject1.getHotspotsList()) .extracting(SearchWsResponse.Hotspot::getKey) - .doesNotContainAnyElementsOf(Arrays.stream(hotspots2).map(IssueDto::getKey).collect(Collectors.toList())); + .doesNotContainAnyElementsOf(Arrays.stream(hotspots2).map(IssueDto::getKey).collect(toList())); assertThat(responseProject1.getComponentsList()) .extracting(Component::getKey) .containsOnly(project1.getKey(), file1.getKey()); @@ -402,7 +428,7 @@ public class SearchActionTest { indexPermissions(); ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); List hotspots = insertRandomNumberOfHotspotsOfAllSupportedStatusesAndResolutions(project, file) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); SearchWsResponse response = newRequest(project, null, null) @@ -421,7 +447,7 @@ public class SearchActionTest { ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); List reviewedHotspots = insertRandomNumberOfHotspotsOfAllSupportedStatusesAndResolutions(project, file) .filter(t -> STATUS_REVIEWED.equals(t.getStatus())) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); SearchWsResponse response = newRequest(project, STATUS_REVIEWED, null) @@ -440,7 +466,7 @@ public class SearchActionTest { ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); List safeHotspots = insertRandomNumberOfHotspotsOfAllSupportedStatusesAndResolutions(project, file) .filter(t -> STATUS_REVIEWED.equals(t.getStatus()) && RESOLUTION_SAFE.equals(t.getResolution())) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); SearchWsResponse response = newRequest(project, STATUS_REVIEWED, RESOLUTION_SAFE) @@ -459,7 +485,7 @@ public class SearchActionTest { ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); List fixedHotspots = insertRandomNumberOfHotspotsOfAllSupportedStatusesAndResolutions(project, file) .filter(t -> STATUS_REVIEWED.equals(t.getStatus()) && RESOLUTION_FIXED.equals(t.getResolution())) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); SearchWsResponse response = newRequest(project, STATUS_REVIEWED, RESOLUTION_FIXED) @@ -507,7 +533,7 @@ public class SearchActionTest { .setStatus(status) .setResolution(resolution)); }) - .collect(Collectors.toList()); + .collect(toList()); Collections.shuffle(hotspots); hotspots.forEach(t -> dbTester.issues().insertIssue(t)); return hotspots.stream(); @@ -570,7 +596,7 @@ public class SearchActionTest { indexPermissions(); ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT, t -> t.setSecurityStandards(securityStandards)); - IssueDto hotspot = dbTester.issues().insert(rule, project, file, t -> t.setType(SECURITY_HOTSPOT)); + IssueDto hotspot = insertHotspot(project, file, rule); indexIssues(); SearchWsResponse response = newRequest(project) @@ -637,9 +663,9 @@ public class SearchActionTest { ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); ComponentDto file2 = dbTester.components().insertComponent(newFileDto(project)); RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); - IssueDto fileHotspot = dbTester.issues().insert(rule, project, file, t -> t.setType(SECURITY_HOTSPOT)); - IssueDto dirHotspot = dbTester.issues().insert(rule, project, directory, t -> t.setType(SECURITY_HOTSPOT)); - IssueDto projectHotspot = dbTester.issues().insert(rule, project, project, t -> t.setType(SECURITY_HOTSPOT)); + IssueDto fileHotspot = insertHotspot(project, file, rule); + IssueDto dirHotspot = insertHotspot(project, directory, rule); + IssueDto projectHotspot = insertHotspot(project, project, rule); indexIssues(); SearchWsResponse response = newRequest(project) @@ -695,7 +721,7 @@ public class SearchActionTest { newIssue(rule1, project, file).setKee(sqCategory + "_a").setType(SECURITY_HOTSPOT), newIssue(rule2, project, file).setKee(sqCategory + "_b").setType(SECURITY_HOTSPOT)); }) - .collect(Collectors.toList()); + .collect(toList()); String[] expectedHotspotKeys = hotspots.stream().map(IssueDto::getKey).toArray(String[]::new); // insert hotspots in random order Collections.shuffle(hotspots); @@ -728,7 +754,7 @@ public class SearchActionTest { newIssue(rule, project, file1).setType(SECURITY_HOTSPOT).setLine(11).setKee("b"), newIssue(rule, project, file2).setType(SECURITY_HOTSPOT).setLine(null), newIssue(rule, project, file2).setType(SECURITY_HOTSPOT).setLine(2)) - .collect(Collectors.toList()); + .collect(toList()); String[] expectedHotspotKeys = hotspots.stream().map(IssueDto::getKey).toArray(String[]::new); // insert hotspots in random order Collections.shuffle(hotspots); @@ -754,7 +780,7 @@ public class SearchActionTest { List hotspots = IntStream.range(0, total) .mapToObj(i -> newIssue(rule, project, file).setType(SECURITY_HOTSPOT).setLine(i)) .map(i -> dbTester.issues().insertIssue(i)) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); TestRequest request = newRequest(project); @@ -797,7 +823,7 @@ public class SearchActionTest { List hotspots = IntStream.range(0, total) .mapToObj(i -> newIssue(rule, project, file).setType(SECURITY_HOTSPOT).setLine(i).setKee("issue_" + i)) .map(i -> dbTester.issues().insertIssue(i)) - .collect(Collectors.toList()); + .collect(toList()); indexIssues(); SearchWsResponse response = newRequest(project) @@ -838,6 +864,53 @@ public class SearchActionTest { assertThat(response.getPaging().getPageSize()).isEqualTo(pageSize); } + @Test + public void returns_empty_if_none_of_hotspot_keys_exist() { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + indexPermissions(); + ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + List hotspots = IntStream.range(0, 1 + RANDOM.nextInt(15)) + .mapToObj(i -> newIssue(rule, project, file).setType(SECURITY_HOTSPOT).setLine(i)) + .map(i -> dbTester.issues().insertIssue(i)) + .collect(toList()); + indexIssues(); + + SearchWsResponse response = newRequest(IntStream.range(0, 1 + RANDOM.nextInt(30)).mapToObj(i -> "key_" + i).collect(toList())) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()).isEmpty(); + } + + @Test + public void returns_specified_hotspots() { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + indexPermissions(); + ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); + RuleDefinitionDto rule = newRule(SECURITY_HOTSPOT); + int total = 1 + RANDOM.nextInt(20); + List hotspots = IntStream.range(0, total) + .mapToObj(i -> newIssue(rule, project, file).setType(SECURITY_HOTSPOT).setLine(i)) + .map(i -> dbTester.issues().insertIssue(i)) + .collect(toList()); + Collections.shuffle(hotspots); + List selectedHotspots = hotspots.stream().limit(total == 1 ? 1 : 1 + RANDOM.nextInt(total - 1)).collect(toList()); + indexIssues(); + + SearchWsResponse response = newRequest(selectedHotspots.stream().map(IssueDto::getKey).collect(toList())) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()) + .extracting(SearchWsResponse.Hotspot::getKey) + .containsExactlyInAnyOrder(selectedHotspots.stream().map(IssueDto::getKey).toArray(String[]::new)); + } + + private IssueDto insertHotspot(ComponentDto project, ComponentDto file, RuleDefinitionDto rule) { + return dbTester.issues().insert(rule, project, file, t -> t.setType(SECURITY_HOTSPOT)); + } + private TestRequest newRequest(ComponentDto project) { return newRequest(project, null, null); } @@ -854,6 +927,11 @@ public class SearchActionTest { return res; } + private TestRequest newRequest(Collection hotspotKeys) { + return actionTester.newRequest() + .setParam("hotspots", hotspotKeys.stream().collect(joining(","))); + } + private void indexPermissions() { permissionIndexer.indexOnStartup(permissionIndexer.getIndexTypes()); } -- 2.39.5