From b922a7648b5a6d4a26a659900ceb8644cbc5c10b Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Mon, 13 Apr 2020 15:37:35 -0500 Subject: SONAR-13196 Search issues since leak period returns all issues if no leak period exists --- .../org/sonar/server/issue/index/IssueIndex.java | 2 +- .../server/issue/index/IssueQueryFactory.java | 21 ++++++---- .../server/issue/index/IssueQueryFactoryTest.java | 47 ++++++++++++---------- 3 files changed, 39 insertions(+), 31 deletions(-) (limited to 'server/sonar-webserver-es') diff --git a/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueIndex.java b/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueIndex.java index d9b1ecadc96..58c00a5fa8d 100644 --- a/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueIndex.java +++ b/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueIndex.java @@ -638,7 +638,7 @@ public class IssueIndex { } private void validateCreationDateBounds(@Nullable Date createdBefore, @Nullable Date createdAfter) { - Preconditions.checkArgument(createdAfter == null || createdAfter.before(new Date(system.now())), + Preconditions.checkArgument(createdAfter == null || createdAfter.compareTo(new Date(system.now())) <= 0, "Start bound cannot be in the future"); Preconditions.checkArgument(createdAfter == null || createdBefore == null || createdAfter.before(createdBefore), "Start bound cannot be larger or equal to end bound"); diff --git a/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueQueryFactory.java b/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueQueryFactory.java index 7dd6221826f..d6c7f69d8c3 100644 --- a/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueQueryFactory.java +++ b/server/sonar-webserver-es/src/main/java/org/sonar/server/issue/index/IssueQueryFactory.java @@ -95,7 +95,7 @@ public class IssueQueryFactory { .map(Enum::name) .collect(MoreCollectors.toSet(RuleType.values().length - 1)); private static final ComponentDto UNKNOWN_COMPONENT = new ComponentDto().setUuid(UNKNOWN).setProjectUuid(UNKNOWN); - + private static final Set QUALIFIERS_WITHOUT_LEAK_PERIOD = new HashSet<>(Arrays.asList(Qualifiers.APP, Qualifiers.VIEW, Qualifiers.SUBVIEW)); private final DbClient dbClient; private final Clock clock; private final UserSession userSession; @@ -145,8 +145,6 @@ public class IssueQueryFactory { } private void setCreatedAfterFromDates(IssueQuery.Builder builder, @Nullable Date createdAfter, @Nullable String createdInLast, boolean createdAfterInclusive) { - checkArgument(createdAfter == null || createdInLast == null, format("Parameters %s and %s cannot be set simultaneously", PARAM_CREATED_AFTER, PARAM_CREATED_IN_LAST)); - Date actualCreatedAfter = createdAfter; if (createdInLast != null) { actualCreatedAfter = Date.from( @@ -171,20 +169,26 @@ public class IssueQueryFactory { String createdInLast = request.getCreatedInLast(); if (request.getSinceLeakPeriod() == null || !request.getSinceLeakPeriod()) { + checkArgument(createdAfter == null || createdInLast == null, format("Parameters %s and %s cannot be set simultaneously", PARAM_CREATED_AFTER, PARAM_CREATED_IN_LAST)); setCreatedAfterFromDates(builder, createdAfter, createdInLast, true); } else { checkArgument(createdAfter == null, "Parameters '%s' and '%s' cannot be set simultaneously", PARAM_CREATED_AFTER, PARAM_SINCE_LEAK_PERIOD); + checkArgument(createdInLast == null, format("Parameters %s and %s cannot be set simultaneously", PARAM_CREATED_IN_LAST, PARAM_SINCE_LEAK_PERIOD)); + checkArgument(componentUuids.size() == 1, "One and only one component must be provided when searching since leak period"); ComponentDto component = componentUuids.iterator().next(); - Date createdAfterFromSnapshot = findCreatedAfterFromComponentUuid(dbSession, component); - setCreatedAfterFromDates(builder, createdAfterFromSnapshot, createdInLast, false); + + if (!QUALIFIERS_WITHOUT_LEAK_PERIOD.contains(component.qualifier()) && request.getPullRequest() == null) { + Date createdAfterFromSnapshot = findCreatedAfterFromComponentUuid(dbSession, component); + setCreatedAfterFromDates(builder, createdAfterFromSnapshot, null, false); + } } } - @CheckForNull private Date findCreatedAfterFromComponentUuid(DbSession dbSession, ComponentDto component) { Optional snapshot = dbClient.snapshotDao().selectLastAnalysisByComponentUuid(dbSession, component.uuid()); - return snapshot.map(s -> longToDate(s.getPeriodDate())).orElse(null); + // if last analysis has no period date, then no issue should be considered new. + return snapshot.map(s -> longToDate(s.getPeriodDate())).orElseGet(() -> new Date(clock.millis())); } private boolean mergeDeprecatedComponentParameters(DbSession session, SearchRequest request, List allComponents) { @@ -319,7 +323,7 @@ public class IssueQueryFactory { } private void addCreatedAfterByProjects(IssueQuery.Builder builder, DbSession dbSession, SearchRequest request, Set applicationUuids) { - if (request.getSinceLeakPeriod() == null || !request.getSinceLeakPeriod()) { + if (request.getSinceLeakPeriod() == null || !request.getSinceLeakPeriod() || request.getPullRequest() != null) { return; } @@ -331,6 +335,7 @@ public class IssueQueryFactory { .stream() .filter(s -> s.getPeriodDate() != null) .collect(uniqueIndex(SnapshotDto::getComponentUuid, s -> new PeriodStart(longToDate(s.getPeriodDate()), false))); + builder.createdAfterByProjectUuids(leakByProjects); } diff --git a/server/sonar-webserver-es/src/test/java/org/sonar/server/issue/index/IssueQueryFactoryTest.java b/server/sonar-webserver-es/src/test/java/org/sonar/server/issue/index/IssueQueryFactoryTest.java index 050eb283ae7..c86fb938571 100644 --- a/server/sonar-webserver-es/src/test/java/org/sonar/server/issue/index/IssueQueryFactoryTest.java +++ b/server/sonar-webserver-es/src/test/java/org/sonar/server/issue/index/IssueQueryFactoryTest.java @@ -24,6 +24,7 @@ import java.time.ZoneOffset; import java.util.ArrayList; import java.util.Collections; import java.util.Date; +import java.util.Map; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -43,6 +44,7 @@ import org.sonar.server.tester.UserSessionRule; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.tuple; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -276,6 +278,7 @@ public class IssueQueryFactoryTest { @Test public void application_search_project_issues_on_leak() { Date now = new Date(); + when(clock.millis()).thenReturn(now.getTime()); ComponentDto project1 = db.components().insertPublicProject(); SnapshotDto analysis1 = db.components().insertSnapshot(project1, s -> s.setPeriodDate(addDays(now, -14).getTime())); ComponentDto project2 = db.components().insertPublicProject(); @@ -292,8 +295,8 @@ public class IssueQueryFactoryTest { .setSinceLeakPeriod(true)); assertThat(result.createdAfterByProjectUuids()).hasSize(1); - assertThat(result.createdAfterByProjectUuids().get(project1.uuid()).date().getTime()).isEqualTo(analysis1.getPeriodDate()); - assertThat(result.createdAfterByProjectUuids().get(project1.uuid()).inclusive()).isFalse(); + assertThat(result.createdAfterByProjectUuids().entrySet()).extracting(Map.Entry::getKey, e -> e.getValue().date(), e -> e.getValue().inclusive()).containsOnly( + tuple(project1.uuid(), new Date(analysis1.getPeriodDate()), false)); assertThat(result.viewUuids()).containsExactlyInAnyOrder(application.uuid()); } @@ -392,14 +395,14 @@ public class IssueQueryFactoryTest { assertThat(underTest.create(new SearchRequest() .setProjectKeys(singletonList(branch.getKey())) .setBranch(branch.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(project.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(project.uuid()), false); assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(branch.getKey())) .setBranch(branch.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(project.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(project.uuid()), false); } @Test @@ -411,22 +414,22 @@ public class IssueQueryFactoryTest { assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(file.getKey())) .setBranch(branch.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(file.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(file.uuid()), false); assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(branch.getKey())) .setFileUuids(singletonList(file.uuid())) .setBranch(branch.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(file.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(file.uuid()), false); assertThat(underTest.create(new SearchRequest() .setProjectKeys(singletonList(branch.getKey())) .setFileUuids(singletonList(file.uuid())) .setBranch(branch.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(file.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.fileUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(file.uuid()), false); } @Test @@ -439,8 +442,8 @@ public class IssueQueryFactoryTest { .setComponentKeys(singletonList(file.getKey())) .setBranch(branch.getBranch()) .setOnComponentOnly(true))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.componentUuids()), IssueQuery::isMainBranch) - .containsOnly(branch.uuid(), singletonList(file.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.componentUuids()), IssueQuery::isMainBranch) + .containsOnly(branch.uuid(), singletonList(file.uuid()), false); } @Test @@ -451,13 +454,13 @@ public class IssueQueryFactoryTest { assertThat(underTest.create(new SearchRequest() .setProjectKeys(singletonList(project.getKey())) .setBranch("master"))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(project.uuid(), singletonList(project.uuid()), true); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(project.uuid(), singletonList(project.uuid()), true); assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(project.getKey())) .setBranch("master"))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(project.uuid(), singletonList(project.uuid()), true); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(project.uuid(), singletonList(project.uuid()), true); } @Test @@ -492,16 +495,16 @@ public class IssueQueryFactoryTest { assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(applicationBranch1.getKey())) .setBranch(applicationBranch1.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(applicationBranch1.uuid(), Collections.emptyList(), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(applicationBranch1.uuid(), Collections.emptyList(), false); // Search on project1Branch1 assertThat(underTest.create(new SearchRequest() .setComponentKeys(singletonList(applicationBranch1.getKey())) .setProjectKeys(singletonList(project1.getKey())) .setBranch(applicationBranch1.getBranch()))) - .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) - .containsOnly(applicationBranch1.uuid(), singletonList(project1.uuid()), false); + .extracting(IssueQuery::branchUuid, query -> new ArrayList<>(query.projectUuids()), IssueQuery::isMainBranch) + .containsOnly(applicationBranch1.uuid(), singletonList(project1.uuid()), false); } @Test -- cgit v1.2.3