From 1f0fdb5b789ca4cebb5216c2b56148264a213bd3 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 20 Feb 2020 13:30:02 +0100 Subject: [PATCH] SONAR-12960 Do not compute Security Hotspots Reviewed when 0 hotspots --- .../NewSecurityReviewMeasuresVisitor.java | 7 ++++--- .../SecurityReviewMeasuresVisitor.java | 13 ++++++------ .../NewSecurityReviewMeasuresVisitorTest.java | 14 ++++++++----- .../SecurityReviewMeasuresVisitorTest.java | 12 +++++++---- .../server/security/SecurityReviewRating.java | 12 ++++++----- .../security/SecurityReviewRatingTest.java | 11 +++++----- .../live/IssueMetricFormulaFactoryImpl.java | 20 +++++++++---------- .../IssueMetricFormulaFactoryImplTest.java | 16 +++++++++++++-- 8 files changed, 65 insertions(+), 40 deletions(-) diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitor.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitor.java index 806915dded5..cbd94f0f20b 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitor.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitor.java @@ -19,6 +19,7 @@ */ package org.sonar.ce.task.projectanalysis.qualitymodel; +import java.util.Optional; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.component.PathAwareVisitorAdapter; @@ -94,9 +95,9 @@ public class NewSecurityReviewMeasuresVisitor extends PathAwareVisitorAdapter analysisMetadataHolder.isPullRequest() || periodHolder.getPeriod().isOnPeriod(issue.creationDate())) .forEach(issue -> path.current().processHotspot(issue)); - double percent = computePercent(path.current().getHotspotsToReview(), path.current().getHotspotsReviewed()); - measureRepository.add(component, newSecurityHotspotsReviewedMetric, Measure.newMeasureBuilder().setVariation(percent).createNoValue()); - measureRepository.add(component, newSecurityReviewRatingMetric, Measure.newMeasureBuilder().setVariation(computeRating(percent).getIndex()).createNoValue()); + Optional percent = computePercent(path.current().getHotspotsToReview(), path.current().getHotspotsReviewed()); + measureRepository.add(component, newSecurityReviewRatingMetric, Measure.newMeasureBuilder().setVariation(computeRating(percent.orElse(null)).getIndex()).createNoValue()); + percent.ifPresent(p -> measureRepository.add(component, newSecurityHotspotsReviewedMetric, Measure.newMeasureBuilder().setVariation(p).createNoValue())); if (!path.isRoot()) { path.parent().add(path.current()); diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitor.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitor.java index 574d53a74e3..2cb2741724d 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitor.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitor.java @@ -19,11 +19,11 @@ */ package org.sonar.ce.task.projectanalysis.qualitymodel; +import java.util.Optional; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.component.PathAwareVisitor; import org.sonar.ce.task.projectanalysis.component.PathAwareVisitorAdapter; import org.sonar.ce.task.projectanalysis.issue.ComponentIssuesRepository; -import org.sonar.ce.task.projectanalysis.measure.Measure; import org.sonar.ce.task.projectanalysis.measure.MeasureRepository; import org.sonar.ce.task.projectanalysis.measure.RatingMeasures; import org.sonar.ce.task.projectanalysis.metric.Metric; @@ -36,6 +36,7 @@ import static org.sonar.api.measures.CoreMetrics.SECURITY_REVIEW_RATING_KEY; import static org.sonar.api.rules.RuleType.SECURITY_HOTSPOT; import static org.sonar.ce.task.projectanalysis.component.ComponentVisitor.Order.POST_ORDER; import static org.sonar.ce.task.projectanalysis.component.CrawlerDepthLimit.FILE; +import static org.sonar.ce.task.projectanalysis.measure.Measure.newMeasureBuilder; import static org.sonar.server.security.SecurityReviewRating.computePercent; import static org.sonar.server.security.SecurityReviewRating.computeRating; @@ -62,8 +63,8 @@ public class SecurityReviewMeasuresVisitor extends PathAwareVisitorAdapter path) { computeMeasure(project, path); // The following measures are only computed on projects level as they are required to compute the others measures on applications - measureRepository.add(project, securityHotspotsReviewedStatusMetric, Measure.newMeasureBuilder().create(path.current().getHotspotsReviewed())); - measureRepository.add(project, securityHotspotsToReviewStatusMetric, Measure.newMeasureBuilder().create(path.current().getHotspotsToReview())); + measureRepository.add(project, securityHotspotsReviewedStatusMetric, newMeasureBuilder().create(path.current().getHotspotsReviewed())); + measureRepository.add(project, securityHotspotsToReviewStatusMetric, newMeasureBuilder().create(path.current().getHotspotsToReview())); } @Override @@ -82,9 +83,9 @@ public class SecurityReviewMeasuresVisitor extends PathAwareVisitorAdapter issue.type().equals(SECURITY_HOTSPOT)) .forEach(issue -> path.current().processHotspot(issue)); - double percent = computePercent(path.current().getHotspotsToReview(), path.current().getHotspotsReviewed()); - measureRepository.add(component, securityHotspotsReviewedMetric, Measure.newMeasureBuilder().create(percent, securityHotspotsReviewedMetric.getDecimalScale())); - measureRepository.add(component, securityReviewRatingMetric, RatingMeasures.get(computeRating(percent))); + Optional percent = computePercent(path.current().getHotspotsToReview(), path.current().getHotspotsReviewed()); + measureRepository.add(component, securityReviewRatingMetric, RatingMeasures.get(computeRating(percent.orElse(null)))); + percent.ifPresent(p -> measureRepository.add(component, securityHotspotsReviewedMetric, newMeasureBuilder().create(p, securityHotspotsReviewedMetric.getDecimalScale()))); if (!path.isRoot()) { path.parent().add(path.current()); diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitorTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitorTest.java index c39752eb775..bf873feccda 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitorTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/NewSecurityReviewMeasuresVisitorTest.java @@ -288,7 +288,7 @@ public class NewSecurityReviewMeasuresVisitorTest { } @Test - public void compute_A_rating_and_100_percent_when_no_new_hotspot_on_new_code() { + public void compute_A_rating_and_no_percent_when_no_new_hotspot_on_new_code() { treeRootHolder.setRoot(ROOT_PROJECT); fillComponentIssuesVisitorRule.setIssues(FILE_1_REF, newHotspot(STATUS_TO_REVIEW, null).setCreationDate(BEFORE_LEAK_PERIOD_DATE), @@ -297,7 +297,7 @@ public class NewSecurityReviewMeasuresVisitorTest { underTest.visit(ROOT_PROJECT); - verifyRatingAndReviewedMeasures(PROJECT_REF, A, 100.0); + verifyRatingAndReviewedMeasures(PROJECT_REF, A, null); } @Test @@ -378,10 +378,14 @@ public class NewSecurityReviewMeasuresVisitorTest { assertThat(measureRepository.getAddedRawMeasures(PROJECT_REF).values()).isEmpty(); } - private void verifyRatingAndReviewedMeasures(int componentRef, Rating expectedReviewRating, double expectedHotspotsReviewed) { + private void verifyRatingAndReviewedMeasures(int componentRef, Rating expectedReviewRating, @Nullable Double expectedHotspotsReviewed) { assertThat(measureRepository.getAddedRawMeasure(componentRef, NEW_SECURITY_REVIEW_RATING_KEY)).hasVariation(expectedReviewRating.getIndex()); - assertThat(measureRepository.getAddedRawMeasure(componentRef, NEW_SECURITY_HOTSPOTS_REVIEWED_KEY)).hasVariation(expectedHotspotsReviewed, - VARIATION_COMPARISON_OFFSET); + if (expectedHotspotsReviewed != null){ + assertThat(measureRepository.getAddedRawMeasure(componentRef, NEW_SECURITY_HOTSPOTS_REVIEWED_KEY)).hasVariation(expectedHotspotsReviewed, + VARIATION_COMPARISON_OFFSET); + } else { + assertThat(measureRepository.getAddedRawMeasure(componentRef, NEW_SECURITY_HOTSPOTS_REVIEWED_KEY)).isAbsent(); + } } private void verifyHotspotStatusMeasures(int componentRef, @Nullable Integer hotspotsReviewed, @Nullable Integer hotspotsToReview) { diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitorTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitorTest.java index 9780f4e72f3..cf47649b6e5 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitorTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/qualitymodel/SecurityReviewMeasuresVisitorTest.java @@ -247,12 +247,12 @@ public class SecurityReviewMeasuresVisitorTest { } @Test - public void compute_A_rating_and_100_percent_when_no_hotspot() { + public void compute_A_rating_and_no_reviewed_when_no_hotspot() { treeRootHolder.setRoot(ROOT_PROJECT); underTest.visit(ROOT_PROJECT); - verifyRatingAndReviewedMeasures(PROJECT_REF, A, 100.0); + verifyRatingAndReviewedMeasures(PROJECT_REF, A, null); } @Test @@ -289,9 +289,13 @@ public class SecurityReviewMeasuresVisitorTest { verifyHotspotStatusMeasures(PROJECT_REF, 0, 0); } - private void verifyRatingAndReviewedMeasures(int componentRef, Rating expectedReviewRating, double expectedHotspotsReviewed) { + private void verifyRatingAndReviewedMeasures(int componentRef, Rating expectedReviewRating, @Nullable Double expectedHotspotsReviewed) { verifySecurityReviewRating(componentRef, expectedReviewRating); - verifySecurityHotspotsReviewed(componentRef, expectedHotspotsReviewed); + if (expectedHotspotsReviewed != null) { + verifySecurityHotspotsReviewed(componentRef, expectedHotspotsReviewed); + } else { + assertThat(measureRepository.getAddedRawMeasure(componentRef, SECURITY_HOTSPOTS_REVIEWED_KEY)).isEmpty(); + } } private void verifySecurityReviewRating(int componentRef, Rating rating) { diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/security/SecurityReviewRating.java b/server/sonar-server-common/src/main/java/org/sonar/server/security/SecurityReviewRating.java index d2c038ccc97..06999dd581d 100644 --- a/server/sonar-server-common/src/main/java/org/sonar/server/security/SecurityReviewRating.java +++ b/server/sonar-server-common/src/main/java/org/sonar/server/security/SecurityReviewRating.java @@ -19,6 +19,8 @@ */ package org.sonar.server.security; +import java.util.Optional; +import javax.annotation.Nullable; import org.sonar.server.measure.Rating; import static org.sonar.server.measure.Rating.A; @@ -33,16 +35,16 @@ public class SecurityReviewRating { // Only static method } - public static double computePercent(long hotspotsToReview, long hotspotsReviewed) { + public static Optional computePercent(long hotspotsToReview, long hotspotsReviewed) { long total = hotspotsToReview + hotspotsReviewed; if (total == 0) { - return 100.0; + return Optional.empty(); } - return hotspotsReviewed * 100.0 / total; + return Optional.of(hotspotsReviewed * 100.0 / total); } - public static Rating computeRating(double percent) { - if (percent >= 80.0) { + public static Rating computeRating(@Nullable Double percent) { + if (percent == null || percent >= 80.0) { return A; } else if (percent >= 70.0) { return B; diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/security/SecurityReviewRatingTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/security/SecurityReviewRatingTest.java index 7c4e5d44821..188401492e8 100644 --- a/server/sonar-server-common/src/test/java/org/sonar/server/security/SecurityReviewRatingTest.java +++ b/server/sonar-server-common/src/test/java/org/sonar/server/security/SecurityReviewRatingTest.java @@ -67,10 +67,11 @@ public class SecurityReviewRatingTest { @Test public void compute_percent() { - assertThat(computePercent(0, 10)).isEqualTo(100.0); - assertThat(computePercent(1, 3)).isEqualTo(75.0); - assertThat(computePercent(3, 4)).isEqualTo(57.14, DOUBLE_OFFSET); - assertThat(computePercent(10, 10)).isEqualTo(50.0); - assertThat(computePercent(10, 0)).isEqualTo(0.0); + assertThat(computePercent(0, 0)).isEmpty(); + assertThat(computePercent(0, 10).get()).isEqualTo(100.0); + assertThat(computePercent(1, 3).get()).isEqualTo(75.0); + assertThat(computePercent(3, 4).get()).isEqualTo(57.14, DOUBLE_OFFSET); + assertThat(computePercent(10, 10).get()).isEqualTo(50.0); + assertThat(computePercent(10, 0).get()).isEqualTo(0.0); } } diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImpl.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImpl.java index d4a63507f33..1d9cbfd14de 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImpl.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImpl.java @@ -110,12 +110,14 @@ public class IssueMetricFormulaFactoryImpl implements IssueMetricFormulaFactory (context, issues) -> context.setValue(RATING_BY_SEVERITY.get(issues.getHighestSeverityOfUnresolved(RuleType.VULNERABILITY, false).orElse(Severity.INFO)))), new IssueMetricFormula(CoreMetrics.SECURITY_REVIEW_RATING, false, - (context, issues) -> context - .setValue(computeRating(computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, false), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, false))))), + (context, issues) -> { + Optional percent = computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, false), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, false)); + context.setValue(computeRating(percent.orElse(null))); + }), new IssueMetricFormula(CoreMetrics.SECURITY_HOTSPOTS_REVIEWED, false, - (context, issues) -> context - .setValue(computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, false), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, false)))), + (context, issues) -> computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, false), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, false)) + .ifPresent(context::setValue)), new IssueMetricFormula(CoreMetrics.SECURITY_HOTSPOTS_REVIEWED_STATUS, false, (context, issues) -> context.setValue(issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, false))), @@ -176,15 +178,13 @@ public class IssueMetricFormulaFactoryImpl implements IssueMetricFormulaFactory new IssueMetricFormula(CoreMetrics.NEW_SECURITY_REVIEW_RATING, true, (context, issues) -> { - Rating rating = computeRating(computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, true), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, true))); - context.setLeakValue(rating); + Optional percent = computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, true), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, true)); + context.setLeakValue(computeRating(percent.orElse(null))); }), new IssueMetricFormula(CoreMetrics.NEW_SECURITY_HOTSPOTS_REVIEWED, true, - (context, issues) -> { - double percent = computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, true), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, true)); - context.setLeakValue(percent); - }), + (context, issues) -> computePercent(issues.countHotspotsByStatus(Issue.STATUS_TO_REVIEW, true), issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, true)) + .ifPresent(context::setLeakValue)), new IssueMetricFormula(CoreMetrics.NEW_SECURITY_HOTSPOTS_REVIEWED_STATUS, true, (context, issues) -> context.setLeakValue(issues.countHotspotsByStatus(Issue.STATUS_REVIEWED, true))), diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImplTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImplTest.java index 7f92edb76e0..aec34528be5 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImplTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/measure/live/IssueMetricFormulaFactoryImplTest.java @@ -142,7 +142,7 @@ public class IssueMetricFormulaFactoryImplTest { .assertThatValueIs(CoreMetrics.SECURITY_HOTSPOTS_REVIEWED, 75.0); withNoIssues() - .assertThatValueIs(CoreMetrics.SECURITY_HOTSPOTS_REVIEWED, 100.0); + .assertNoValue(CoreMetrics.SECURITY_HOTSPOTS_REVIEWED); } @Test @@ -697,7 +697,7 @@ public class IssueMetricFormulaFactoryImplTest { .assertThatLeakValueIs(CoreMetrics.NEW_SECURITY_HOTSPOTS_REVIEWED, 75.0); withNoIssues() - .assertThatLeakValueIs(CoreMetrics.NEW_SECURITY_HOTSPOTS_REVIEWED, 100.0); + .assertNoLeakValue(CoreMetrics.NEW_SECURITY_HOTSPOTS_REVIEWED); } @Test @@ -850,12 +850,24 @@ public class IssueMetricFormulaFactoryImplTest { return this; } + Verifier assertNoLeakValue(Metric metric) { + TestContext context = run(metric, true); + assertThat(context.ratingLeakValue).isNull(); + return this; + } + Verifier assertThatValueIs(Metric metric, Rating expectedValue) { TestContext context = run(metric, false); assertThat(context.ratingValue).isNotNull().isEqualTo(expectedValue); return this; } + Verifier assertNoValue(Metric metric) { + TestContext context = run(metric, false); + assertThat(context.ratingValue).isNull(); + return this; + } + private TestContext run(Metric metric, boolean expectLeakFormula) { IssueMetricFormula formula = underTest.getFormulas().stream() .filter(f -> f.getMetric().getKey().equals(metric.getKey())) -- 2.39.5