From 7588d0515996e11f5456c59f4e45794c8bc1a1f6 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Wed, 4 Dec 2019 14:33:58 +0100 Subject: [PATCH] SONAR-12717 drop rule list from api/hotspots/search response --- .../sonar/server/hotspot/ws/SearchAction.java | 45 +++---- .../server/hotspot/ws/SearchActionTest.java | 110 +++++++++++++++--- sonar-ws/src/main/protobuf/ws-hotspots.proto | 28 ++--- 3 files changed, 119 insertions(+), 64 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 3e38031cafc..021901007cf 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,10 +22,12 @@ package org.sonar.server.hotspot.ws; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -40,7 +42,6 @@ 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; @@ -62,7 +63,9 @@ import static org.sonar.api.server.ws.WebService.Param.PAGE; import static org.sonar.api.server.ws.WebService.Param.PAGE_SIZE; import static org.sonar.api.utils.DateUtils.formatDateTime; 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_PROJECT_EXAMPLE_001; import static org.sonar.server.ws.WsUtils.writeProtobuf; @@ -130,7 +133,7 @@ public class SearchAction implements HotspotsWsAction { SearchResponse result = doIndexSearch(wsRequest, project); List issueKeys = Arrays.stream(result.getHits().getHits()) .map(SearchHit::getId) - .collect(MoreCollectors.toList(result.getHits().getHits().length)); + .collect(toList(result.getHits().getHits().length)); List hotspots = toIssueDtos(dbSession, issueKeys); @@ -190,7 +193,6 @@ public class SearchAction implements HotspotsWsAction { if (!searchResponseData.isEmpty()) { formatHotspots(searchResponseData, responseBuilder); formatComponents(searchResponseData, responseBuilder); - formatRules(searchResponseData, responseBuilder); } return responseBuilder.build(); } @@ -213,12 +215,18 @@ public class SearchAction implements HotspotsWsAction { Hotspots.Hotspot.Builder builder = Hotspots.Hotspot.newBuilder(); for (IssueDto hotspot : orderedHotspots) { + RuleDefinitionDto rule = searchResponseData.getRule(hotspot.getRuleKey()) + // due to join with table Rule when retrieving data from Issues, this can't happen + .orElseThrow(() -> new IllegalStateException(String.format( + "Rule with key '%s' not found for Hotspot '%s'", hotspot.getRuleKey(), hotspot.getKey()))); + SecurityStandards.SQCategory sqCategory = fromSecurityStandards(rule.getSecurityStandards()).getSqCategory(); builder .clear() .setKey(hotspot.getKey()) .setComponent(hotspot.getComponentKey()) .setProject(hotspot.getProjectKey()) - .setRule(hotspot.getRuleKey().toString()); + .setSecurityCategory(sqCategory.getKey()) + .setVulnerabilityProbability(sqCategory.getVulnerability().name()); ofNullable(hotspot.getStatus()).ifPresent(builder::setStatus); // FIXME resolution field will be added later // ofNullable(hotspot.getResolution()).ifPresent(builder::setResolution); @@ -257,27 +265,6 @@ public class SearchAction implements HotspotsWsAction { } } - private void formatRules(SearchResponseData searchResponseData, Hotspots.SearchWsResponse.Builder responseBuilder) { - Set rules = searchResponseData.getRules(); - if (rules.isEmpty()) { - return; - } - - Hotspots.Rule.Builder ruleBuilder = Hotspots.Rule.newBuilder(); - for (RuleDefinitionDto rule : rules) { - SecurityStandards securityStandards = SecurityStandards.fromSecurityStandards(rule.getSecurityStandards()); - SecurityStandards.SQCategory sqCategory = securityStandards.getSqCategory(); - ruleBuilder - .clear() - .setKey(rule.getKey().toString()) - .setName(nullToEmpty(rule.getName())) - .setSecurityCategory(sqCategory.getKey()) - .setVulnerabilityProbability(sqCategory.getVulnerability().name()); - - responseBuilder.addRules(ruleBuilder.build()); - } - } - private static final class WsRequest { private final int page; private final int index; @@ -306,7 +293,7 @@ public class SearchAction implements HotspotsWsAction { private final Paging paging; private final List orderedHotspots; private final Set components = new HashSet<>(); - private final Set rules = new HashSet<>(); + private final Map rulesByRuleKey = new HashMap<>(); private SearchResponseData(Paging paging, List orderedHotspots) { this.paging = paging; @@ -334,11 +321,11 @@ public class SearchAction implements HotspotsWsAction { } void addRules(Collection rules) { - this.rules.addAll(rules); + rules.forEach(t -> rulesByRuleKey.put(t.getKey(), t)); } - Set getRules() { - return rules; + Optional getRule(RuleKey ruleKey) { + return ofNullable(rulesByRuleKey.get(ruleKey)); } } 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 082e4a801da..2e173802409 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 @@ -19,7 +19,11 @@ */ package org.sonar.server.hotspot.ws; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; +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.Collections; import java.util.Comparator; @@ -33,6 +37,7 @@ import java.util.stream.IntStream; import java.util.stream.Stream; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; import org.sonar.api.issue.Issue; import org.sonar.api.rules.RuleType; import org.sonar.api.utils.System2; @@ -64,6 +69,7 @@ import org.sonarqube.ws.Hotspots.Component; import org.sonarqube.ws.Hotspots.SearchWsResponse; import static java.util.Collections.singleton; +import static java.util.stream.Collectors.toSet; import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -74,6 +80,7 @@ import static org.sonar.db.component.ComponentTesting.newDirectory; import static org.sonar.db.component.ComponentTesting.newFileDto; import static org.sonar.db.issue.IssueTesting.newIssue; +@RunWith(DataProviderRunner.class) public class SearchActionTest { private static final Random RANDOM = new Random(); @@ -158,7 +165,6 @@ public class SearchActionTest { assertThat(response.getHotspotsList()).isEmpty(); assertThat(response.getComponentsList()).isEmpty(); - assertThat(response.getRulesList()).isEmpty(); } @Test @@ -172,7 +178,33 @@ public class SearchActionTest { assertThat(response.getHotspotsList()).isEmpty(); assertThat(response.getComponentsList()).isEmpty(); - assertThat(response.getRulesList()).isEmpty(); + } + + @Test + public void does_not_fail_if_rule_of_hotspot_does_not_exist_in_DB() { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); + indexPermissions(); + 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)); + }) + .toArray(IssueDto[]::new); + indexIssues(); + IssueDto hotspotWithoutRule = hotspots[RANDOM.nextInt(hotspots.length)]; + dbTester.executeUpdateSql("delete from rules where id=" + hotspotWithoutRule.getRuleId()); + + SearchWsResponse response = newRequest(project) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()) + .extracting(Hotspots.Hotspot::getKey) + .containsOnly(Arrays.stream(hotspots) + .filter(t -> !t.getKey().equals(hotspotWithoutRule.getKey())) + .map(IssueDto::getKey) + .toArray(String[]::new)); } @Test @@ -191,14 +223,10 @@ public class SearchActionTest { SearchWsResponse response = newRequest(project) .executeProtobuf(SearchWsResponse.class); - - assertThat(response.getHotspotsList()).isEmpty(); - assertThat(response.getComponentsList()).isEmpty(); - assertThat(response.getRulesList()).isEmpty(); } @Test - public void returns_hotspot_component_and_rule_when_project_has_hotspots() { + public void returns_hotspot_components_when_project_has_hotspots() { ComponentDto project = dbTester.components().insertPublicProject(); userSessionRule.registerComponents(project); indexPermissions(); @@ -227,9 +255,30 @@ public class SearchActionTest { assertThat(response.getComponentsList()) .extracting(Component::getKey) .containsOnly(project.getKey(), fileWithHotspot.getKey()); - assertThat(response.getRulesList()) - .extracting(Hotspots.Rule::getKey) - .containsOnly(Arrays.stream(hotspots).map(t -> t.getRuleKey().toString()).toArray(String[]::new)); + } + + @Test + public void returns_single_component_when_all_hotspots_are_on_project() { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + indexPermissions(); + 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)); + }) + .toArray(IssueDto[]::new); + indexIssues(); + + SearchWsResponse response = newRequest(project) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()) + .extracting(Hotspots.Hotspot::getKey) + .containsOnly(Arrays.stream(hotspots).map(IssueDto::getKey).toArray(String[]::new)); + assertThat(response.getComponentsList()) + .extracting(Component::getKey) + .containsOnly(project.getKey()); } @Test @@ -258,9 +307,6 @@ public class SearchActionTest { assertThat(responseProject1.getComponentsList()) .extracting(Component::getKey) .containsOnly(project1.getKey(), file1.getKey()); - assertThat(responseProject1.getRulesList()) - .extracting(Hotspots.Rule::getKey) - .containsOnly(Arrays.stream(hotspots2).map(t -> t.getRuleKey().toString()).toArray(String[]::new)); SearchWsResponse responseProject2 = newRequest(project2) .executeProtobuf(SearchWsResponse.class); @@ -271,9 +317,6 @@ public class SearchActionTest { assertThat(responseProject2.getComponentsList()) .extracting(Component::getKey) .containsOnly(project2.getKey(), file2.getKey()); - assertThat(responseProject2.getRulesList()) - .extracting(Hotspots.Rule::getKey) - .containsOnly(Arrays.stream(hotspots2).map(t -> t.getRuleKey().toString()).toArray(String[]::new)); } @Test @@ -323,7 +366,6 @@ public class SearchActionTest { Hotspots.Hotspot actual = response.getHotspots(0); assertThat(actual.getComponent()).isEqualTo(file.getKey()); assertThat(actual.getProject()).isEqualTo(project.getKey()); - assertThat(actual.getRule()).isEqualTo(hotspot.getRuleKey().toString()); assertThat(actual.getStatus()).isEqualTo(hotspot.getStatus()); // FIXME resolution field will be added later // assertThat(actual.getResolution()).isEqualTo(hotspot.getResolution()); @@ -335,6 +377,40 @@ public class SearchActionTest { assertThat(actual.getUpdateDate()).isEqualTo(formatDateTime(hotspot.getIssueUpdateDate())); } + @Test + @UseDataProvider("allSQCategories") + public void returns_SQCategory_and_VulnerabilityProbability_of_rule(Set securityStandards, SQCategory expected) { + ComponentDto project = dbTester.components().insertPublicProject(); + userSessionRule.registerComponents(project); + 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)); + indexIssues(); + + SearchWsResponse response = newRequest(project) + .executeProtobuf(SearchWsResponse.class); + + assertThat(response.getHotspotsList()).hasSize(1); + Hotspots.Hotspot actual = response.getHotspots(0); + assertThat(actual.getSecurityCategory()).isEqualTo(expected.getKey()); + assertThat(actual.getVulnerabilityProbability()).isEqualTo(expected.getVulnerability().name()); + } + + @DataProvider + public static Object[][] allSQCategories() { + Stream allCategoriesButOTHERS = SecurityStandards.CWES_BY_SQ_CATEGORY.entrySet() + .stream() + .map(t -> new Object[] { + t.getValue().stream().map(c -> "cwe:" + c).collect(toSet()), + t.getKey() + }); + Stream sqCategoryOTHERS = Stream.of( + new Object[] {Collections.emptySet(), SQCategory.OTHERS}, + new Object[] {ImmutableSet.of("foo", "donut", "acme"), SQCategory.OTHERS}); + return Stream.concat(allCategoriesButOTHERS, sqCategoryOTHERS).toArray(Object[][]::new); + } + @Test public void does_not_fail_when_hotspot_has_none_of_the_nullable_fields() { ComponentDto project = dbTester.components().insertPublicProject(); diff --git a/sonar-ws/src/main/protobuf/ws-hotspots.proto b/sonar-ws/src/main/protobuf/ws-hotspots.proto index a6d71dffee1..e6650472d04 100644 --- a/sonar-ws/src/main/protobuf/ws-hotspots.proto +++ b/sonar-ws/src/main/protobuf/ws-hotspots.proto @@ -31,24 +31,23 @@ message SearchWsResponse { optional sonarqube.ws.commons.Paging paging = 1; repeated Hotspot hotspots = 2; repeated Component components = 3; - repeated Rule rules = 4; } message Hotspot { optional string key = 1; optional string component = 2; optional string project = 3; - optional string rule = 4; - optional string status = 5; + optional string securityCategory = 4; + optional string vulnerabilityProbability = 5; + optional string status = 6; // FIXME resolution field will be added later -// optional string resolution = 6; - optional int32 line = 7; - optional string message = 8; - optional string assignee = 9; - // SCM login of the committer who introduced the issue - optional string author = 10; - optional string creationDate = 11; - optional string updateDate = 12; +// optional string resolution = 7; + optional int32 line = 8; + optional string message = 9; + optional string assignee = 10; + optional string author = 11; + optional string creationDate = 12; + optional string updateDate = 13; } message Component { @@ -59,10 +58,3 @@ message Component { optional string longName = 5; optional string path = 6; } - -message Rule { - optional string key = 1; - optional string name = 2; - optional string securityCategory = 3; - optional string vulnerabilityProbability = 4; -} -- 2.39.5