From b95a4db6e61b44a8cb1d7985257cf532165827c7 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 26 Nov 2018 18:15:59 +0100 Subject: [PATCH] Throw a parsing exception when boost is set in span_or query (#28390) (#34112) --- .../migration/migrate_7_0/search.asciidoc | 6 +- .../reference/query-dsl/span-queries.asciidoc | 8 +- .../query/SpanContainingQueryBuilder.java | 4 + .../index/query/SpanFirstQueryBuilder.java | 9 +- .../index/query/SpanNearQueryBuilder.java | 8 +- .../index/query/SpanNotQueryBuilder.java | 14 ++- .../index/query/SpanOrQueryBuilder.java | 10 +- .../index/query/SpanQueryBuilder.java | 30 +++++- .../index/query/SpanWithinQueryBuilder.java | 4 + .../SpanContainingQueryBuilderTests.java | 91 +++++++++++++++- .../query/SpanFirstQueryBuilderTests.java | 28 ++++- .../query/SpanNearQueryBuilderTests.java | 39 ++++++- .../index/query/SpanNotQueryBuilderTests.java | 100 +++++++++++++++++- .../index/query/SpanOrQueryBuilderTests.java | 25 ++++- .../query/SpanTermQueryBuilderTests.java | 9 +- .../query/SpanWithinQueryBuilderTests.java | 91 +++++++++++++++- .../index/query/WrapperQueryBuilderTests.java | 7 +- .../test/AbstractQueryTestCase.java | 40 ++++--- 18 files changed, 474 insertions(+), 49 deletions(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 2576a0e5136..348763762aa 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -18,12 +18,14 @@ 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 + with a custom analyzer will now throw an exception. * An `envelope` crossing the dateline in a `geo_shape `query is now processed correctly when specified using REST API instead of having its left and right corners flipped. +* Attempts to set `boost` on inner span queries will now throw a parsing exception. + [float] ==== Adaptive replica selection enabled by default @@ -156,4 +158,4 @@ To deboost a specific query you can use a `boost` comprise between 0 and 1. Negative scores in the Function Score Query are deprecated in 6.x, and are not allowed in this version. If a negative score is produced as a result of computation (e.g. in `script_score` or `field_value_factor` functions), -an error will be thrown. \ No newline at end of file +an error will be thrown. diff --git a/docs/reference/query-dsl/span-queries.asciidoc b/docs/reference/query-dsl/span-queries.asciidoc index 4a1a019574e..7dc65433432 100644 --- a/docs/reference/query-dsl/span-queries.asciidoc +++ b/docs/reference/query-dsl/span-queries.asciidoc @@ -5,6 +5,12 @@ Span queries are low-level positional queries which provide expert control over the order and proximity of the specified terms. These are typically used to implement very specific queries on legal documents or patents. +It is only allowed to set boost on an outer span query. Compound span queries, +like span_near, only use the list of matching spans of inner span queries in +order to find their own spans, which they then use to produce a score. Scores +are never computed on inner span queries, which is the reason why boosts are not +allowed: they only influence the way scores are computed, not spans. + Span queries cannot be mixed with non-span queries (with the exception of the `span_multi` query). The queries in this group are: @@ -67,4 +73,4 @@ include::span-containing-query.asciidoc[] include::span-within-query.asciidoc[] -include::span-field-masking-query.asciidoc[] \ No newline at end of file +include::span-field-masking-query.asciidoc[] diff --git a/server/src/main/java/org/elasticsearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/SpanContainingQueryBuilder.java index 2842b84fa1c..164a5809f6e 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/SpanContainingQueryBuilder.java @@ -32,6 +32,8 @@ import org.elasticsearch.common.xcontent.XContentParser; import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost; + /** * Builder for {@link org.apache.lucene.search.spans.SpanContainingQuery}. */ @@ -117,12 +119,14 @@ public class SpanContainingQueryBuilder extends AbstractQueryBuilder implements SpanQueryBuilder { public static final String NAME = "span_first"; @@ -115,9 +117,10 @@ public class SpanFirstQueryBuilder extends AbstractQueryBuilder implements SpanQueryBuilder { public static final String NAME = "span_not"; @@ -181,15 +183,17 @@ public class SpanNotQueryBuilder extends AbstractQueryBuilder while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { QueryBuilder query = parseInnerQueryBuilder(parser); if (query instanceof SpanQueryBuilder == false) { - throw new ParsingException(parser.getTokenLocation(), "spanOr [clauses] must be of type span query"); + throw new ParsingException(parser.getTokenLocation(), "span_or [clauses] must be of type span query"); } - clauses.add((SpanQueryBuilder) query); + final SpanQueryBuilder clause = (SpanQueryBuilder) query; + checkNoBoost(NAME, currentFieldName, parser, clause); + clauses.add(clause); } } else { throw new ParsingException(parser.getTokenLocation(), "[span_or] query does not support [" + currentFieldName + "]"); @@ -132,7 +136,7 @@ public class SpanOrQueryBuilder extends AbstractQueryBuilder } if (clauses.isEmpty()) { - throw new ParsingException(parser.getTokenLocation(), "spanOr must include [clauses]"); + throw new ParsingException(parser.getTokenLocation(), "span_or must include [clauses]"); } SpanOrQueryBuilder queryBuilder = new SpanOrQueryBuilder(clauses.get(0)); diff --git a/server/src/main/java/org/elasticsearch/index/query/SpanQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/SpanQueryBuilder.java index fec1cac2696..f7bf784d6cf 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SpanQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/SpanQueryBuilder.java @@ -19,9 +19,37 @@ package org.elasticsearch.index.query; +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.xcontent.XContentParser; + /** - * Marker interface for a specific type of {@link QueryBuilder} that allows to build span queries + * Marker interface for a specific type of {@link QueryBuilder} that allows to build span queries. */ public interface SpanQueryBuilder extends QueryBuilder { + class SpanQueryBuilderUtil { + private SpanQueryBuilderUtil() { + // utility class + } + + /** + * Checks boost value of a nested span clause is equal to {@link AbstractQueryBuilder#DEFAULT_BOOST}. + * + * @param queryName a query name + * @param fieldName a field name + * @param parser a parser + * @param clause a span query builder + * @throws ParsingException if query boost value isn't equal to {@link AbstractQueryBuilder#DEFAULT_BOOST} + */ + static void checkNoBoost(String queryName, String fieldName, XContentParser parser, SpanQueryBuilder clause) { + try { + if (clause.boost() != AbstractQueryBuilder.DEFAULT_BOOST) { + throw new ParsingException(parser.getTokenLocation(), queryName + " [" + fieldName + "] " + + "as a nested span clause can't have non-default boost value [" + clause.boost() + "]"); + } + } catch (UnsupportedOperationException ignored) { + // if boost is unsupported it can't have been set + } + } + } } diff --git a/server/src/main/java/org/elasticsearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/SpanWithinQueryBuilder.java index a454dd0fb52..8f970fc25c1 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/SpanWithinQueryBuilder.java @@ -32,6 +32,8 @@ import org.elasticsearch.common.xcontent.XContentParser; import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost; + /** * Builder for {@link org.apache.lucene.search.spans.SpanWithinQuery}. */ @@ -122,12 +124,14 @@ public class SpanWithinQueryBuilder extends AbstractQueryBuilder { @@ -80,7 +82,7 @@ public class SpanContainingQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_containing [big] as a nested span clause can't have non-default boost value [2.0]")); + } + + public void testFromJsonWithNonDefaultBoostInLittleQuery() { + String json = + "{\n" + + " \"span_containing\" : {\n" + + " \"little\" : {\n" + + " \"span_near\" : {\n" + + " \"clauses\" : [ {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"bar\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " }, {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"baz\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " } ],\n" + + " \"slop\" : 5,\n" + + " \"in_order\" : true,\n" + + " \"boost\" : 2.0\n" + + " }\n" + + " },\n" + + " \"big\" : {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"foo\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " },\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + + Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_containing [little] as a nested span clause can't have non-default boost value [2.0]")); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanFirstQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanFirstQueryBuilderTests.java index 71539939c8c..2ac3610f2d6 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanFirstQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanFirstQueryBuilderTests.java @@ -31,6 +31,7 @@ import org.elasticsearch.test.AbstractQueryTestCase; import java.io.IOException; import static org.elasticsearch.index.query.QueryBuilders.spanTermQuery; +import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; public class SpanFirstQueryBuilderTests extends AbstractQueryTestCase { @@ -59,7 +60,7 @@ public class SpanFirstQueryBuilderTests extends AbstractQueryTestCase parseQuery(Strings.toString(builder))); - assertTrue(e.getMessage().contains("spanFirst must have [end] set")); + assertTrue(e.getMessage().contains("span_first must have [end] set")); } { XContentBuilder builder = XContentFactory.jsonBuilder(); @@ -70,7 +71,7 @@ public class SpanFirstQueryBuilderTests extends AbstractQueryTestCase parseQuery(Strings.toString(builder))); - assertTrue(e.getMessage().contains("spanFirst must have [match] span query clause")); + assertTrue(e.getMessage().contains("span_first must have [match] span query clause")); } } @@ -97,4 +98,27 @@ public class SpanFirstQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_first [match] as a nested span clause can't have non-default boost value [2.0]")); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTests.java index 0e22f33db77..cde83fb6f74 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTests.java @@ -114,7 +114,7 @@ public class SpanNearQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_near [clauses] as a nested span clause can't have non-default boost value [2.0]")); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanNotQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanNotQueryBuilderTests.java index 1ffa85de0ec..7df58553e27 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanNotQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanNotQueryBuilderTests.java @@ -132,7 +132,7 @@ public class SpanNotQueryBuilderTests extends AbstractQueryTestCase parseQuery(Strings.toString(builder))); - assertThat(e.getDetailedMessage(), containsString("spanNot must have [include]")); + assertThat(e.getDetailedMessage(), containsString("span_not must have [include]")); } { XContentBuilder builder = XContentFactory.jsonBuilder(); @@ -146,7 +146,7 @@ public class SpanNotQueryBuilderTests extends AbstractQueryTestCase parseQuery(Strings.toString(builder))); - assertThat(e.getDetailedMessage(), containsString("spanNot must have [exclude]")); + assertThat(e.getDetailedMessage(), containsString("span_not must have [exclude]")); } { XContentBuilder builder = XContentFactory.jsonBuilder(); @@ -163,7 +163,7 @@ public class SpanNotQueryBuilderTests extends AbstractQueryTestCase parseQuery(Strings.toString(builder))); - assertThat(e.getDetailedMessage(), containsString("spanNot can either use [dist] or [pre] & [post] (or none)")); + assertThat(e.getDetailedMessage(), containsString("span_not can either use [dist] or [pre] & [post] (or none)")); } } @@ -203,7 +203,7 @@ public class SpanNotQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_not [include] as a nested span clause can't have non-default boost value [2.0]")); + } + + + public void testFromJsonWithNonDefaultBoostInExcludeQuery() { + String json = + "{\n" + + " \"span_not\" : {\n" + + " \"include\" : {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"hoya\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " },\n" + + " \"exclude\" : {\n" + + " \"span_near\" : {\n" + + " \"clauses\" : [ {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"la\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " }, {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"hoya\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " } ],\n" + + " \"slop\" : 0,\n" + + " \"in_order\" : true,\n" + + " \"boost\" : 2.0\n" + + " }\n" + + " },\n" + + " \"pre\" : 0,\n" + + " \"post\" : 0,\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + + Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_not [exclude] as a nested span clause can't have non-default boost value [2.0]")); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanOrQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanOrQueryBuilderTests.java index a480681a5f4..9497cebc4ce 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanOrQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanOrQueryBuilderTests.java @@ -22,6 +22,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.Query; import org.apache.lucene.search.spans.SpanOrQuery; import org.apache.lucene.search.spans.SpanQuery; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.test.AbstractQueryTestCase; @@ -94,7 +95,7 @@ public class SpanOrQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_or [clauses] as a nested span clause can't have non-default boost value [2.0]")); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java index 34dc08d29fb..a5ef596e025 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java @@ -76,19 +76,16 @@ public class SpanTermQueryBuilderTests extends AbstractTermQueryTestCase { @@ -80,7 +82,7 @@ public class SpanWithinQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_within [big] as a nested span clause can't have non-default boost value [2.0]")); + } + + public void testFromJsonWithNonDefaultBoostInLittleQuery() { + String json = + "{\n" + + " \"span_within\" : {\n" + + " \"little\" : {\n" + + " \"span_near\" : {\n" + + " \"clauses\" : [ {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"bar\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " }, {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"baz\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " } ],\n" + + " \"slop\" : 5,\n" + + " \"in_order\" : true,\n" + + " \"boost\" : 2.0\n" + + " }\n" + + " },\n" + + " \"big\" : {\n" + + " \"span_term\" : {\n" + + " \"field1\" : {\n" + + " \"value\" : \"foo\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + " },\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + + Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); + assertThat(exception.getMessage(), + equalTo("span_within [little] as a nested span clause can't have non-default boost value [2.0]")); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java index d9c6b89e8df..7c5aa31722b 100644 --- a/server/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java @@ -39,7 +39,12 @@ import java.io.UnsupportedEncodingException; public class WrapperQueryBuilderTests extends AbstractQueryTestCase { @Override - protected boolean supportsBoostAndQueryName() { + protected boolean supportsBoost() { + return false; + } + + @Override + protected boolean supportsQueryName() { return false; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index cebd63dfa32..2f46b8a887c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -20,7 +20,6 @@ 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; @@ -83,15 +82,16 @@ public abstract class AbstractQueryTestCase> private static final int NUMBER_OF_TESTQUERIES = 20; public final QB createTestQueryBuilder() { + return createTestQueryBuilder(supportsBoost(), supportsQueryName()); + } + + public final QB createTestQueryBuilder(boolean supportsBoost, boolean supportsQueryName) { QB query = doCreateTestQueryBuilder(); - //we should not set boost and query name for queries that don't parse it - if (supportsBoostAndQueryName()) { - if (randomBoolean()) { - query.boost(2.0f / randomIntBetween(1, 20)); - } - if (randomBoolean()) { - query.queryName(createUniqueRandomName()); - } + if (supportsBoost && randomBoolean()) { + query.boost(2.0f / randomIntBetween(1, 20)); + } + if (supportsQueryName && randomBoolean()) { + query.queryName(createUniqueRandomName()); } return query; } @@ -460,7 +460,7 @@ public abstract class AbstractQueryTestCase> rewrite(secondLuceneQuery), rewrite(firstLuceneQuery)); } - if (supportsBoostAndQueryName()) { + if (supportsBoost()) { secondQuery.boost(firstQuery.boost() + 1f + randomFloat()); Query thirdLuceneQuery = rewriteQuery(secondQuery, context).toQuery(context); assertNotEquals("modifying the boost doesn't affect the corresponding lucene query", rewrite(firstLuceneQuery), @@ -487,12 +487,22 @@ public abstract class AbstractQueryTestCase> } /** - * Few queries allow you to set the boost and queryName on the java api, although the corresponding parser - * doesn't parse them as they are not supported. This method allows to disable boost and queryName related tests for those queries. - * Those queries are easy to identify: their parsers don't parse `boost` and `_name` as they don't apply to the specific query: - * wrapper query and match_none + * Few queries allow you to set the boost on the Java API, although the corresponding parser + * doesn't parse it as it isn't supported. This method allows to disable boost related tests for those queries. + * Those queries are easy to identify: their parsers don't parse {@code boost} as they don't apply to the specific query: + * wrapper query and {@code match_none}. */ - protected boolean supportsBoostAndQueryName() { + protected boolean supportsBoost() { + return true; + } + + /** + * Few queries allow you to set the query name on the Java API, although the corresponding parser + * doesn't parse it as it isn't supported. This method allows to disable query name related tests for those queries. + * Those queries are easy to identify: their parsers don't parse {@code _name} as they don't apply to the specific query: + * wrapper query and {@code match_none}. + */ + protected boolean supportsQueryName() { return true; }