Browse Source

Match phrase queries against non-indexed fields should throw an exception (#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
Alan Woodward 7 years ago
parent
commit
852df128a5

+ 3 - 0
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

+ 17 - 14
server/src/main/java/org/elasticsearch/index/search/MatchQuery.java

@@ -352,38 +352,41 @@ public class MatchQuery {
 
         @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;
         }
 
         /**

+ 7 - 10
server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java

@@ -61,7 +61,7 @@ public class MatchPhrasePrefixQueryBuilderTests extends AbstractQueryTestCase<Ma
 
         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 @@ public class MatchPhrasePrefixQueryBuilderTests extends AbstractQueryTestCase<Ma
                 .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 class MatchPhrasePrefixQueryBuilderTests extends AbstractQueryTestCase<Ma
         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" +

+ 1 - 10
server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java

@@ -64,7 +64,7 @@ public class MatchPhraseQueryBuilderTests extends AbstractQueryTestCase<MatchPhr
 
         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 @@ public class MatchPhraseQueryBuilderTests extends AbstractQueryTestCase<MatchPhr
                 .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());

+ 1 - 0
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;