From 8c6d9a250399bbc5b3fc28a9fd666f4c70b0cf35 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 29 Mar 2019 14:28:54 -0400 Subject: [PATCH] Remove timezone validation on rollup range queries We enforced the timezone of range queries when using the rollup search endpoint, but this validation is not needed. Since rollup dates are stored in UTC, and range queries are always converted to UTC (even if specifying a `time_zone`) the validation is not needed and can prevent legitimate queries from running. --- .../action/TransportRollupSearchAction.java | 38 +++---------------- .../rollup/action/SearchActionTests.java | 9 ++--- 2 files changed, 10 insertions(+), 37 deletions(-) diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java index 610275705eef8..e85a92c061366 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java @@ -56,17 +56,14 @@ import org.elasticsearch.xpack.core.rollup.RollupField; import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps; import org.elasticsearch.xpack.core.rollup.action.RollupSearchAction; -import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig; import org.elasticsearch.xpack.rollup.Rollup; import org.elasticsearch.xpack.rollup.RollupJobIdentifierUtils; import org.elasticsearch.xpack.rollup.RollupRequestTranslator; import org.elasticsearch.xpack.rollup.RollupResponseTranslator; -import org.joda.time.DateTimeZone; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -286,11 +283,8 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set jobCap } else if (builder.getWriteableName().equals(RangeQueryBuilder.NAME)) { RangeQueryBuilder range = (RangeQueryBuilder) builder; String fieldName = range.fieldName(); - // Many range queries don't include the timezone because the default is UTC, but the query - // builder will return null so we need to set it here - String timeZone = range.timeZone() == null ? DateTimeZone.UTC.toString() : range.timeZone(); - String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName, timeZone); + String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName); RangeQueryBuilder rewritten = new RangeQueryBuilder(rewrittenFieldName) .from(range.from()) .to(range.to()) @@ -306,12 +300,12 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set jobCap } else if (builder.getWriteableName().equals(TermQueryBuilder.NAME)) { TermQueryBuilder term = (TermQueryBuilder) builder; String fieldName = term.fieldName(); - String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null); + String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName); return new TermQueryBuilder(rewrittenFieldName, term.value()); } else if (builder.getWriteableName().equals(TermsQueryBuilder.NAME)) { TermsQueryBuilder terms = (TermsQueryBuilder) builder; String fieldName = terms.fieldName(); - String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null); + String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName); return new TermsQueryBuilder(rewrittenFieldName, terms.values()); } else if (builder.getWriteableName().equals(MatchAllQueryBuilder.NAME)) { // no-op @@ -321,11 +315,7 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set jobCap } } - private static String rewriteFieldName(Set jobCaps, - String builderName, - String fieldName, - String timeZone) { - List incompatibleTimeZones = timeZone == null ? Collections.emptyList() : new ArrayList<>(); + private static String rewriteFieldName(Set jobCaps, String builderName, String fieldName) { List rewrittenFieldNames = jobCaps.stream() // We only care about job caps that have the query's target field .filter(caps -> caps.getFieldCaps().keySet().contains(fieldName)) @@ -335,17 +325,7 @@ private static String rewriteFieldName(Set jobCaps, // For now, we only allow filtering on grouping fields .filter(agg -> { String type = (String)agg.get(RollupField.AGG); - - // If the cap is for a date_histo, and the query is a range, the timezones need to match - if (type.equals(DateHistogramAggregationBuilder.NAME) && timeZone != null) { - boolean matchingTZ = ((String)agg.get(DateHistogramGroupConfig.TIME_ZONE)) - .equalsIgnoreCase(timeZone); - if (matchingTZ == false) { - incompatibleTimeZones.add((String)agg.get(DateHistogramGroupConfig.TIME_ZONE)); - } - return matchingTZ; - } - // Otherwise just make sure it's one of the three groups + // make sure it's one of the three groups return type.equals(TermsAggregationBuilder.NAME) || type.equals(DateHistogramAggregationBuilder.NAME) || type.equals(HistogramAggregationBuilder.NAME); @@ -363,14 +343,8 @@ private static String rewriteFieldName(Set jobCaps, .distinct() .collect(ArrayList::new, List::addAll, List::addAll); if (rewrittenFieldNames.isEmpty()) { - if (incompatibleTimeZones.isEmpty()) { - throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName + throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName + "] query is not available in selected rollup indices, cannot query."); - } else { - throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName - + "] query was found in rollup indices, but requested timezone is not compatible. Options include: " - + incompatibleTimeZones); - } } else if (rewrittenFieldNames.size() > 1) { throw new IllegalArgumentException("Ambiguous field name resolution when mapping to rolled fields. Field name [" + fieldName + "] was mapped to: [" + Strings.collectionToDelimitedString(rewrittenFieldNames, ",") + "]."); diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java index 0032b5a88a563..5a851d17e5eaf 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java @@ -140,16 +140,15 @@ public void testRangeNullTimeZone() { assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp")); } - public void testRangeWrongTZ() { + public void testRangeDifferentTZ() { final GroupConfig groupConfig = new GroupConfig(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, "UTC")); final RollupJobConfig config = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, groupConfig, emptyList(), null); RollupJobCaps cap = new RollupJobCaps(config); Set caps = new HashSet<>(); caps.add(cap); - Exception e = expectThrows(IllegalArgumentException.class, - () -> TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps)); - assertThat(e.getMessage(), equalTo("Field [foo] in [range] query was found in rollup indices, but requested timezone is not " + - "compatible. Options include: [UTC]")); + QueryBuilder rewritten = TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps); + assertThat(rewritten, instanceOf(RangeQueryBuilder.class)); + assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp")); } public void testTermQuery() {