From 7af23324e3291176bce59535ead95d59de326f4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 14 Jun 2019 11:14:53 +0200 Subject: [PATCH] SimpleQ.S.B and QueryStringQ.S.B tests should avoid `now` in query (#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 --- .../index/query/FullTextQueryTestCase.java | 60 ------------------- .../index/query/MatchQueryBuilderTests.java | 27 ++++++--- .../query/MultiMatchQueryBuilderTests.java | 27 ++++++--- .../query/QueryStringQueryBuilderTests.java | 51 +++++++++++++--- .../query/SimpleQueryStringBuilderTests.java | 46 ++++++++++++-- .../test/AbstractQueryTestCase.java | 3 +- 6 files changed, 125 insertions(+), 89 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 7f9d85e3e26..00000000000 --- a/server/src/test/java/org/elasticsearch/index/query/FullTextQueryTestCase.java +++ /dev/null @@ -1,60 +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 { - protected abstract boolean isCacheable(QB queryBuilder); - - /** - * 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 f79bbb86242..0fc8e0f1de6 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java @@ -46,6 +46,7 @@ import org.elasticsearch.index.search.MatchQuery; 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; @@ -55,19 +56,13 @@ import java.util.List; 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() { @@ -526,4 +521,22 @@ public class MatchQueryBuilderTests 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, @@ -557,4 +552,22 @@ public class MultiMatchQueryBuilderTests 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 { @@ -109,8 +106,11 @@ public class QueryStringQueryBuilderTests extends FullTextQueryTestCase s.toLowerCase(Locale.ROOT).contains("now"), + () -> randomAlphaOfLengthBetween(4, 10)); + query += (randomBoolean() ? STRING_FIELD_NAME + ":" : "") + term + " "; } QueryStringQueryBuilder queryStringQueryBuilder = new QueryStringQueryBuilder(query); if (randomBoolean()) { @@ -1581,4 +1581,39 @@ public class QueryStringQueryBuilderTests extends FullTextQueryTestCase { - @Override - protected boolean isCacheable(SimpleQueryStringBuilder queryBuilder) { - return isCacheable(queryBuilder.fields().keySet(), queryBuilder.value()); - } +public class SimpleQueryStringBuilderTests extends AbstractQueryTestCase { @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()); @@ -790,4 +789,39 @@ public class SimpleQueryStringBuilderTests extends FullTextQueryTestCase> } } - 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);