Skip to content

Commit

Permalink
SimpleQ.S.B and QueryStringQ.S.B tests should avoid now in query (#…
Browse files Browse the repository at this point in the history 
…43199)

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
  • Loading branch information
Christoph Büscher committed Jun 14, 2019
1 parent 4c8e77e commit 7af2332
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 89 deletions.
60 changes: 0 additions & 60 deletions server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java
View file

This file was deleted.

27 changes: 20 additions & 7 deletions server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,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;
Expand All @@ -55,19 +56,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<MatchQueryBuilder> {
@Override
protected boolean isCacheable(MatchQueryBuilder queryBuilder) {
return queryBuilder.fuzziness() != null
|| isCacheable(singletonList(queryBuilder.fieldName()), queryBuilder.value().toString());
}
public class MatchQueryBuilderTests extends AbstractQueryTestCase<MatchQueryBuilder> {

@Override
protected MatchQueryBuilder doCreateTestQueryBuilder() {
Expand Down Expand Up @@ -526,4 +521,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());
}
}
27 changes: 20 additions & 7 deletions server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,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;
Expand All @@ -59,16 +60,10 @@
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.collection.IsCollectionWithSize.hasSize;

public class MultiMatchQueryBuilderTests extends FullTextQueryTestCase<MultiMatchQueryBuilder> {
public class MultiMatchQueryBuilderTests extends AbstractQueryTestCase<MultiMatchQueryBuilder> {
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,
Expand Down Expand Up @@ -557,4 +552,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());
}
}
51 changes: 43 additions & 8 deletions server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,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;

Expand All @@ -72,6 +73,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;
Expand All @@ -84,12 +86,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.instanceOf;

public class QueryStringQueryBuilderTests extends FullTextQueryTestCase<QueryStringQueryBuilder> {
@Override
protected boolean isCacheable(QueryStringQueryBuilder queryBuilder) {
return queryBuilder.fuzziness() != null
|| isCacheable(queryBuilder.fields().keySet(), queryBuilder.queryString());
}
public class QueryStringQueryBuilderTests extends AbstractQueryTestCase<QueryStringQueryBuilder> {

@Override
protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
Expand All @@ -109,8 +106,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()) {
Expand Down Expand Up @@ -1581,4 +1581,39 @@ 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 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();
/*
* 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(qb, new QueryShardContext(context));
assertNotNull(rewritten.toQuery(context));
assertEquals("query should " + (cachingExpected ? "" : "not") + " be cacheable: " + qb.toString(), cachingExpected,
context.isCacheable());
}
}
46 changes: 40 additions & 6 deletions server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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;
Expand All @@ -65,15 +66,13 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;

public class SimpleQueryStringBuilderTests extends FullTextQueryTestCase<SimpleQueryStringBuilder> {
@Override
protected boolean isCacheable(SimpleQueryStringBuilder queryBuilder) {
return isCacheable(queryBuilder.fields().keySet(), queryBuilder.value());
}
public class SimpleQueryStringBuilderTests extends AbstractQueryTestCase<SimpleQueryStringBuilder> {

@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());
Expand Down Expand Up @@ -790,4 +789,39 @@ 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 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);
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();
/*
* 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(qb, new QueryShardContext(context));
assertNotNull(rewritten.toQuery(context));
assertEquals("query should " + (cachingExpected ? "" : "not") + " be cacheable: " + qb.toString(), cachingExpected,
context.isCacheable());
}
}
3 changes: 2 additions & 1 deletion test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 7af2332

Please sign in to comment.