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

Forbid negative field boosts in analyzed queries #37930

Merged
merged 6 commits into from
Feb 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions docs/reference/migration/migrate_7_0/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ instead which is a more appropriate value for a scenario where scores are not av
[float]
==== Negative boosts are not allowed

Setting a negative `boost` in a query, deprecated in 6x, are not allowed in this version.
To deboost a specific query you can use a `boost` comprise between 0 and 1.
Setting a negative `boost` for a query or a field, deprecated in 6x, is not allowed in this version.
To deboost a specific query or field you can use a `boost` comprise between 0 and 1.

[float]
==== Negative scores are not allowed in Function Score Query
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ protected AbstractQueryBuilder() {

protected AbstractQueryBuilder(StreamInput in) throws IOException {
boost = in.readFloat();
checkNegativeBoost(boost);
queryName = in.readOptionalString();
}

Expand Down Expand Up @@ -139,17 +140,21 @@ public final float boost() {
return this.boost;
}

protected final void checkNegativeBoost(float boost) {
if (Float.compare(boost, 0f) < 0) {
throw new IllegalArgumentException("negative [boost] are not allowed in [" + toString() + "], " +
"use a value between 0 and 1 to deboost");
}
}

/**
* Sets the boost for this query. Documents matching this query will (in addition to the normal
* weightings) have their score multiplied by the boost provided.
*/
@SuppressWarnings("unchecked")
@Override
public final QB boost(float boost) {
if (Float.compare(boost, 0f) < 0) {
throw new IllegalArgumentException("negative [boost] are not allowed in [" + toString() + "], " +
"use a value between 0 and 1 to deboost");
}
checkNegativeBoost(boost);
this.boost = boost;
return (QB) this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,10 @@ public MultiMatchQueryBuilder(StreamInput in) throws IOException {
int size = in.readVInt();
fieldsBoosts = new TreeMap<>();
for (int i = 0; i < size; i++) {
fieldsBoosts.put(in.readString(), in.readFloat());
String field = in.readString();
float boost = in.readFloat();
checkNegativeBoost(boost);
fieldsBoosts.put(field, boost);
}
type = Type.readFromStream(in);
operator = Operator.readFromStream(in);
Expand Down Expand Up @@ -295,6 +298,7 @@ public MultiMatchQueryBuilder field(String field, float boost) {
if (Strings.isEmpty(field)) {
throw new IllegalArgumentException("supplied field is null or empty.");
}
checkNegativeBoost(boost);
this.fieldsBoosts.put(field, boost);
return this;
}
Expand All @@ -303,6 +307,9 @@ public MultiMatchQueryBuilder field(String field, float boost) {
* Add several fields to run the query against with a specific boost.
*/
public MultiMatchQueryBuilder fields(Map<String, Float> fields) {
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsBoosts.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ public QueryStringQueryBuilder(StreamInput in) throws IOException {
defaultField = in.readOptionalString();
int size = in.readVInt();
for (int i = 0; i < size; i++) {
fieldsAndWeights.put(in.readString(), in.readFloat());
String field = in.readString();
Float weight = in.readFloat();
checkNegativeBoost(weight);
fieldsAndWeights.put(field, weight);
}
defaultOperator = Operator.readFromStream(in);
analyzer = in.readOptionalString();
Expand Down Expand Up @@ -264,6 +267,7 @@ public QueryStringQueryBuilder field(String field) {
* Adds a field to run the query string against with a specific boost.
*/
public QueryStringQueryBuilder field(String field, float boost) {
checkNegativeBoost(boost);
this.fieldsAndWeights.put(field, boost);
return this;
}
Expand All @@ -272,6 +276,9 @@ public QueryStringQueryBuilder field(String field, float boost) {
* Add several fields to run the query against with a specific boost.
*/
public QueryStringQueryBuilder fields(Map<String, Float> fields) {
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsAndWeights.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ public SimpleQueryStringBuilder(StreamInput in) throws IOException {
for (int i = 0; i < size; i++) {
String field = in.readString();
Float weight = in.readFloat();
checkNegativeBoost(weight);
fields.put(field, weight);
}
fieldsAndWeights.putAll(fields);
Expand Down Expand Up @@ -223,13 +224,17 @@ public SimpleQueryStringBuilder field(String field, float boost) {
if (Strings.isEmpty(field)) {
throw new IllegalArgumentException("supplied field is null or empty");
}
checkNegativeBoost(boost);
this.fieldsAndWeights.put(field, boost);
return this;
}

/** Add several fields to run the query against with a specific boost. */
public SimpleQueryStringBuilder fields(Map<String, Float> fields) {
Objects.requireNonNull(fields, "fields cannot be null");
for (float fieldBoost : fields.values()) {
checkNegativeBoost(fieldBoost);
}
this.fieldsAndWeights.putAll(fields);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,10 @@ private List<Query> buildFieldQueries(MultiMatchQueryBuilder.Type type, Map<Stri
// ignore unmapped fields
continue;
}
Float boostValue = fieldNames.get(fieldName);
float boostValue = fieldNames.getOrDefault(fieldName, 1.0f);
Query query = parse(type.matchQueryType(), fieldName, value);
query = Queries.maybeApplyMinimumShouldMatch(query, minimumShouldMatch);
if (query != null
&& boostValue != null
&& boostValue != AbstractQueryBuilder.DEFAULT_BOOST
&& query instanceof MatchNoDocsQuery == false) {
query = new BoostQuery(query, boostValue);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,15 @@ public void testWithStopWords() throws Exception {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new MultiMatchQueryBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.elasticsearch.index.search.QueryStringQueryParser;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.test.AbstractQueryTestCase;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matchers;

import java.io.IOException;
Expand Down Expand Up @@ -1471,6 +1472,15 @@ public void testAnalyzedPrefix() throws Exception {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new QueryStringQueryBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), CoreMatchers.containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import java.util.Map;
import java.util.Set;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -718,6 +719,15 @@ public void testUnmappedFieldNoTokenWithAndOperator() throws IOException {
assertEquals(expected, query);
}

public void testNegativeFieldBoost() {
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
() -> new SimpleQueryStringBuilder("the quick fox")
.field(STRING_FIELD_NAME, -1.0f)
.field(STRING_FIELD_NAME_2)
.toQuery(createShardContext()));
assertThat(exc.getMessage(), containsString("negative [boost]"));
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
Expand Down