Skip to content

Commit

Permalink
Fixes query_string query equals timezone check (#29406)
Browse files Browse the repository at this point in the history 
* Fixes query_string query equals timezone check

This change fixes a bug where two `QueryStringQueryBuilder`s were found
to be equal if they had the same timezone set even if the query string
in the builders were different

Closes #29403

* Adds mutate function to QueryStringQueryBuilderTests

* iter

server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuild
er.java
server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuild
erTests.java
test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCas
e.java
server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuild
er.java
  • Loading branch information
@colings86
colings86 committed Apr 6, 2018
1 parent b26f934 commit 00817a9
Show file tree
Hide file tree
Showing 3 changed files with 240 additions and 8 deletions.
26 changes: 21 additions & 5 deletions server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -388,7 +387,7 @@ public QueryStringQueryBuilder tieBreaker(float tieBreaker) {
return this;
}

public float tieBreaker() {
public Float tieBreaker() {
return this.tieBreaker;
}

Expand Down Expand Up @@ -419,6 +418,22 @@ public QueryStringQueryBuilder analyzer(String analyzer) {
this.analyzer = analyzer;
return this;
}

/**
* The optional analyzer used to analyze the query string. Note, if a field has search analyzer
* defined for it, then it will be used automatically. Defaults to the smart search analyzer.
*/
public String analyzer() {
return analyzer;
}

/**
* The optional analyzer used to analyze the query string for phrase searches. Note, if a field has search (quote) analyzer
* defined for it, then it will be used automatically. Defaults to the smart search analyzer.
*/
public String quoteAnalyzer() {
return quoteAnalyzer;
}

/**
* The optional analyzer used to analyze the query string for phrase searches. Note, if a field has search (quote) analyzer
Expand Down Expand Up @@ -926,9 +941,10 @@ protected boolean doEquals(QueryStringQueryBuilder other) {
Objects.equals(tieBreaker, other.tieBreaker) &&
Objects.equals(rewrite, other.rewrite) &&
Objects.equals(minimumShouldMatch, other.minimumShouldMatch) &&
Objects.equals(lenient, other.lenient) &&
timeZone == null ? other.timeZone == null : other.timeZone != null &&
Objects.equals(timeZone.getID(), other.timeZone.getID()) &&
Objects.equals(lenient, other.lenient) &&
Objects.equals(
timeZone == null ? null : timeZone.getID(),
other.timeZone == null ? null : other.timeZone.getID()) &&
Objects.equals(escape, other.escape) &&
Objects.equals(maxDeterminizedStates, other.maxDeterminizedStates) &&
Objects.equals(autoGenerateSynonymsPhraseQuery, other.autoGenerateSynonymsPhraseQuery) &&
Expand Down
212 changes: 212 additions & 0 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 @@ -67,7 +67,9 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery;
Expand Down Expand Up @@ -173,6 +175,206 @@ protected QueryStringQueryBuilder doCreateTestQueryBuilder() {
return queryStringQueryBuilder;
}

@Override
public QueryStringQueryBuilder mutateInstance(QueryStringQueryBuilder instance) throws IOException {
String query = instance.queryString();
String defaultField = instance.defaultField();
Map<String, Float> fields = instance.fields();
Operator operator = instance.defaultOperator();
Fuzziness fuzziness = instance.fuzziness();
String analyzer = instance.analyzer();
String quoteAnalyzer = instance.quoteAnalyzer();
Boolean allowLeadingWildCard = instance.allowLeadingWildcard();
Boolean analyzeWildcard = instance.analyzeWildcard();
int maxDeterminizedStates = instance.maxDeterminizedStates();
boolean enablePositionIncrements = instance.enablePositionIncrements();
boolean escape = instance.escape();
int phraseSlop = instance.phraseSlop();
int fuzzyMaxExpansions = instance.fuzzyMaxExpansions();
int fuzzyPrefixLength = instance.fuzzyPrefixLength();
String fuzzyRewrite = instance.fuzzyRewrite();
String rewrite = instance.rewrite();
String quoteFieldSuffix = instance.quoteFieldSuffix();
Float tieBreaker = instance.tieBreaker();
String minimumShouldMatch = instance.minimumShouldMatch();
String timeZone = instance.timeZone() == null ? null : instance.timeZone().getID();
boolean autoGenerateSynonymsPhraseQuery = instance.autoGenerateSynonymsPhraseQuery();
boolean fuzzyTranspositions = instance.fuzzyTranspositions();

switch (between(0, 23)) {
case 0:
query = query + " foo";
break;
case 1:
if (defaultField == null) {
defaultField = randomAlphaOfLengthBetween(1, 10);
} else {
defaultField = defaultField + randomAlphaOfLength(5);
}
break;
case 2:
fields = new HashMap<>(fields);
fields.put(randomAlphaOfLength(10), 1.0f);
break;
case 3:
operator = randomValueOtherThan(operator, () -> randomFrom(Operator.values()));
break;
case 4:
fuzziness = randomValueOtherThan(fuzziness, () -> randomFrom(Fuzziness.AUTO, Fuzziness.ZERO, Fuzziness.ONE, Fuzziness.TWO));
break;
case 5:
if (analyzer == null) {
analyzer = randomAnalyzer();
} else {
analyzer = null;
}
break;
case 6:
if (quoteAnalyzer == null) {
quoteAnalyzer = randomAnalyzer();
} else {
quoteAnalyzer = null;
}
break;
case 7:
if (allowLeadingWildCard == null) {
allowLeadingWildCard = randomBoolean();
} else {
allowLeadingWildCard = randomBoolean() ? null : (allowLeadingWildCard == false);
}
break;
case 8:
if (analyzeWildcard == null) {
analyzeWildcard = randomBoolean();
} else {
analyzeWildcard = randomBoolean() ? null : (analyzeWildcard == false);
}
break;
case 9:
maxDeterminizedStates += 5;
break;
case 10:
enablePositionIncrements = (enablePositionIncrements == false);
break;
case 11:
escape = (escape == false);
break;
case 12:
phraseSlop += 5;
break;
case 13:
fuzzyMaxExpansions += 5;
break;
case 14:
fuzzyPrefixLength += 5;
break;
case 15:
if (fuzzyRewrite == null) {
fuzzyRewrite = getRandomRewriteMethod();
} else {
fuzzyRewrite = null;
}
break;
case 16:
if (rewrite == null) {
rewrite = getRandomRewriteMethod();
} else {
rewrite = null;
}
break;
case 17:
if (quoteFieldSuffix == null) {
quoteFieldSuffix = randomAlphaOfLengthBetween(1, 3);
} else {
quoteFieldSuffix = quoteFieldSuffix + randomAlphaOfLength(1);
}
break;
case 18:
if (tieBreaker == null) {
tieBreaker = randomFloat();
} else {
tieBreaker += 0.05f;
}
break;
case 19:
if (minimumShouldMatch == null) {
minimumShouldMatch = randomMinimumShouldMatch();
} else {
minimumShouldMatch = null;
}
break;
case 20:
if (timeZone == null) {
timeZone = randomDateTimeZone().getID();
} else {
if (randomBoolean()) {
timeZone = null;
} else {
timeZone = randomValueOtherThan(timeZone, () -> randomDateTimeZone().getID());
}
}
break;
case 21:
autoGenerateSynonymsPhraseQuery = (autoGenerateSynonymsPhraseQuery == false);
break;
case 22:
fuzzyTranspositions = (fuzzyTranspositions == false);
break;
case 23:
return changeNameOrBoost(instance);
default:
throw new AssertionError("Illegal randomisation branch");
}

QueryStringQueryBuilder newInstance = new QueryStringQueryBuilder(query);
if (defaultField != null) {
newInstance.defaultField(defaultField);
}
newInstance.fields(fields);
newInstance.defaultOperator(operator);
newInstance.fuzziness(fuzziness);
if (analyzer != null) {
newInstance.analyzer(analyzer);
}
if (quoteAnalyzer != null) {
newInstance.quoteAnalyzer(quoteAnalyzer);
}
if (allowLeadingWildCard != null) {
newInstance.allowLeadingWildcard(allowLeadingWildCard);
}
if (analyzeWildcard != null) {
newInstance.analyzeWildcard(analyzeWildcard);
}
newInstance.maxDeterminizedStates(maxDeterminizedStates);
newInstance.enablePositionIncrements(enablePositionIncrements);
newInstance.escape(escape);
newInstance.phraseSlop(phraseSlop);
newInstance.fuzzyMaxExpansions(fuzzyMaxExpansions);
newInstance.fuzzyPrefixLength(fuzzyPrefixLength);
if (fuzzyRewrite != null) {
newInstance.fuzzyRewrite(fuzzyRewrite);
}
if (rewrite != null) {
newInstance.rewrite(rewrite);
}
if (quoteFieldSuffix != null) {
newInstance.quoteFieldSuffix(quoteFieldSuffix);
}
if (tieBreaker != null) {
newInstance.tieBreaker(tieBreaker);
}
if (minimumShouldMatch != null) {
newInstance.minimumShouldMatch(minimumShouldMatch);
}
if (timeZone != null) {
newInstance.timeZone(timeZone);
}
newInstance.autoGenerateSynonymsPhraseQuery(autoGenerateSynonymsPhraseQuery);
newInstance.fuzzyTranspositions(fuzzyTranspositions);

return newInstance;
}

@Override
protected void doAssertLuceneQuery(QueryStringQueryBuilder queryBuilder,
Query query, SearchContext context) throws IOException {
Expand All @@ -183,6 +385,16 @@ protected void doAssertLuceneQuery(QueryStringQueryBuilder queryBuilder,
.or(instanceOf(MatchNoDocsQuery.class)));
}

// Tests fix for https://github.com/elastic/elasticsearch/issues/29403
public void testTimezoneEquals() {
QueryStringQueryBuilder builder1 = new QueryStringQueryBuilder("bar");
QueryStringQueryBuilder builder2 = new QueryStringQueryBuilder("foo");
assertNotEquals(builder1, builder2);
builder1.timeZone("Europe/London");
builder2.timeZone("Europe/London");
assertNotEquals(builder1, builder2);
}

public void testIllegalArguments() {
expectThrows(IllegalArgumentException.class, () -> new QueryStringQueryBuilder((String) null));
}
Expand Down
10 changes: 7 additions & 3 deletions 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 @@ -55,7 +56,6 @@
import org.elasticsearch.common.xcontent.DeprecationHandler;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentGenerator;
Expand Down Expand Up @@ -742,10 +742,14 @@ public void testEqualsAndHashcode() {
for (int runs = 0; runs < NUMBER_OF_TESTQUERIES; runs++) {
// TODO we only change name and boost, we should extend by any sub-test supplying a "mutate" method that randomly changes one
// aspect of the object under test
checkEqualsAndHashCode(createTestQueryBuilder(), this::copyQuery, this::changeNameOrBoost);
checkEqualsAndHashCode(createTestQueryBuilder(), this::copyQuery, this::mutateInstance);
}
}

public QB mutateInstance(QB instance) throws IOException {
return changeNameOrBoost(instance);
}

/**
* Generic test that checks that the <code>Strings.toString()</code> method
* renders the XContent correctly.
Expand All @@ -761,7 +765,7 @@ public void testValidOutput() throws IOException {
}
}

private QB changeNameOrBoost(QB original) throws IOException {
protected QB changeNameOrBoost(QB original) throws IOException {
QB secondQuery = copyQuery(original);
if (randomBoolean()) {
secondQuery.queryName(secondQuery.queryName() == null ? randomAlphaOfLengthBetween(1, 30) : secondQuery.queryName()
Expand Down

0 comments on commit 00817a9

Please sign in to comment.