From 2a95ecb7c1807024f62038a7ebd0dc25982763b3 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 25 Feb 2020 12:00:23 +0000 Subject: [PATCH] Don't index ranges including NOW in percolator (#52748) Currently, date ranges queries using NOW-based date math are rewritten to MatchAllDocs queries when being preprocessed for the percolator. However, since we added the verification step, this can result in incorrect matches when percolator queries are run without scores. This commit changes things to instead wrap date queries that use NOW with a new DateRangeIncludingNowQuery. This is a simple wrapper query that returns its delegate at rewrite time, but it can be detected by the percolator QueryAnalyzer and be dealt with accordingly. This also allows us to remove a method on QueryRewriteContext, and push all logic relating to NOW-based ranges into the DateFieldMapper. Fixes #52617 --- .../percolator/PercolatorFieldMapper.java | 9 +-- .../percolator/QueryAnalyzer.java | 5 ++ .../PercolatorFieldMapperTests.java | 11 +++ .../percolator/PercolatorQuerySearchIT.java | 33 +++++++++ .../index/mapper/DateFieldMapper.java | 13 +++- .../query/DateRangeIncludingNowQuery.java | 74 +++++++++++++++++++ .../index/query/QueryRewriteContext.java | 11 --- .../index/query/RangeQueryBuilder.java | 10 --- .../index/mapper/DateFieldTypeTests.java | 10 +++ .../index/query/RangeQueryBuilderTests.java | 31 +------- 10 files changed, 147 insertions(+), 60 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/query/DateRangeIncludingNowQuery.java diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 501d71465d679..e22eed78a6a52 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -407,14 +407,7 @@ public void parse(ParseContext context) throws IOException { Version indexVersion = context.mapperService().getIndexSettings().getIndexVersionCreated(); createQueryBuilderField(indexVersion, queryBuilderField, queryBuilder, context); - QueryBuilder queryBuilderForProcessing = queryBuilder.rewrite(new QueryShardContext(queryShardContext) { - - @Override - public boolean convertNowRangeToMatchAll() { - return true; - } - }); - Query query = toQuery(queryShardContext, isMapUnmappedFieldAsText(), queryBuilderForProcessing); + Query query = toQuery(queryShardContext, isMapUnmappedFieldAsText(), queryBuilder); processQuery(query, context); } diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java index 800706b74b9d3..db7020cf8176c 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java @@ -40,6 +40,7 @@ import org.apache.lucene.util.NumericUtils; import org.elasticsearch.Version; import org.elasticsearch.common.lucene.search.function.FunctionScoreQuery; +import org.elasticsearch.index.query.DateRangeIncludingNowQuery; import java.util.ArrayList; import java.util.Arrays; @@ -149,6 +150,10 @@ Result getResult() { @Override public QueryVisitor getSubVisitor(Occur occur, Query parent) { + if (parent instanceof DateRangeIncludingNowQuery) { + terms.add(Result.UNKNOWN); + return QueryVisitor.EMPTY_VISITOR; + } this.verified = isVerified(parent); if (occur == Occur.MUST || occur == Occur.FILTER) { ResultBuilder builder = new ResultBuilder(true); diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java index 658981e6251e1..f41c4c400e2dd 100644 --- a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java +++ b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java @@ -490,6 +490,17 @@ public void testPercolatorFieldMapper() throws Exception { assertThat(doc.rootDoc().getFields(fieldType.queryBuilderField.name()).length, equalTo(1)); qbSource = doc.rootDoc().getFields(fieldType.queryBuilderField.name())[0].binaryValue(); assertQueryBuilder(qbSource, queryBuilder); + + queryBuilder = rangeQuery("date_field").from("now"); + doc = mapperService.documentMapper().parse(new SourceToParse("test", "1", BytesReference.bytes(XContentFactory + .jsonBuilder() + .startObject() + .field(fieldName, queryBuilder) + .endObject()), + XContentType.JSON)); + assertThat(doc.rootDoc().getFields(fieldType.extractionResultField.name()).length, equalTo(1)); + assertThat(doc.rootDoc().getFields(fieldType.extractionResultField.name())[0].stringValue(), + equalTo(EXTRACTION_FAILED)); } public void testStoringQueries() throws Exception { diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java index cbd319d342c94..ce8e871e51de3 100644 --- a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java +++ b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java @@ -48,6 +48,7 @@ import static org.elasticsearch.common.xcontent.XContentFactory.smileBuilder; import static org.elasticsearch.common.xcontent.XContentFactory.yamlBuilder; import static org.elasticsearch.index.query.QueryBuilders.boolQuery; +import static org.elasticsearch.index.query.QueryBuilders.constantScoreQuery; import static org.elasticsearch.index.query.QueryBuilders.geoBoundingBoxQuery; import static org.elasticsearch.index.query.QueryBuilders.geoDistanceQuery; import static org.elasticsearch.index.query.QueryBuilders.geoPolygonQuery; @@ -930,4 +931,36 @@ public void testDisallowExpensiveQueries() throws IOException { assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); } } + + public void testWrappedWithConstantScore() throws Exception { + + assertAcked(client().admin().indices().prepareCreate("test") + .setMapping("d", "type=date", "q", "type=percolator") + ); + + client().prepareIndex("test").setId("1") + .setSource(jsonBuilder().startObject().field("q", + boolQuery().must(rangeQuery("d").gt("now")) + ).endObject()) + .execute().actionGet(); + + client().prepareIndex("test").setId("2") + .setSource(jsonBuilder().startObject().field("q", + boolQuery().must(rangeQuery("d").lt("now")) + ).endObject()) + .execute().actionGet(); + + client().admin().indices().prepareRefresh().get(); + + SearchResponse response = client().prepareSearch("test").setQuery(new PercolateQueryBuilder("q", + BytesReference.bytes(jsonBuilder().startObject().field("d", "2020-02-01T15:00:00.000+11:00").endObject()), + XContentType.JSON)).get(); + assertEquals(1, response.getHits().getTotalHits().value); + + response = client().prepareSearch("test").setQuery(constantScoreQuery(new PercolateQueryBuilder("q", + BytesReference.bytes(jsonBuilder().startObject().field("d", "2020-02-01T15:00:00.000+11:00").endObject()), + XContentType.JSON))).get(); + assertEquals(1, response.getHits().getTotalHits().value); + + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index a42a07a3d9e0b..87d489d04b556 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -49,6 +49,7 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType; import org.elasticsearch.index.fielddata.plain.DocValuesIndexFieldData; +import org.elasticsearch.index.query.DateRangeIncludingNowQuery; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; @@ -389,11 +390,16 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower DateMathParser parser = forcedDateParser == null ? dateMathParser : forcedDateParser; + boolean[] nowUsed = new boolean[1]; + LongSupplier nowSupplier = () -> { + nowUsed[0] = true; + return context.nowInMillis(); + }; long l, u; if (lowerTerm == null) { l = Long.MIN_VALUE; } else { - l = parseToLong(lowerTerm, !includeLower, timeZone, parser, context::nowInMillis); + l = parseToLong(lowerTerm, !includeLower, timeZone, parser, nowSupplier); if (includeLower == false) { ++l; } @@ -401,7 +407,7 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower if (upperTerm == null) { u = Long.MAX_VALUE; } else { - u = parseToLong(upperTerm, includeUpper, timeZone, parser, context::nowInMillis); + u = parseToLong(upperTerm, includeUpper, timeZone, parser, nowSupplier); if (includeUpper == false) { --u; } @@ -411,6 +417,9 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower Query dvQuery = SortedNumericDocValuesField.newSlowRangeQuery(name(), l, u); query = new IndexOrDocValuesQuery(query, dvQuery); } + if (nowUsed[0]) { + query = new DateRangeIncludingNowQuery(query); + } return query; } diff --git a/server/src/main/java/org/elasticsearch/index/query/DateRangeIncludingNowQuery.java b/server/src/main/java/org/elasticsearch/index/query/DateRangeIncludingNowQuery.java new file mode 100644 index 0000000000000..e3db9cf66d84f --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/query/DateRangeIncludingNowQuery.java @@ -0,0 +1,74 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.query; + +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.search.BooleanClause; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.QueryVisitor; + +import java.io.IOException; +import java.util.Objects; + +/** + * A simple wrapper class that indicates that the wrapped query has made use of NOW + * when parsing its datemath. Useful for preprocessors such as the percolator that + * need to know when not to extract dates from the query. + */ +public class DateRangeIncludingNowQuery extends Query { + + private final Query in; + + public DateRangeIncludingNowQuery(Query in) { + this.in = in; + } + + public Query getQuery() { + return in; + } + + @Override + public Query rewrite(IndexReader reader) throws IOException { + return in; + } + + @Override + public String toString(String field) { + return "DateRangeIncludingNowQuery(" + in + ")"; + } + + @Override + public void visit(QueryVisitor visitor) { + in.visit(visitor.getSubVisitor(BooleanClause.Occur.MUST, this)); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + DateRangeIncludingNowQuery that = (DateRangeIncludingNowQuery) o; + return Objects.equals(in, that.in); + } + + @Override + public int hashCode() { + return Objects.hash(in); + } +} diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java b/server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java index b44f0edfebbc4..ead32047e113d 100644 --- a/server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java +++ b/server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java @@ -125,15 +125,4 @@ public void onFailure(Exception e) { } } - /** - * In pre-processing contexts that happen at index time 'now' date ranges should be replaced by a {@link MatchAllQueryBuilder}. - * Otherwise documents that should match at query time would never match and the document that have fallen outside the - * date range would continue to match. - * - * @return indicates whether range queries with date ranges using 'now' are rewritten to a {@link MatchAllQueryBuilder}. - */ - public boolean convertNowRangeToMatchAll() { - return false; - } - } diff --git a/server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java index 0219c782d06af..156fa27264e1f 100644 --- a/server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java @@ -451,16 +451,6 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC @Override protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { - // Percolator queries get rewritten and pre-processed at index time. - // If a range query has a date range using 'now' and 'now' gets resolved at index time then - // the pre-processing uses that to pre-process. This can then lead to mismatches at query time. - if (queryRewriteContext.convertNowRangeToMatchAll()) { - if ((from() != null && from().toString().contains("now")) || - (to() != null && to().toString().contains("now"))) { - return new MatchAllQueryBuilder(); - } - } - final MappedFieldType.Relation relation = getRelation(queryRewriteContext); switch (relation) { case DISJOINT: diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java index 011d5ae3ef6c1..82302c0493669 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java @@ -48,6 +48,7 @@ import org.elasticsearch.index.mapper.DateFieldMapper.Resolution; import org.elasticsearch.index.mapper.MappedFieldType.Relation; import org.elasticsearch.index.mapper.ParseContext.Document; +import org.elasticsearch.index.query.DateRangeIncludingNowQuery; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; import org.joda.time.DateTimeZone; @@ -269,6 +270,15 @@ BigArrays.NON_RECYCLING_INSTANCE, null, null, null, null, null, xContentRegistry assertEquals(expected, ft.rangeQuery(date1, date2, true, true, null, null, null, context).rewrite(new MultiReader())); + instant1 = nowInMillis; + instant2 = instant1 + 100; + expected = new DateRangeIncludingNowQuery(new IndexOrDocValuesQuery( + LongPoint.newRangeQuery("field", instant1, instant2), + SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2) + )); + assertEquals(expected, + ft.rangeQuery("now", instant2, true, true, null, null, null, context)); + ft.setIndexOptions(IndexOptions.NONE); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ft.rangeQuery(date1, date2, true, true, null, null, null, context)); diff --git a/server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java index df1bf4910e76e..dfa19d1623d5d 100644 --- a/server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java @@ -343,6 +343,8 @@ public void testDateRangeQueryTimezone() throws IOException { "}"; QueryShardContext context = createShardContext(); Query parsedQuery = parseQuery(query).toQuery(context); + assertThat(parsedQuery, instanceOf(DateRangeIncludingNowQuery.class)); + parsedQuery = ((DateRangeIncludingNowQuery)parsedQuery).getQuery(); assertThat(parsedQuery, instanceOf(IndexOrDocValuesQuery.class)); parsedQuery = ((IndexOrDocValuesQuery) parsedQuery).getIndexQuery(); assertThat(parsedQuery, instanceOf(PointRangeQuery.class)); @@ -565,35 +567,6 @@ public void testParseRelation() { assertEquals(ShapeRelation.INTERSECTS, builder.relation()); } - public void testConvertNowRangeToMatchAll() throws IOException { - RangeQueryBuilder query = new RangeQueryBuilder(DATE_FIELD_NAME); - DateTime queryFromValue = new DateTime(2019, 1, 1, 0, 0, 0, ISOChronology.getInstanceUTC()); - DateTime queryToValue = new DateTime(2020, 1, 1, 0, 0, 0, ISOChronology.getInstanceUTC()); - if (randomBoolean()) { - query.from("now"); - query.to(queryToValue); - } else if (randomBoolean()) { - query.from(queryFromValue); - query.to("now"); - } else { - query.from("now"); - query.to("now+1h"); - } - QueryShardContext queryShardContext = createShardContext(); - QueryBuilder rewritten = query.rewrite(queryShardContext); - assertThat(rewritten, instanceOf(RangeQueryBuilder.class)); - - queryShardContext = new QueryShardContext(queryShardContext) { - - @Override - public boolean convertNowRangeToMatchAll() { - return true; - } - }; - rewritten = query.rewrite(queryShardContext); - assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class)); - } - public void testTypeField() throws IOException { RangeQueryBuilder builder = QueryBuilders.rangeQuery("_type") .from("value1");