From d340f45b2d4fed7229226227e1f0824bf9b054ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 13 Jun 2019 14:31:19 +0200 Subject: [PATCH 1/3] SimpleQ.S.B and QueryStringQ.S.B tests should avoid `now` in query Currently the randomization of the q.b. in these tests can create query strings that can cause caching to be disabled for this query if we query all fields and there is a date field present. This is pretty much an anomaly that we shouldn't generally test for in the "testToQuery" tests where cache policies are checked. This change makes sure we don't create offending query strings so the cache checks never hit these cases and adds a special test method to check this edge case. Closes #43112 --- .../index/query/FullTextQueryTestCase.java | 1 - .../query/QueryStringQueryBuilderTests.java | 41 ++++++++++++++++++- .../query/SimpleQueryStringBuilderTests.java | 38 ++++++++++++++--- .../test/AbstractQueryTestCase.java | 3 +- 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java b/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java index 7f9d85e3e26b7..85ae9874b67e6 100644 --- a/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java +++ b/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java @@ -28,7 +28,6 @@ import java.util.Set; public abstract class FullTextQueryTestCase> extends AbstractQueryTestCase { - protected abstract boolean isCacheable(QB queryBuilder); /** * Full text queries that start with "now" are not cacheable if they diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index 723417fb85207..42362e593a4ba 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -71,6 +71,7 @@ import java.util.Arrays; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; @@ -84,6 +85,7 @@ import static org.hamcrest.Matchers.instanceOf; public class QueryStringQueryBuilderTests extends FullTextQueryTestCase { + @Override protected boolean isCacheable(QueryStringQueryBuilder queryBuilder) { return queryBuilder.fuzziness() != null @@ -108,8 +110,11 @@ protected QueryStringQueryBuilder doCreateTestQueryBuilder() { int numTerms = randomIntBetween(0, 5); String query = ""; for (int i = 0; i < numTerms; i++) { - //min length 4 makes sure that the text is not an operator (AND/OR) so toQuery won't break - query += (randomBoolean() ? STRING_FIELD_NAME + ":" : "") + randomAlphaOfLengthBetween(4, 10) + " "; + // min length 4 makes sure that the text is not an operator (AND/OR) so toQuery won't break + // also avoid "now" since we might hit dqte fields later and this complicates caching checks + String term = randomValueOtherThanMany(s -> s.toLowerCase(Locale.ROOT).contains("now"), + () -> randomAlphaOfLengthBetween(4, 10)); + query += (randomBoolean() ? STRING_FIELD_NAME + ":" : "") + term + " "; } QueryStringQueryBuilder queryStringQueryBuilder = new QueryStringQueryBuilder(query); if (randomBoolean()) { @@ -1578,4 +1583,36 @@ private void assertQueryWithAllFieldsWildcard(Query query) { assertThat(disjunctionMaxQuery.getDisjuncts(), hasItems(new TermQuery(new Term(STRING_FIELD_NAME, "hello")), new TermQuery(new Term(STRING_FIELD_NAME_2, "hello")))); } + + /** + * Query terms that start with "now" can trigger a query to not be cacheable if it hits all field + * and there is a date field amongst them. This test checks the search context cacheable flag is + * updated accordingly. + * Note: this cannot happen when directly hitting the date field, dates are not parsed leniently then + * and we typically get a parse exception. + */ + public void testCachingStrategiesWithNow() throws IOException { + String query = "now " + randomAlphaOfLengthBetween(4, 10); + QueryStringQueryBuilder queryStringQueryBuilder = new QueryStringQueryBuilder(query); + QueryShardContext context = createShardContext(); + assert context.isCacheable(); + /* + * We use a private rewrite context here since we want the most realistic way of asserting that we are cacheable or not. We do it + * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the + * actual lucene query + */ + QueryBuilder rewritten = rewriteQuery(queryStringQueryBuilder, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertFalse("query was marked as cacheable in the context but it should not be cacheable: " + query.toString(), + context.isCacheable()); + + query = randomFrom("NoW", "nOw", "NOW") + " " + randomAlphaOfLengthBetween(4, 10); + queryStringQueryBuilder = new QueryStringQueryBuilder(query); + + context = createShardContext(); + rewritten = rewriteQuery(queryStringQueryBuilder, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertTrue("query was marked as not cacheable in the context but it should be cacheable: " + query.toString(), + context.isCacheable()); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java index 2ac15af573adf..8195b5f97cb71 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java @@ -65,14 +65,12 @@ import static org.hamcrest.Matchers.notNullValue; public class SimpleQueryStringBuilderTests extends FullTextQueryTestCase { - @Override - protected boolean isCacheable(SimpleQueryStringBuilder queryBuilder) { - return isCacheable(queryBuilder.fields().keySet(), queryBuilder.value()); - } @Override protected SimpleQueryStringBuilder doCreateTestQueryBuilder() { - String queryText = randomAlphaOfLengthBetween(1, 10); + // we avoid strings with "now" since those can have different caching policies that are checked elsewhere + String queryText = randomValueOtherThanMany(s -> s.toLowerCase(Locale.ROOT).contains("now"), + () -> randomAlphaOfLengthBetween(1, 10)); SimpleQueryStringBuilder result = new SimpleQueryStringBuilder(queryText); if (randomBoolean()) { result.analyzeWildcard(randomBoolean()); @@ -786,4 +784,34 @@ private void assertQueryWithAllFieldsWildcard(Query query) { assertThat(disjunctionMaxQuery.getDisjuncts(), hasItems(new TermQuery(new Term(STRING_FIELD_NAME, "hello")), new TermQuery(new Term(STRING_FIELD_NAME_2, "hello")))); } + + /** + * Query terms that start with "now" can trigger a query to not be cacheable if it hits all field + * and there is a date field amongst them. This test checks the search context cacheable flag is + * updated accordingly + * Note: this cannot happen when directly hitting the date field, dates are not parsed leniently then + * and we typically get a parse exception. + */ + public void testCachingStrategiesWithNow() throws IOException { + SimpleQueryStringBuilder query = new SimpleQueryStringBuilder("now" + randomAlphaOfLengthBetween(1, 10)); + QueryShardContext context = createShardContext(); + assert context.isCacheable(); + /* + * We use a private rewrite context here since we want the most realistic way of asserting that we are cacheable or not. We do it + * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the + * actual lucene query + */ + QueryBuilder rewritten = rewriteQuery(query, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertFalse("query was marked as cacheable in the context but it should not be cacheable: " + query.toString(), + context.isCacheable()); + + query = new SimpleQueryStringBuilder(randomFrom("NoW", "nOw", "NOW") + randomAlphaOfLengthBetween(1, 10)); + + context = createShardContext(); + rewritten = rewriteQuery(query, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertTrue("query was marked as not cacheable in the context but it should be cacheable: " + query.toString(), + context.isCacheable()); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index 847fb58eca3e7..0ce38f3d68144 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -20,6 +20,7 @@ package org.elasticsearch.test; import com.fasterxml.jackson.core.io.JsonStringEncoder; + import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; @@ -470,7 +471,7 @@ public void testToQuery() throws IOException { } } - private QueryBuilder rewriteQuery(QB queryBuilder, QueryRewriteContext rewriteContext) throws IOException { + protected QueryBuilder rewriteQuery(QB queryBuilder, QueryRewriteContext rewriteContext) throws IOException { QueryBuilder rewritten = rewriteAndFetch(queryBuilder, rewriteContext); // extra safety to fail fast - serialize the rewritten version to ensure it's serializable. assertSerialization(rewritten); From 624228123d4c873731e820f2c0b18c947e223b45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 13 Jun 2019 16:47:06 +0200 Subject: [PATCH 2/3] Remove FullTextQueryTestCase --- .../index/query/FullTextQueryTestCase.java | 59 ------------------- .../index/query/MatchQueryBuilderTests.java | 9 +-- .../query/MultiMatchQueryBuilderTests.java | 9 +-- .../query/QueryStringQueryBuilderTests.java | 9 +-- 4 files changed, 6 insertions(+), 80 deletions(-) delete mode 100644 server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java diff --git a/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java b/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java deleted file mode 100644 index 85ae9874b67e6..0000000000000 --- a/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * 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.elasticsearch.index.mapper.DateFieldMapper; -import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.test.AbstractQueryTestCase; - -import java.util.Collection; -import java.util.HashSet; -import java.util.Set; - -public abstract class FullTextQueryTestCase> extends AbstractQueryTestCase { - - /** - * Full text queries that start with "now" are not cacheable if they - * target a {@link DateFieldMapper.DateFieldType} field. - */ - protected final boolean isCacheable(Collection fields, String value) { - if (value.length() < 3 - || value.substring(0, 3).equalsIgnoreCase("now") == false) { - return true; - } - Set dateFields = new HashSet<>(); - getMapping().forEach(ft -> { - if (ft instanceof DateFieldMapper.DateFieldType) { - dateFields.add(ft.name()); - } - }); - for (MappedFieldType ft : getMapping()) { - if (ft instanceof DateFieldMapper.DateFieldType) { - dateFields.add(ft.name()); - } - } - if (fields.isEmpty()) { - // special case: all fields are requested - return dateFields.isEmpty(); - } - return fields.stream() - .anyMatch(dateFields::contains) == false; - } -} diff --git a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java index 76ea5aa9dc6a0..cbd9daac29baf 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java @@ -45,6 +45,7 @@ import org.elasticsearch.index.search.MatchQuery.Type; import org.elasticsearch.index.search.MatchQuery.ZeroTermsQuery; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.AbstractQueryTestCase; import org.hamcrest.Matcher; import java.io.IOException; @@ -54,19 +55,13 @@ import java.util.Locale; import java.util.Map; -import static java.util.Collections.singletonList; import static org.hamcrest.CoreMatchers.either; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; -public class MatchQueryBuilderTests extends FullTextQueryTestCase { - @Override - protected boolean isCacheable(MatchQueryBuilder queryBuilder) { - return queryBuilder.fuzziness() != null - || isCacheable(singletonList(queryBuilder.fieldName()), queryBuilder.value().toString()); - } +public class MatchQueryBuilderTests extends AbstractQueryTestCase { @Override protected MatchQueryBuilder doCreateTestQueryBuilder() { diff --git a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java index ef810d6686c4b..6bce018213aa5 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java @@ -41,6 +41,7 @@ import org.elasticsearch.index.query.MultiMatchQueryBuilder.Type; import org.elasticsearch.index.search.MatchQuery; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.AbstractQueryTestCase; import java.io.IOException; import java.util.Arrays; @@ -58,16 +59,10 @@ import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.collection.IsCollectionWithSize.hasSize; -public class MultiMatchQueryBuilderTests extends FullTextQueryTestCase { +public class MultiMatchQueryBuilderTests extends AbstractQueryTestCase { private static final String MISSING_WILDCARD_FIELD_NAME = "missing_*"; private static final String MISSING_FIELD_NAME = "missing"; - @Override - protected boolean isCacheable(MultiMatchQueryBuilder queryBuilder) { - return queryBuilder.fuzziness() != null - || isCacheable(queryBuilder.fields().keySet(), queryBuilder.value().toString()); - } - @Override protected MultiMatchQueryBuilder doCreateTestQueryBuilder() { String fieldName = randomFrom(STRING_FIELD_NAME, INT_FIELD_NAME, DOUBLE_FIELD_NAME, BOOLEAN_FIELD_NAME, DATE_FIELD_NAME, diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index 42362e593a4ba..7fac144f34165 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -61,6 +61,7 @@ import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.search.QueryStringQueryParser; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.AbstractQueryTestCase; import org.hamcrest.CoreMatchers; import org.hamcrest.Matchers; @@ -84,13 +85,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; -public class QueryStringQueryBuilderTests extends FullTextQueryTestCase { - - @Override - protected boolean isCacheable(QueryStringQueryBuilder queryBuilder) { - return queryBuilder.fuzziness() != null - || isCacheable(queryBuilder.fields().keySet(), queryBuilder.queryString()); - } +public class QueryStringQueryBuilderTests extends AbstractQueryTestCase { @Override protected void initializeAdditionalMappings(MapperService mapperService) throws IOException { From 38a9d55f7fe75fb8f5564b9c1fc015aee19176ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 13 Jun 2019 17:08:42 +0200 Subject: [PATCH 3/3] Add now on date-field tests to Match- and Multi-MatchQB --- .../index/query/MatchQueryBuilderTests.java | 18 +++++++++ .../query/MultiMatchQueryBuilderTests.java | 18 +++++++++ .../query/QueryStringQueryBuilderTests.java | 35 ++++++++-------- .../query/SimpleQueryStringBuilderTests.java | 40 +++++++++++-------- 4 files changed, 78 insertions(+), 33 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java index cbd9daac29baf..99c7a1e7fdc14 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java @@ -508,4 +508,22 @@ private static CannedBinaryTokenStream.BinaryToken[] createGiantGraphMultiTerms( } return tokens.toArray(new CannedBinaryTokenStream.BinaryToken[0]); } + + /** + * "now" on date fields should make the query non-cachable. + */ + public void testCachingStrategiesWithNow() throws IOException { + // if we hit a date field with "now", this should diable cachability + MatchQueryBuilder queryBuilder = new MatchQueryBuilder(DATE_FIELD_NAME, "now"); + QueryShardContext context = createShardContext(); + assert context.isCacheable(); + /* + * We use a private rewrite context here since we want the most realistic way of asserting that we are cacheable or not. We do it + * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the + * actual lucene query + */ + QueryBuilder rewritten = rewriteQuery(queryBuilder, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertFalse("query should not be cacheable: " + queryBuilder.toString(), context.isCacheable()); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java index 6bce018213aa5..3bbcca56beca7 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java @@ -550,4 +550,22 @@ private void assertQueryWithAllFieldsWildcard(Query query) { assertThat(disjunctionMaxQuery.getDisjuncts(), hasItems(new TermQuery(new Term(STRING_FIELD_NAME, "hello")), new TermQuery(new Term(STRING_FIELD_NAME_2, "hello")))); } + + /** + * "now" on date fields should make the query non-cachable. + */ + public void testCachingStrategiesWithNow() throws IOException { + // if we hit a date field with "now", this should diable cachability + MultiMatchQueryBuilder queryBuilder = new MultiMatchQueryBuilder("now", DATE_FIELD_NAME); + QueryShardContext context = createShardContext(); + assert context.isCacheable(); + /* + * We use a private rewrite context here since we want the most realistic way of asserting that we are cacheable or not. We do it + * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the + * actual lucene query + */ + QueryBuilder rewritten = rewriteQuery(queryBuilder, new QueryShardContext(context)); + assertNotNull(rewritten.toQuery(context)); + assertFalse("query should not be cacheable: " + queryBuilder.toString(), context.isCacheable()); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index 7fac144f34165..ad6152e96737f 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -1580,15 +1580,27 @@ private void assertQueryWithAllFieldsWildcard(Query query) { } /** - * Query terms that start with "now" can trigger a query to not be cacheable if it hits all field - * and there is a date field amongst them. This test checks the search context cacheable flag is - * updated accordingly. - * Note: this cannot happen when directly hitting the date field, dates are not parsed leniently then - * and we typically get a parse exception. + * Query terms that contain "now" can trigger a query to not be cacheable. + * This test checks the search context cacheable flag is updated accordingly. */ public void testCachingStrategiesWithNow() throws IOException { + // if we hit all fields, this should contain a date field and should diable cachability String query = "now " + randomAlphaOfLengthBetween(4, 10); QueryStringQueryBuilder queryStringQueryBuilder = new QueryStringQueryBuilder(query); + assertQueryCachability(queryStringQueryBuilder, false); + + // if we hit a date field with "now", this should diable cachability + queryStringQueryBuilder = new QueryStringQueryBuilder("now"); + queryStringQueryBuilder.field(DATE_FIELD_NAME); + assertQueryCachability(queryStringQueryBuilder, false); + + // everything else is fine on all fields + query = randomFrom("NoW", "nOw", "NOW") + " " + randomAlphaOfLengthBetween(4, 10); + queryStringQueryBuilder = new QueryStringQueryBuilder(query); + assertQueryCachability(queryStringQueryBuilder, true); + } + + private void assertQueryCachability(QueryStringQueryBuilder qb, boolean cachingExpected) throws IOException { QueryShardContext context = createShardContext(); assert context.isCacheable(); /* @@ -1596,18 +1608,9 @@ public void testCachingStrategiesWithNow() throws IOException { * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the * actual lucene query */ - QueryBuilder rewritten = rewriteQuery(queryStringQueryBuilder, new QueryShardContext(context)); - assertNotNull(rewritten.toQuery(context)); - assertFalse("query was marked as cacheable in the context but it should not be cacheable: " + query.toString(), - context.isCacheable()); - - query = randomFrom("NoW", "nOw", "NOW") + " " + randomAlphaOfLengthBetween(4, 10); - queryStringQueryBuilder = new QueryStringQueryBuilder(query); - - context = createShardContext(); - rewritten = rewriteQuery(queryStringQueryBuilder, new QueryShardContext(context)); + QueryBuilder rewritten = rewriteQuery(qb, new QueryShardContext(context)); assertNotNull(rewritten.toQuery(context)); - assertTrue("query was marked as not cacheable in the context but it should be cacheable: " + query.toString(), + assertEquals("query should " + (cachingExpected ? "" : "not") + " be cacheable: " + qb.toString(), cachingExpected, context.isCacheable()); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java index 8195b5f97cb71..fbd707b4c2ae1 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java @@ -43,6 +43,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.search.SimpleQueryStringQueryParser; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.AbstractQueryTestCase; import java.io.IOException; import java.util.ArrayList; @@ -64,7 +65,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; -public class SimpleQueryStringBuilderTests extends FullTextQueryTestCase { +public class SimpleQueryStringBuilderTests extends AbstractQueryTestCase { @Override protected SimpleQueryStringBuilder doCreateTestQueryBuilder() { @@ -786,14 +787,27 @@ private void assertQueryWithAllFieldsWildcard(Query query) { } /** - * Query terms that start with "now" can trigger a query to not be cacheable if it hits all field - * and there is a date field amongst them. This test checks the search context cacheable flag is - * updated accordingly - * Note: this cannot happen when directly hitting the date field, dates are not parsed leniently then - * and we typically get a parse exception. + * Query terms that contain "now" can trigger a query to not be cacheable. + * This test checks the search context cacheable flag is updated accordingly. */ public void testCachingStrategiesWithNow() throws IOException { - SimpleQueryStringBuilder query = new SimpleQueryStringBuilder("now" + randomAlphaOfLengthBetween(1, 10)); + // if we hit all fields, this should contain a date field and should diable cachability + String query = "now " + randomAlphaOfLengthBetween(4, 10); + SimpleQueryStringBuilder queryBuilder = new SimpleQueryStringBuilder(query); + assertQueryCachability(queryBuilder, false); + + // if we hit a date field with "now", this should diable cachability + queryBuilder = new SimpleQueryStringBuilder("now"); + queryBuilder.field(DATE_FIELD_NAME); + assertQueryCachability(queryBuilder, false); + + // everything else is fine on all fields + query = randomFrom("NoW", "nOw", "NOW") + " " + randomAlphaOfLengthBetween(4, 10); + queryBuilder = new SimpleQueryStringBuilder(query); + assertQueryCachability(queryBuilder, true); + } + + private void assertQueryCachability(SimpleQueryStringBuilder qb, boolean cachingExpected) throws IOException { QueryShardContext context = createShardContext(); assert context.isCacheable(); /* @@ -801,17 +815,9 @@ public void testCachingStrategiesWithNow() throws IOException { * this way in SearchService where we first rewrite the query with a private context, then reset the context and then build the * actual lucene query */ - QueryBuilder rewritten = rewriteQuery(query, new QueryShardContext(context)); - assertNotNull(rewritten.toQuery(context)); - assertFalse("query was marked as cacheable in the context but it should not be cacheable: " + query.toString(), - context.isCacheable()); - - query = new SimpleQueryStringBuilder(randomFrom("NoW", "nOw", "NOW") + randomAlphaOfLengthBetween(1, 10)); - - context = createShardContext(); - rewritten = rewriteQuery(query, new QueryShardContext(context)); + QueryBuilder rewritten = rewriteQuery(qb, new QueryShardContext(context)); assertNotNull(rewritten.toQuery(context)); - assertTrue("query was marked as not cacheable in the context but it should be cacheable: " + query.toString(), + assertEquals("query should " + (cachingExpected ? "" : "not") + " be cacheable: " + qb.toString(), cachingExpected, context.isCacheable()); } }