Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw a parsing exception when boost is set in span_or query (#28390) #34112

Merged
merged 48 commits into from
Nov 26, 2018
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
6c12c4a
Throw a parsing exception when boost is set in span_or query (#28390)
cbismuth Sep 27, 2018
265acde
Throw a parsing exception when boost is parsed in span_or query (#28390)
cbismuth Sep 27, 2018
268b78e
Check boost field only in SpanOrQueryBuilder#fromXContent API (#28390)
cbismuth Oct 4, 2018
6effd0a
Throw a parsing exception only when boost differs from default (#28390)
cbismuth Oct 4, 2018
6349cb3
Set per query builder "boost" and "query name" support (#28390)
cbismuth Oct 5, 2018
e8b61bd
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Oct 5, 2018
04f66ce
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Oct 10, 2018
8029eb2
Allow non-default boost value in outer "span_or" query(#28390)
cbismuth Oct 10, 2018
0f95a83
Revert "Set per query builder "boost" and "query name" support (#28390)"
cbismuth Oct 10, 2018
e94a942
Allow disabling boost and query name in test query builders (#28390)
cbismuth Oct 10, 2018
7c348bf
Restore previous boost and query name API (#28390)
cbismuth Oct 10, 2018
a20d761
Disallow setting boost in "span_containing" queries (#28390)
cbismuth Oct 10, 2018
e39bf1d
Disallow setting boost in "span_within" queries (#28390)
cbismuth Oct 10, 2018
bee3ef6
Disallow setting boost in "span_near" queries (#28390)
cbismuth Oct 10, 2018
c48777c
Fix "span_or" query builder exception message format (#28390)
cbismuth Oct 10, 2018
9426ffe
Disallow setting boost in "span_not" queries (#28390)
cbismuth Oct 10, 2018
af54fc8
Restore query name generation in test query builders (#28390)
cbismuth Oct 10, 2018
87da533
Ensure boost can be set in outer "span_containing" queries (#28390)
cbismuth Oct 10, 2018
95ff5ed
Ensure boost can be set in outer "span_near" queries (#28390)
cbismuth Oct 10, 2018
7d25d14
Ensure boost can be set in outer "span_not" queries (#28390)
cbismuth Oct 10, 2018
1f08196
Ensure boost can be set in outer "span_or" queries (#28390)
cbismuth Oct 10, 2018
daea8b1
Ensure boost can be set in outer "span_within" queries (#28390)
cbismuth Oct 10, 2018
70cffbe
Disallow setting boost in "span_gap" queries (#28390)
cbismuth Oct 10, 2018
9e433b3
Extract method to check no boost value has been set (#28390)
cbismuth Oct 10, 2018
006b37d
Reuse query name in exception message (#28390)
cbismuth Oct 10, 2018
8f8e035
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Oct 25, 2018
e8ec5c1
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 6, 2018
a5dd5b8
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 14, 2018
49ce791
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 16, 2018
0ac11ec
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 16, 2018
a88866e
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 18, 2018
9c751b1
Move check method to base SpanQueryBuilder class (#28390)
cbismuth Nov 19, 2018
8a75747
Improve "checkNoBoost" exception message (#28390)
cbismuth Nov 19, 2018
8e9c70d
Split `supportsBoost` and `supportsQueryName` test API (#28390)
cbismuth Nov 19, 2018
f2861cd
Remove unnecessary `supportsQueryName` parameter in test API (#28390)
cbismuth Nov 19, 2018
df03f38
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 19, 2018
11f6060
Remove boost support from span query test API (#28390)
cbismuth Nov 20, 2018
043020b
Remove underscore characters in test names (#28390)
cbismuth Nov 20, 2018
c96aaa9
Remove boost support from `span_first` query (#28390)
cbismuth Nov 20, 2018
3b667bf
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 20, 2018
259c446
Add migration to 7.x breaking change disclaimer (#28390)
cbismuth Nov 20, 2018
e486466
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 21, 2018
78e8c21
Update documentation (#28390)
cbismuth Nov 21, 2018
16127d9
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 22, 2018
1fb94ee
Revert "Update documentation (#28390)"
cbismuth Nov 22, 2018
94b5bcb
Fix line length to 80 characters (#28390)
cbismuth Nov 22, 2018
dc1e913
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 22, 2018
309e00b
Merge branch 'master' into 28390_disallow_setting_boosts_on_inner_spa…
cbismuth Nov 23, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
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}.
*/
Expand Down Expand Up @@ -117,12 +119,14 @@ public static SpanContainingQueryBuilder fromXContent(XContentParser parser) thr
throw new ParsingException(parser.getTokenLocation(), "span_containing [big] must be of type span query");
}
big = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, big);
} else if (LITTLE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "span_containing [little] must be of type span query");
}
little = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, little);
} else {
throw new ParsingException(parser.getTokenLocation(),
"[span_containing] query does not support [" + currentFieldName + "]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import java.io.IOException;
import java.util.Objects;

import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost;

public class SpanFirstQueryBuilder extends AbstractQueryBuilder<SpanFirstQueryBuilder> implements SpanQueryBuilder {
public static final String NAME = "span_first";

Expand Down Expand Up @@ -115,9 +117,10 @@ public static SpanFirstQueryBuilder fromXContent(XContentParser parser) throws I
if (MATCH_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "spanFirst [match] must be of type span query");
throw new ParsingException(parser.getTokenLocation(), "span_first [match] must be of type span query");
}
match = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, match);
} else {
throw new ParsingException(parser.getTokenLocation(), "[span_first] query does not support [" + currentFieldName + "]");
}
Expand All @@ -134,10 +137,10 @@ public static SpanFirstQueryBuilder fromXContent(XContentParser parser) throws I
}
}
if (match == null) {
throw new ParsingException(parser.getTokenLocation(), "spanFirst must have [match] span query clause");
throw new ParsingException(parser.getTokenLocation(), "span_first must have [match] span query clause");
}
if (end == null) {
throw new ParsingException(parser.getTokenLocation(), "spanFirst must have [end] set for it");
throw new ParsingException(parser.getTokenLocation(), "span_first must have [end] set for it");
}
SpanFirstQueryBuilder queryBuilder = new SpanFirstQueryBuilder(match, end);
queryBuilder.boost(boost).queryName(queryName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import java.util.List;
import java.util.Objects;

import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost;

/**
* Matches spans which are near one another. One can specify slop, the maximum number
* of intervening unmatched positions, as well as whether matches are required to be in-order.
Expand Down Expand Up @@ -166,9 +168,11 @@ public static SpanNearQueryBuilder fromXContent(XContentParser parser) throws IO
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "spanNear [clauses] must be of type span query");
throw new ParsingException(parser.getTokenLocation(), "span_near [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_near] query does not support [" + currentFieldName + "]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import java.io.IOException;
import java.util.Objects;

import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost;

public class SpanNotQueryBuilder extends AbstractQueryBuilder<SpanNotQueryBuilder> implements SpanQueryBuilder {
public static final String NAME = "span_not";

Expand Down Expand Up @@ -181,15 +183,17 @@ public static SpanNotQueryBuilder fromXContent(XContentParser parser) throws IOE
if (INCLUDE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "spanNot [include] must be of type span query");
throw new ParsingException(parser.getTokenLocation(), "span_not [include] must be of type span query");
}
include = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, include);
} else if (EXCLUDE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "spanNot [exclude] must be of type span query");
throw new ParsingException(parser.getTokenLocation(), "span_not [exclude] must be of type span query");
}
exclude = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, exclude);
} else {
throw new ParsingException(parser.getTokenLocation(), "[span_not] query does not support [" + currentFieldName + "]");
}
Expand All @@ -210,13 +214,13 @@ public static SpanNotQueryBuilder fromXContent(XContentParser parser) throws IOE
}
}
if (include == null) {
throw new ParsingException(parser.getTokenLocation(), "spanNot must have [include] span query clause");
throw new ParsingException(parser.getTokenLocation(), "span_not must have [include] span query clause");
}
if (exclude == null) {
throw new ParsingException(parser.getTokenLocation(), "spanNot must have [exclude] span query clause");
throw new ParsingException(parser.getTokenLocation(), "span_not must have [exclude] span query clause");
}
if (dist != null && (pre != null || post != null)) {
throw new ParsingException(parser.getTokenLocation(), "spanNot can either use [dist] or [pre] & [post] (or none)");
throw new ParsingException(parser.getTokenLocation(), "span_not can either use [dist] or [pre] & [post] (or none)");
}

SpanNotQueryBuilder spanNotQuery = new SpanNotQueryBuilder(include, exclude);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import java.util.List;
import java.util.Objects;

import static org.elasticsearch.index.query.SpanQueryBuilder.SpanQueryBuilderUtil.checkNoBoost;

/**
* Span query that matches the union of its clauses. Maps to {@link SpanOrQuery}.
*/
Expand Down Expand Up @@ -113,9 +115,11 @@ public static SpanOrQueryBuilder fromXContent(XContentParser parser) throws IOEx
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these 3 lines are not necessary, as each inner SpanQueryBuilder within their fromXContent should check for boost and fail if boost provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compound span_or queries can contain span_term inner queries and setting boost in span_term queries is allowed, that's why I've added this check here, do you agree with it? See this test.

checkNoBoost(NAME, currentFieldName, parser, clause);
clauses.add(clause);
}
} else {
throw new ParsingException(parser.getTokenLocation(), "[span_or] query does not support [" + currentFieldName + "]");
Expand All @@ -132,7 +136,7 @@ public static SpanOrQueryBuilder fromXContent(XContentParser parser) throws IOEx
}

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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
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}.
*/
Expand Down Expand Up @@ -122,12 +124,14 @@ public static SpanWithinQueryBuilder fromXContent(XContentParser parser) throws
throw new ParsingException(parser.getTokenLocation(), "span_within [big] must be of type span query");
}
big = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, big);
} else if (LITTLE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
QueryBuilder query = parseInnerQueryBuilder(parser);
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "span_within [little] must be of type span query");
}
little = (SpanQueryBuilder) query;
checkNoBoost(NAME, currentFieldName, parser, little);
} else {
throw new ParsingException(parser.getTokenLocation(),
"[span_within] query does not support [" + currentFieldName + "]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@

import org.apache.lucene.search.Query;
import org.apache.lucene.search.spans.SpanContainingQuery;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.test.AbstractQueryTestCase;

import java.io.IOException;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;

public class SpanContainingQueryBuilderTests extends AbstractQueryTestCase<SpanContainingQueryBuilder> {
Expand Down Expand Up @@ -80,7 +82,7 @@ public void testFromJson() throws IOException {
" }\n" +
" }\n" +
" },\n" +
" \"boost\" : 1.0\n" +
" \"boost\" : 2.0\n" +
" }\n" +
"}";

Expand All @@ -89,5 +91,92 @@ public void testFromJson() throws IOException {

assertEquals(json, 2, ((SpanNearQueryBuilder) parsed.bigQuery()).clauses().size());
assertEquals(json, "foo", ((SpanTermQueryBuilder) parsed.littleQuery()).value());
assertEquals(json, 2.0, parsed.boost(), 0.0);
}

public void testFromJsoWithNonDefaultBoostInBigQuery() {
String json =
"{\n" +
" \"span_containing\" : {\n" +
" \"big\" : {\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" +
" \"little\" : {\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 [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]"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
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<SpanFirstQueryBuilder> {
Expand Down Expand Up @@ -59,7 +60,7 @@ public void testParseEnd() throws IOException {
builder.endObject();

ParsingException e = expectThrows(ParsingException.class, () -> 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();
Expand All @@ -70,7 +71,7 @@ public void testParseEnd() throws IOException {
builder.endObject();

ParsingException e = expectThrows(ParsingException.class, () -> 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"));
}
}

Expand All @@ -97,4 +98,27 @@ public void testFromJson() throws IOException {
assertEquals(json, 3, parsed.end());
assertEquals(json, "kimchy", ((SpanTermQueryBuilder) parsed.innerQuery()).value());
}


public void testFromJsonWithNonDefaultBoostInMatchQuery() {
String json =
"{\n" +
" \"span_first\" : {\n" +
" \"match\" : {\n" +
" \"span_term\" : {\n" +
" \"user\" : {\n" +
" \"value\" : \"kimchy\",\n" +
" \"boost\" : 2.0\n" +
" }\n" +
" }\n" +
" },\n" +
" \"end\" : 3,\n" +
" \"boost\" : 1.0\n" +
" }\n" +
"}";

Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json));
assertThat(exception.getMessage(),
equalTo("span_first [match] as a nested span clause can't have non-default boost value [2.0]"));
}
}
Loading