Match phrase queries against non-indexed fields should throw an excep…

…tion (#31060) When `lenient=false`, attempts to create match phrase queries with custom analyzers against non-text fields will throw an IllegalArgumentException. Also changes `*Match*QueryBuilderTests` so that it avoids this scenario Fixes #31061
elastic · Jun 4, 2018 · 852df12 · 852df12
1 parent 609de08
commit 852df12
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 34 deletions.
diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc
@@ -15,6 +15,9 @@
 *   The boundary specified using geohashes in the `geo_bounding_box` query
     now include entire geohash cell, instead of just geohash center.
 
+*   Attempts to generate multi-term phrase queries against non-text fields
+    with a custom analyzer will now throw an exception
+
 ==== Adaptive replica selection enabled by default
 
 Adaptive replica selection has been enabled by default. If you wish to return to

diff --git a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java
@@ -352,38 +352,41 @@ protected Query newSynonymQuery(Term[] terms) {
 
         @Override
         protected Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {
-            IllegalStateException e = checkForPositions(field);
-            if (e != null) {
+            try {
+                checkForPositions(field);
+                Query query = mapper.phraseQuery(field, stream, slop, enablePositionIncrements);
+                if (query instanceof PhraseQuery) {
+                    // synonyms that expand to multiple terms can return a phrase query.
+                    return blendPhraseQuery((PhraseQuery) query, mapper);
+                }
+                return query;
+            }
+            catch (IllegalArgumentException | IllegalStateException e) {
                 if (lenient) {
                     return newLenientFieldQuery(field, e);
                 }
                 throw e;
             }
-            Query query = mapper.phraseQuery(field, stream, slop, enablePositionIncrements);
-            if (query instanceof PhraseQuery) {
-                // synonyms that expand to multiple terms can return a phrase query.
-                return blendPhraseQuery((PhraseQuery) query, mapper);
-            }
-            return query;
         }
 
         @Override
         protected Query analyzeMultiPhrase(String field, TokenStream stream, int slop) throws IOException {
-            IllegalStateException e = checkForPositions(field);
-            if (e != null) {
+            try {
+                checkForPositions(field);
+                return mapper.multiPhraseQuery(field, stream, slop, enablePositionIncrements);
+            }
+            catch (IllegalArgumentException | IllegalStateException e) {
                 if (lenient) {
                     return newLenientFieldQuery(field, e);
                 }
                 throw e;
             }
-            return mapper.multiPhraseQuery(field, stream, slop, enablePositionIncrements);
         }
 
-        private IllegalStateException checkForPositions(String field) {
+        private void checkForPositions(String field) {
             if (hasPositions(mapper) == false) {
-                return new IllegalStateException("field:[" + field + "] was indexed without position data; cannot run PhraseQuery");
+                throw new IllegalStateException("field:[" + field + "] was indexed without position data; cannot run PhraseQuery");
             }
-            return null;
         }
 
         /**

diff --git a/server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java
@@ -61,7 +61,7 @@ protected MatchPhrasePrefixQueryBuilder doCreateTestQueryBuilder() {
 
         MatchPhrasePrefixQueryBuilder matchQuery = new MatchPhrasePrefixQueryBuilder(fieldName, value);
 
-        if (randomBoolean()) {
+        if (randomBoolean() && fieldName.equals(STRING_FIELD_NAME)) {
             matchQuery.analyzer(randomFrom("simple", "keyword", "whitespace"));
         }
 
@@ -99,15 +99,6 @@ protected void doAssertLuceneQuery(MatchPhrasePrefixQueryBuilder queryBuilder, Q
                 .or(instanceOf(IndexOrDocValuesQuery.class)).or(instanceOf(MatchNoDocsQuery.class)));
     }
 
-    /**
-     * Overridden to allow for annotating with @AwaitsFix. Please remove this method after fixing.
-     */
-    @Override
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31061")
-    public void testToQuery() throws IOException {
-        super.testToQuery();
-    }
-
     public void testIllegalValues() {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new MatchPhrasePrefixQueryBuilder(null, "value"));
         assertEquals("[match_phrase_prefix] requires fieldName", e.getMessage());
@@ -127,6 +118,12 @@ public void testBadAnalyzer() throws IOException {
         assertThat(e.getMessage(), containsString("analyzer [bogusAnalyzer] not found"));
     }
 
+    public void testPhraseOnFieldWithNoTerms() {
+        MatchPhrasePrefixQueryBuilder matchQuery = new MatchPhrasePrefixQueryBuilder(DATE_FIELD_NAME, "three term phrase");
+        matchQuery.analyzer("whitespace");
+        expectThrows(IllegalArgumentException.class, () -> matchQuery.doToQuery(createShardContext()));
+    }
+
     public void testPhrasePrefixMatchQuery() throws IOException {
         String json1 = "{\n" +
                 "    \"match_phrase_prefix\" : {\n" +

diff --git a/server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java
@@ -64,7 +64,7 @@ protected MatchPhraseQueryBuilder doCreateTestQueryBuilder() {
 
         MatchPhraseQueryBuilder matchQuery = new MatchPhraseQueryBuilder(fieldName, value);
 
-        if (randomBoolean()) {
+        if (randomBoolean() && fieldName.equals(STRING_FIELD_NAME)) {
             matchQuery.analyzer(randomFrom("simple", "keyword", "whitespace"));
         }
 
@@ -107,15 +107,6 @@ protected void doAssertLuceneQuery(MatchPhraseQueryBuilder queryBuilder, Query q
                 .or(instanceOf(IndexOrDocValuesQuery.class)).or(instanceOf(MatchNoDocsQuery.class)));
     }
 
-    /**
-     * Overridden to allow for annotating with @AwaitsFix. Please remove this method after fixing.
-     */
-    @Override
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31061")
-    public void testToQuery() throws IOException {
-        super.testToQuery();
-    }
-
     public void testIllegalValues() {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new MatchPhraseQueryBuilder(null, "value"));
         assertEquals("[match_phrase] requires fieldName", e.getMessage());

diff --git a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.index.query;
 
+import com.carrotsearch.randomizedtesting.annotations.Seed;
 import org.apache.lucene.index.Term;
 import org.apache.lucene.queries.ExtendedCommonTermsQuery;
 import org.apache.lucene.search.BooleanQuery;