From 666520561a57ef23d7cc1fa3267671768317a15c Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 10:49:49 +0100 Subject: [PATCH 1/6] Match phrase queries against non-indexed fields should throw an exception --- docs/reference/migration/migrate_7_0/search.asciidoc | 3 +++ .../index/query/MatchPhrasePrefixQueryBuilderTests.java | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 7505b6f14d1b4..b1d5c3d12b696 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-indexed fields + 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/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java index b4d5f98fe0b47..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")); } @@ -118,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" + From c8637c4fc6343654bff4874576297a658ce097d6 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 12:52:14 +0100 Subject: [PATCH 2/6] feedback on docs --- docs/reference/migration/migrate_7_0/search.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index b1d5c3d12b696..53a7d88394bb9 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -15,8 +15,8 @@ * 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-indexed fields - will now throw an exception +* 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 From bc07d695fc45c139532e40b3a428909760c0eb77 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 14:34:45 +0100 Subject: [PATCH 3/6] Apply fix to MatchPhraseQueryBuilderTests as well --- .../elasticsearch/index/query/MatchPhraseQueryBuilderTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1ac53992ebe8c..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")); } From e6122a6585602b6511abdb33d2e83f4e74d4fb81 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 14:36:18 +0100 Subject: [PATCH 4/6] Unmute tests --- .../index/query/MatchPhrasePrefixQueryBuilderTests.java | 9 --------- .../index/query/MatchPhraseQueryBuilderTests.java | 9 --------- 2 files changed, 18 deletions(-) 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 1a53f5fe4a7df..8b706e552bcbf 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchPhrasePrefixQueryBuilderTests.java @@ -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()); 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 35899d0246390..fb03d54aeff8c 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java @@ -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()); From e1f3f2d3c929c2943dd1f44a0f4e76dd82f24682 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 15:28:17 +0100 Subject: [PATCH 5/6] Deal with exceptions if lenient=true --- .../index/search/MatchQuery.java | 31 ++++++++++--------- .../query/MultiMatchQueryBuilderTests.java | 1 + 2 files changed, 18 insertions(+), 14 deletions(-) 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..a9a3e67dadf4b 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 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 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 IllegalArgumentException("field:[" + field + "] was indexed without position data; cannot run PhraseQuery"); } - return null; } /** 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; From 0752b3c0af1810bb739c26e9969fbd35857f9eeb Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 4 Jun 2018 16:03:12 +0100 Subject: [PATCH 6/6] positions check back to IllegalStateException --- .../java/org/elasticsearch/index/search/MatchQuery.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 a9a3e67dadf4b..d60d9cd9ce6b6 100644 --- a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java +++ b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java @@ -361,7 +361,7 @@ protected Query analyzePhrase(String field, TokenStream stream, int slop) throws } return query; } - catch (IllegalArgumentException e) { + catch (IllegalArgumentException | IllegalStateException e) { if (lenient) { return newLenientFieldQuery(field, e); } @@ -375,7 +375,7 @@ protected Query analyzeMultiPhrase(String field, TokenStream stream, int slop) t checkForPositions(field); return mapper.multiPhraseQuery(field, stream, slop, enablePositionIncrements); } - catch (IllegalArgumentException e) { + catch (IllegalArgumentException | IllegalStateException e) { if (lenient) { return newLenientFieldQuery(field, e); } @@ -385,7 +385,7 @@ protected Query analyzeMultiPhrase(String field, TokenStream stream, int slop) t private void checkForPositions(String field) { if (hasPositions(mapper) == false) { - throw new IllegalArgumentException("field:[" + field + "] was indexed without position data; cannot run PhraseQuery"); + throw new IllegalStateException("field:[" + field + "] was indexed without position data; cannot run PhraseQuery"); } }