From 852df128a5544dbafaf7f901e31f3d88431c767e Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 19:12:45 +0100 Subject: [PATCH] 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 --- .../migration/migrate_7_0/search.asciidoc | 3 ++ .../index/search/MatchQuery.java | 31 ++++++++++--------- .../MatchPhrasePrefixQueryBuilderTests.java | 17 +++++----- .../query/MatchPhraseQueryBuilderTests.java | 11 +------ .../query/MultiMatchQueryBuilderTests.java | 1 + 5 files changed, 29 insertions(+), 34 deletions(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 7505b6f14d1b4..53a7d88394bb9 100644 --- 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 index 43c842a1697e6..d60d9cd9ce6b6 100644 --- 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 index 00701adc449c3..8b706e552bcbf 100644 --- 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 index 91a775dbf0256..fb03d54aeff8c 100644 --- 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 index b6d4816b01f4a..fcdf128c41695 100644 --- 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;