From 3c0b5c93a8b6b09dfb2c8b5ed2ec71f117cd3431 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Thu, 16 Sep 2021 14:52:15 -0500 Subject: [PATCH] SONAR-15352 Reopened closed hotspots should bring back their previous state --- .../issue/ComponentIssuesLoader.java | 5 +- .../org/sonar/db/issue/IssueMapper.xml | 1 - .../org/sonar/db/issue/IssueMapperTest.java | 8 +-- .../server/issue/workflow/IsHotspot.java | 33 ++++++++++++ .../server/issue/workflow/IssueWorkflow.java | 20 +++++++ .../IssueWorkflowForSecurityHotspotsTest.java | 54 +++++++++---------- .../issue/workflow/IssueWorkflowTest.java | 2 +- 7 files changed, 87 insertions(+), 36 deletions(-) create mode 100644 server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IsHotspot.java diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComponentIssuesLoader.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComponentIssuesLoader.java index a1e359739e6..bafc40cf4ef 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComponentIssuesLoader.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComponentIssuesLoader.java @@ -46,6 +46,7 @@ import org.sonar.db.issue.IssueMapper; import static com.google.common.base.Preconditions.checkState; import static java.util.Collections.emptyList; +import static java.util.Collections.unmodifiableList; import static java.util.stream.Collectors.groupingBy; import static java.util.stream.Collectors.toList; import static org.sonar.api.issue.Issue.STATUS_CLOSED; @@ -203,7 +204,7 @@ public class ComponentIssuesLoader { /** * Load closed issues for the specified Component, which have at least one line diff in changelog AND are - * neither hotspots nor manual vulnerabilities. + * not manual vulnerabilities. *

* Closed issues do not have a line number in DB (it is unset when the issue is closed), this method * returns {@link DefaultIssue} objects which line number is populated from the most recent diff logging @@ -230,7 +231,7 @@ public class ComponentIssuesLoader { private static List loadClosedIssues(DbSession dbSession, String componentUuid, long closeDateAfter) { ClosedIssuesResultHandler handler = new ClosedIssuesResultHandler(); dbSession.getMapper(IssueMapper.class).scrollClosedByComponentUuid(componentUuid, closeDateAfter, handler); - return ImmutableList.copyOf(handler.issues); + return unmodifiableList(handler.issues); } private static class ClosedIssuesResultHandler implements ResultHandler { diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/issue/IssueMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/issue/IssueMapper.xml index 933fa200820..f8abc8fdf9f 100644 --- a/server/sonar-db-dao/src/main/resources/org/sonar/db/issue/IssueMapper.xml +++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/issue/IssueMapper.xml @@ -228,7 +228,6 @@ and i.status = 'CLOSED' and i.issue_close_date is not null and i.issue_close_date >= #{closeDateAfter,jdbcType=BIGINT} - and i.issue_type <> 4 order by i.kee, ic.issue_change_creation_date desc diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueMapperTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueMapperTest.java index ff9bb5f60dc..fc40fcab457 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueMapperTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueMapperTest.java @@ -313,11 +313,11 @@ public class IssueMapperTest { } @Test - public void scrollClosedByComponentUuid_does_not_return_closed_issues_of_type_SECURITY_HOTSPOT() { + public void scrollClosedByComponentUuid_returns_closed_issues_of_type_SECURITY_HOTSPOT() { RuleType ruleType = randomSupportedRuleType(); ComponentDto component = randomComponent(); IssueDto securityHotspotIssue = insertNewClosedIssue(component, RuleType.SECURITY_HOTSPOT); - insertToClosedDiff(securityHotspotIssue); + IssueChangeDto hotspotChangedData = insertToClosedDiff(securityHotspotIssue); IssueDto issue = insertNewClosedIssue(component, ruleType); IssueChangeDto issueChange = insertToClosedDiff(issue); @@ -326,7 +326,9 @@ public class IssueMapperTest { assertThat(resultHandler.issues) .extracting(IssueDto::getKey, t -> t.getClosedChangeData().get()) - .containsOnly(tuple(issue.getKey(), issueChange.getChangeData())); + .containsOnly( + tuple(issue.getKey(), issueChange.getChangeData()), + tuple(securityHotspotIssue.getKey(), hotspotChangedData.getChangeData())); } @Test diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IsHotspot.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IsHotspot.java new file mode 100644 index 00000000000..0e0cbaa5f7f --- /dev/null +++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IsHotspot.java @@ -0,0 +1,33 @@ +/* + * SonarQube + * Copyright (C) 2009-2021 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.issue.workflow; + +import org.sonar.api.issue.Issue; +import org.sonar.api.rules.RuleType; +import org.sonar.core.issue.DefaultIssue; + +enum IsHotspot implements Condition { + INSTANCE; + + @Override + public boolean matches(Issue issue) { + return ((DefaultIssue) issue).type() == RuleType.SECURITY_HOTSPOT; + } +} diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java index 613d0ef9db4..bd2671a2c18 100644 --- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java +++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java @@ -283,6 +283,26 @@ public class IssueWorkflow implements Startable { IsNotHotspot.INSTANCE) .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE) .automatic() + .build()) + + // reopen closed hotspots + .transition(Transition.builder("automaticunclosetoreview") + .from(STATUS_CLOSED).to(STATUS_TO_REVIEW) + .conditions( + new PreviousStatusWas(STATUS_TO_REVIEW), + new HasResolution(RESOLUTION_REMOVED, RESOLUTION_FIXED), + IsHotspot.INSTANCE) + .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE) + .automatic() + .build()) + .transition(Transition.builder("automaticunclosereviewed") + .from(STATUS_CLOSED).to(STATUS_REVIEWED) + .conditions( + new PreviousStatusWas(STATUS_REVIEWED), + new HasResolution(RESOLUTION_REMOVED, RESOLUTION_FIXED), + IsHotspot.INSTANCE) + .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE) + .automatic() .build()); } diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowForSecurityHotspotsTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowForSecurityHotspotsTest.java index d71bb665765..83815d8c87c 100644 --- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowForSecurityHotspotsTest.java +++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowForSecurityHotspotsTest.java @@ -22,7 +22,6 @@ package org.sonar.server.issue.workflow; 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.Calendar; import java.util.Collection; import java.util.Date; @@ -53,15 +52,14 @@ import static org.sonar.api.issue.Issue.STATUS_REVIEWED; import static org.sonar.api.issue.Issue.STATUS_TO_REVIEW; import static org.sonar.api.rules.RuleType.SECURITY_HOTSPOT; import static org.sonar.db.rule.RuleTesting.XOO_X1; +import static org.sonar.server.issue.workflow.IssueWorkflowTest.emptyIfNull; @RunWith(DataProviderRunner.class) public class IssueWorkflowForSecurityHotspotsTest { - private static final IssueChangeContext SOME_CHANGE_CONTEXT = IssueChangeContext.createUser(new Date(), "USER1"); - private IssueFieldsSetter updater = new IssueFieldsSetter(); - - private IssueWorkflow underTest = new IssueWorkflow(new FunctionExecutor(updater), updater); + private final IssueFieldsSetter updater = new IssueFieldsSetter(); + private final IssueWorkflow underTest = new IssueWorkflow(new FunctionExecutor(updater), updater); @Test @UseDataProvider("anyResolutionIncludingNone") @@ -255,31 +253,26 @@ public class IssueWorkflowForSecurityHotspotsTest { } @Test - @UseDataProvider("allStatusesLeadingToClosed") - public void do_not_automatically_reopen_closed_issues_of_security_hotspots(String previousStatus) { - DefaultIssue[] hotspots = Stream.of(RESOLUTION_FIXED, RESOLUTION_REMOVED) - .map(resolution -> { - DefaultIssue issue = newClosedHotspot(resolution); - setStatusPreviousToClosed(issue, previousStatus); - return issue; - }) - .toArray(DefaultIssue[]::new); + public void automatically_reopen_closed_security_hotspots() { + DefaultIssue hotspot1 = newClosedHotspot(RESOLUTION_REMOVED); + setStatusPreviousToClosed(hotspot1, STATUS_REVIEWED, RESOLUTION_SAFE, RESOLUTION_REMOVED); + + DefaultIssue hotspot2 = newClosedHotspot(RESOLUTION_FIXED); + setStatusPreviousToClosed(hotspot2, STATUS_TO_REVIEW, null, RESOLUTION_FIXED); + Date now = new Date(); underTest.start(); - Arrays.stream(hotspots).forEach(hotspot -> { - underTest.doAutomaticTransition(hotspot, IssueChangeContext.createScan(now)); + underTest.doAutomaticTransition(hotspot1, IssueChangeContext.createScan(now)); + underTest.doAutomaticTransition(hotspot2, IssueChangeContext.createScan(now)); - assertThat(hotspot.status()).isEqualTo(STATUS_CLOSED); - assertThat(hotspot.updateDate()).isNull(); - }); - } + assertThat(hotspot1.updateDate()).isNotNull(); + assertThat(hotspot1.status()).isEqualTo(STATUS_REVIEWED); + assertThat(hotspot1.resolution()).isEqualTo(RESOLUTION_SAFE); - @DataProvider - public static Object[][] allStatusesLeadingToClosed() { - return Stream.of(STATUS_TO_REVIEW, STATUS_REVIEWED) - .map(t -> new Object[] {t}) - .toArray(Object[][]::new); + assertThat(hotspot2.updateDate()).isNotNull(); + assertThat(hotspot2.status()).isEqualTo(STATUS_TO_REVIEW); + assertThat(hotspot2.resolution()).isNull(); } @Test @@ -299,12 +292,15 @@ public class IssueWorkflowForSecurityHotspotsTest { return transitions.stream().map(Transition::key).collect(MoreCollectors.toList()); } - private static void setStatusPreviousToClosed(DefaultIssue hotspot, String previousStatus) { - addStatusChange(hotspot, new Date(), previousStatus, STATUS_CLOSED); + private static void setStatusPreviousToClosed(DefaultIssue hotspot, String previousStatus, @Nullable String previousResolution, @Nullable String newResolution) { + addStatusChange(hotspot, new Date(), previousStatus, STATUS_CLOSED, previousResolution, newResolution); } - private static void addStatusChange(DefaultIssue issue, Date date, String previousStatus, String newStatus) { - issue.addChange(new FieldDiffs().setCreationDate(date).setDiff("status", previousStatus, newStatus)); + private static void addStatusChange(DefaultIssue issue, Date date, String previousStatus, String newStatus, @Nullable String previousResolution, @Nullable String newResolution) { + issue.addChange(new FieldDiffs() + .setCreationDate(date) + .setDiff("status", previousStatus, newStatus) + .setDiff("resolution", emptyIfNull(previousResolution), emptyIfNull(newResolution))); } private static DefaultIssue newClosedHotspot(String resolution) { diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java index f8499c73d49..a26a03d6ec6 100644 --- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java +++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java @@ -466,7 +466,7 @@ public class IssueWorkflowTest { issue.addChange(fieldDiffs); } - private static String emptyIfNull(@Nullable String newResolution) { + static String emptyIfNull(@Nullable String newResolution) { return newResolution == null ? "" : newResolution; } -- 2.39.5