From 8ec822898f98c85194b9394e347f1cf68805cbb9 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Fri, 2 Feb 2018 15:32:07 +0100 Subject: [PATCH] SONAR-10257 Fix leak period date filters --- .../org/sonar/server/issue/IssueQuery.java | 45 ++++++++++++++----- .../sonar/server/issue/IssueQueryFactory.java | 28 ++++++------ .../sonar/server/issue/index/IssueIndex.java | 21 +++++---- .../server/issue/IssueQueryFactoryTest.java | 38 +++++++++++++--- .../sonar/server/issue/IssueQueryTest.java | 6 ++- .../server/issue/index/IssueIndexTest.java | 12 ++--- 6 files changed, 105 insertions(+), 45 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java index 72c0905cbed..b8379edacd3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java @@ -70,12 +70,12 @@ public class IssueQuery { private final Collection languages; private final Collection tags; private final Collection types; - private final Map createdAfterByProjectUuids; + private final Map createdAfterByProjectUuids; private final Boolean onComponentOnly; private final Boolean assigned; private final Boolean resolved; private final Date createdAt; - private final Date createdAfter; + private final PeriodStart createdAfter; private final Date createdBefore; private final String sort; private final Boolean asc; @@ -187,7 +187,7 @@ public class IssueQuery { return types; } - public Map createdAfterByProjectUuids() { + public Map createdAfterByProjectUuids() { return createdAfterByProjectUuids; } @@ -207,8 +207,8 @@ public class IssueQuery { } @CheckForNull - public Date createdAfter() { - return createdAfter == null ? null : new Date(createdAfter.getTime()); + public PeriodStart createdAfter() { + return createdAfter; } @CheckForNull @@ -280,12 +280,12 @@ public class IssueQuery { private Collection languages; private Collection tags; private Collection types; - private Map createdAfterByProjectUuids; + private Map createdAfterByProjectUuids; private Boolean onComponentOnly = false; private Boolean assigned = null; private Boolean resolved = null; private Date createdAt; - private Date createdAfter; + private PeriodStart createdAfter; private Date createdBefore; private String sort; private Boolean asc = false; @@ -384,7 +384,7 @@ public class IssueQuery { return this; } - public Builder createdAfterByProjectUuids(@Nullable Map createdAfterByProjectUuids) { + public Builder createdAfterByProjectUuids(@Nullable Map createdAfterByProjectUuids) { this.createdAfterByProjectUuids = createdAfterByProjectUuids; return this; } @@ -422,7 +422,12 @@ public class IssueQuery { } public Builder createdAfter(@Nullable Date d) { - this.createdAfter = d == null ? null : new Date(d.getTime()); + this.createdAfter(d, true); + return this; + } + + public Builder createdAfter(@Nullable Date d, boolean inclusive) { + this.createdAfter = d == null ? null : new PeriodStart(new Date(d.getTime()), inclusive); return this; } @@ -481,8 +486,28 @@ public class IssueQuery { return c == null ? Collections.emptyList() : Collections.unmodifiableCollection(c); } - private static Map defaultMap(@Nullable Map map) { + private static Map defaultMap(@Nullable Map map) { return map == null ? Collections.emptyMap() : Collections.unmodifiableMap(map); } + public static class PeriodStart { + private final Date date; + private final boolean inclusive; + + public PeriodStart(Date date, boolean inclusive) { + this.date = date; + this.inclusive = inclusive; + + } + + public Date date() { + return date; + } + + public boolean inclusive() { + return inclusive; + } + + } + } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java index fe3a088a0e6..8af0aff307a 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java @@ -52,6 +52,7 @@ import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; import org.sonar.db.component.SnapshotDto; import org.sonar.db.organization.OrganizationDto; +import org.sonar.server.issue.IssueQuery.PeriodStart; import org.sonar.server.user.UserSession; import static com.google.common.base.Preconditions.checkArgument; @@ -121,7 +122,7 @@ public class IssueQueryFactory { boolean effectiveOnComponentOnly = mergeDeprecatedComponentParameters(dbSession, request, allComponents); addComponentParameters(builder, dbSession, effectiveOnComponentOnly, allComponents, request); - builder.createdAfter(buildCreatedAfterFromRequest(dbSession, request, allComponents)); + setCreatedAfterFromRequest(dbSession, builder, request, allComponents); String sort = request.getSort(); if (!Strings.isNullOrEmpty(sort)) { builder.sort(sort); @@ -131,8 +132,7 @@ public class IssueQueryFactory { } } - @CheckForNull - private Date buildCreatedAfterFromDates(@Nullable Date createdAfter, @Nullable String createdInLast) { + 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; @@ -142,7 +142,7 @@ public class IssueQueryFactory { .minus(Period.parse("P" + createdInLast.toUpperCase(Locale.ENGLISH))) .toInstant()); } - return actualCreatedAfter; + builder.createdAfter(actualCreatedAfter, createdAfterInclusive); } @CheckForNull @@ -154,19 +154,19 @@ public class IssueQueryFactory { return organization.map(OrganizationDto::getUuid).orElse(UNKNOWN); } - private Date buildCreatedAfterFromRequest(DbSession dbSession, SearchRequest request, List componentUuids) { + private void setCreatedAfterFromRequest(DbSession dbSession, IssueQuery.Builder builder, SearchRequest request, List componentUuids) { Date createdAfter = parseStartingDateOrDateTime(request.getCreatedAfter()); String createdInLast = request.getCreatedInLast(); if (request.getSinceLeakPeriod() == null || !request.getSinceLeakPeriod()) { - return buildCreatedAfterFromDates(createdAfter, createdInLast); + setCreatedAfterFromDates(builder, createdAfter, createdInLast, true); + } else { + checkRequest(createdAfter == null, "Parameters '%s' and '%s' cannot be set simultaneously", PARAM_CREATED_AFTER, 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); } - - checkRequest(createdAfter == null, "Parameters '%s' and '%s' cannot be set simultaneously", PARAM_CREATED_AFTER, 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); - return buildCreatedAfterFromDates(createdAfterFromSnapshot, createdInLast); } @CheckForNull @@ -328,10 +328,10 @@ public class IssueQueryFactory { .flatMap(app -> dbClient.componentDao().selectProjectsFromView(dbSession, app, app).stream()) .collect(toSet()); - Map leakByProjects = dbClient.snapshotDao().selectLastAnalysesByRootComponentUuids(dbSession, projectUuids) + Map leakByProjects = dbClient.snapshotDao().selectLastAnalysesByRootComponentUuids(dbSession, projectUuids) .stream() .filter(s -> s.getPeriodDate() != null) - .collect(uniqueIndex(SnapshotDto::getComponentUuid, s -> longToDate(s.getPeriodDate()))); + .collect(uniqueIndex(SnapshotDto::getComponentUuid, s -> new PeriodStart(longToDate(s.getPeriodDate()), false))); builder.createdAfterByProjectUuids(leakByProjects); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java index f9584f9b10d..7e2a89645e8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java @@ -42,6 +42,7 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.index.query.RangeQueryBuilder; import org.elasticsearch.indices.TermsLookup; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregationBuilders; @@ -73,6 +74,7 @@ import org.sonar.server.es.SearchOptions; import org.sonar.server.es.Sorting; import org.sonar.server.es.StickyFacetBuilder; import org.sonar.server.issue.IssueQuery; +import org.sonar.server.issue.IssueQuery.PeriodStart; import org.sonar.server.permission.index.AuthorizationTypeSupport; import org.sonar.server.user.UserSession; import org.sonar.server.view.index.ViewIndexDefinition; @@ -427,15 +429,15 @@ public class IssueIndex { } private void addDatesFilter(Map filters, IssueQuery query) { - Date createdAfter = query.createdAfter(); + PeriodStart createdAfter = query.createdAfter(); Date createdBefore = query.createdBefore(); - validateCreationDateBounds(createdBefore, createdAfter); + validateCreationDateBounds(createdBefore, createdAfter != null ? createdAfter.date() : null); if (createdAfter != null) { filters.put("__createdAfter", QueryBuilders .rangeQuery(IssueIndexDefinition.FIELD_ISSUE_FUNC_CREATED_AT) - .gte(BaseDoc.dateToEpochSeconds(createdAfter))); + .from(BaseDoc.dateToEpochSeconds(createdAfter.date()), createdAfter.inclusive())); } if (createdBefore != null) { filters.put("__createdBefore", QueryBuilders @@ -449,11 +451,11 @@ public class IssueIndex { } private static void addCreatedAfterByProjectsFilter(Map filters, IssueQuery query) { - Map createdAfterByProjectUuids = query.createdAfterByProjectUuids(); + Map createdAfterByProjectUuids = query.createdAfterByProjectUuids(); BoolQueryBuilder boolQueryBuilder = boolQuery(); createdAfterByProjectUuids.forEach((projectUuid, createdAfterDate) -> boolQueryBuilder.should(boolQuery() .filter(termQuery(IssueIndexDefinition.FIELD_ISSUE_PROJECT_UUID, projectUuid)) - .filter(rangeQuery(IssueIndexDefinition.FIELD_ISSUE_FUNC_CREATED_AT).gte(BaseDoc.dateToEpochSeconds(createdAfterDate))))); + .filter(rangeQuery(IssueIndexDefinition.FIELD_ISSUE_FUNC_CREATED_AT).from(BaseDoc.dateToEpochSeconds(createdAfterDate.date()), createdAfterDate.inclusive())))); filters.put("createdAfterByProjectUuids", boolQueryBuilder); } @@ -513,15 +515,18 @@ public class IssueIndex { private Optional getCreatedAtFacet(IssueQuery query, Map filters, QueryBuilder esQuery) { long startTime; - Date createdAfter = query.createdAfter(); + boolean startInclusive; + PeriodStart createdAfter = query.createdAfter(); if (createdAfter == null) { Optional minDate = getMinCreatedAt(filters, esQuery); if (!minDate.isPresent()) { return Optional.empty(); } startTime = minDate.get(); + startInclusive = true; } else { - startTime = createdAfter.getTime(); + startTime = createdAfter.date().getTime(); + startInclusive = createdAfter.inclusive(); } Date createdBefore = query.createdBefore(); long endTime = createdBefore == null ? system.now() : createdBefore.getTime(); @@ -543,7 +548,7 @@ public class IssueIndex { .format(DateUtils.DATETIME_FORMAT) .timeZone(DateTimeZone.forOffsetMillis(system.getDefaultTimeZone().getRawOffset())) // ES dateHistogram bounds are inclusive while createdBefore parameter is exclusive - .extendedBounds(new ExtendedBounds(startTime, endTime - 1L)); + .extendedBounds(new ExtendedBounds(startInclusive ? startTime : startTime + 1, endTime - 1L)); addEffortAggregationIfNeeded(query, dateHistogram); return Optional.of(dateHistogram); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryFactoryTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryFactoryTest.java index 2679ff7d48f..04e8edffc61 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryFactoryTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryFactoryTest.java @@ -22,6 +22,7 @@ package org.sonar.server.issue; import java.time.Clock; import java.time.ZoneOffset; import java.util.ArrayList; +import java.util.Collections; import java.util.Date; import org.junit.Rule; import org.junit.Test; @@ -108,12 +109,35 @@ public class IssueQueryFactoryTest { assertThat(query.assigned()).isTrue(); assertThat(query.rules()).hasSize(2); assertThat(query.directories()).containsOnly("aDirPath"); - assertThat(query.createdAfter()).isEqualTo(DateUtils.parseDateTime("2013-04-16T09:08:24+0200")); + assertThat(query.createdAfter().date()).isEqualTo(DateUtils.parseDateTime("2013-04-16T09:08:24+0200")); + assertThat(query.createdAfter().inclusive()).isTrue(); assertThat(query.createdBefore()).isEqualTo(DateUtils.parseDateTime("2013-04-17T09:08:24+0200")); assertThat(query.sort()).isEqualTo(IssueQuery.SORT_BY_CREATION_DATE); assertThat(query.asc()).isTrue(); } + @Test + public void leak_period_start_date_is_exclusive() { + long leakPeriodStart = addDays(new Date(), -14).getTime(); + + ComponentDto project = db.components().insertPublicProject(); + ComponentDto file = db.components().insertComponent(newFileDto(project)); + + SnapshotDto analysis = db.components().insertSnapshot(project, s -> s.setPeriodDate(leakPeriodStart)); + + SearchRequest request = new SearchRequest() + .setComponentUuids(Collections.singletonList(file.uuid())) + .setOnComponentOnly(true) + .setSinceLeakPeriod(true); + + IssueQuery query = underTest.create(request); + + assertThat(query.componentUuids()).containsOnly(file.uuid()); + assertThat(query.createdAfter().date()).isEqualTo(new Date(leakPeriodStart)); + assertThat(query.createdAfter().inclusive()).isFalse(); + + } + @Test public void dates_are_inclusive() { SearchRequest request = new SearchRequest() @@ -122,7 +146,8 @@ public class IssueQueryFactoryTest { IssueQuery query = underTest.create(request); - assertThat(query.createdAfter()).isEqualTo(DateUtils.parseDate("2013-04-16")); + assertThat(query.createdAfter().date()).isEqualTo(DateUtils.parseDate("2013-04-16")); + assertThat(query.createdAfter().inclusive()).isTrue(); assertThat(query.createdBefore()).isEqualTo(DateUtils.parseDate("2013-04-18")); } @@ -276,8 +301,9 @@ public class IssueQueryFactoryTest { .setComponentUuids(singletonList(application.uuid())) .setSinceLeakPeriod(true)); - assertThat(result.createdAfterByProjectUuids()).containsOnly( - entry(project1.uuid(), new Date(analysis1.getPeriodDate()))); + 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.viewUuids()).containsExactlyInAnyOrder(application.uuid()); } @@ -465,7 +491,9 @@ public class IssueQueryFactoryTest { when(clock.getZone()).thenReturn(ZoneOffset.UTC); SearchRequest request = new SearchRequest() .setCreatedInLast("1y2m3w4d"); - assertThat(underTest.create(request).createdAfter()).isEqualTo(DateUtils.parseDateTime("2012-04-30T07:35:00+0100")); + assertThat(underTest.create(request).createdAfter().date()).isEqualTo(DateUtils.parseDateTime("2012-04-30T07:35:00+0100")); + assertThat(underTest.create(request).createdAfter().inclusive()).isTrue(); + } @Test diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java index f03dcc74c76..ab5d9d6f0c9 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java @@ -26,6 +26,7 @@ import org.junit.Test; import org.sonar.api.issue.Issue; import org.sonar.api.rule.RuleKey; import org.sonar.api.rule.Severity; +import org.sonar.server.issue.IssueQuery.PeriodStart; import static com.google.common.collect.Lists.newArrayList; import static org.assertj.core.api.Assertions.assertThat; @@ -35,6 +36,7 @@ public class IssueQueryTest { @Test public void build_query() { + PeriodStart filterDate = new IssueQuery.PeriodStart(new Date(10_000_000_000L),false); IssueQuery query = IssueQuery.builder() .issueKeys(newArrayList("ABCDE")) .severities(newArrayList(Severity.BLOCKER)) @@ -50,7 +52,7 @@ public class IssueQueryTest { .types(newArrayList("RELIABILITY", "SECURITY")) .organizationUuid("orga") .branchUuid("my_branch") - .createdAfterByProjectUuids(ImmutableMap.of("PROJECT", new Date(10_000_000_000L))) + .createdAfterByProjectUuids(ImmutableMap.of("PROJECT", filterDate)) .assigned(true) .createdAfter(new Date()) .createdBefore(new Date()) @@ -72,7 +74,7 @@ public class IssueQueryTest { assertThat(query.types()).containsOnly("RELIABILITY", "SECURITY"); assertThat(query.organizationUuid()).isEqualTo("orga"); assertThat(query.branchUuid()).isEqualTo("my_branch"); - assertThat(query.createdAfterByProjectUuids()).containsOnly(entry("PROJECT", new Date(10_000_000_000L))); + assertThat(query.createdAfterByProjectUuids()).containsOnly(entry("PROJECT", filterDate)); assertThat(query.assigned()).isTrue(); assertThat(query.rules()).containsOnly(RuleKey.of("squid", "AvoidCycle")); assertThat(query.createdAfter()).isNotNull(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java index c641177397d..3f4f70dc8a3 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java @@ -327,27 +327,27 @@ public class IssueIndexTest { // Search for issues of project 1 having less than 15 days assertThatSearchReturnsOnly(IssueQuery.builder() - .createdAfterByProjectUuids(ImmutableMap.of(project1.uuid(), addDays(now, -15))), + .createdAfterByProjectUuids(ImmutableMap.of(project1.uuid(), new IssueQuery.PeriodStart(addDays(now, -15), true))), project1Issue1.key()); // Search for issues of project 1 having less than 14 days and project 2 having less then 25 days assertThatSearchReturnsOnly(IssueQuery.builder() .createdAfterByProjectUuids(ImmutableMap.of( - project1.uuid(), addDays(now, -14), - project2.uuid(), addDays(now, -25))), + project1.uuid(), new IssueQuery.PeriodStart(addDays(now, -14), true), + project2.uuid(), new IssueQuery.PeriodStart(addDays(now, -25), true))), project1Issue1.key(), project2Issue1.key()); // Search for issues of project 1 having less than 30 days assertThatSearchReturnsOnly(IssueQuery.builder() .createdAfterByProjectUuids(ImmutableMap.of( - project1.uuid(), addDays(now, -30))), + project1.uuid(), new IssueQuery.PeriodStart(addDays(now, -30), true))), project1Issue1.key(), project1Issue2.key()); // Search for issues of project 1 and project 2 having less than 5 days assertThatSearchReturnsOnly(IssueQuery.builder() .createdAfterByProjectUuids(ImmutableMap.of( - project1.uuid(), addDays(now, -5), - project2.uuid(), addDays(now, -5)))); + project1.uuid(), new IssueQuery.PeriodStart(addDays(now, -5), true), + project2.uuid(), new IssueQuery.PeriodStart(addDays(now, -5), true)))); } @Test -- 2.39.5