Browse Source

Forbid negative field boosts in analyzed queries (#37930)

This change forbids negative field boost in the `query_string`, `simple_query_string`
and `multi_match` queries.
Negative boosts are not allowed in Lucene 8 (scores must be positive).
The backport of this change to 6x will turn the error into a deprecation warning
in order to raise the awareness of this breaking change in 7.0.

Closes #33309
Jim Ferenczi 6 years ago
parent
commit
6fa93ca493

+ 2 - 2
docs/reference/migration/migrate_7_0/search.asciidoc

@@ -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

+ 9 - 4
server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java

@@ -64,6 +64,7 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
 
     protected AbstractQueryBuilder(StreamInput in) throws IOException {
         boost = in.readFloat();
+        checkNegativeBoost(boost);
         queryName = in.readOptionalString();
     }
 
@@ -139,6 +140,13 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
         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.
@@ -146,10 +154,7 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
     @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;
     }

+ 8 - 1
server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java

@@ -211,7 +211,10 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder<MultiMatchQuery
         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);
@@ -295,6 +298,7 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder<MultiMatchQuery
         if (Strings.isEmpty(field)) {
             throw new IllegalArgumentException("supplied field is null or empty.");
         }
+        checkNegativeBoost(boost);
         this.fieldsBoosts.put(field, boost);
         return this;
     }
@@ -303,6 +307,9 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder<MultiMatchQuery
      * 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;
     }

+ 8 - 1
server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java

@@ -169,7 +169,10 @@ public class QueryStringQueryBuilder extends AbstractQueryBuilder<QueryStringQue
         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();
@@ -264,6 +267,7 @@ public class QueryStringQueryBuilder extends AbstractQueryBuilder<QueryStringQue
      * 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;
     }
@@ -272,6 +276,9 @@ public class QueryStringQueryBuilder extends AbstractQueryBuilder<QueryStringQue
      * 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;
     }

+ 5 - 0
server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java

@@ -161,6 +161,7 @@ public class SimpleQueryStringBuilder extends AbstractQueryBuilder<SimpleQuerySt
         for (int i = 0; i < size; i++) {
             String field = in.readString();
             Float weight = in.readFloat();
+            checkNegativeBoost(weight);
             fields.put(field, weight);
         }
         fieldsAndWeights.putAll(fields);
@@ -223,6 +224,7 @@ public class SimpleQueryStringBuilder extends AbstractQueryBuilder<SimpleQuerySt
         if (Strings.isEmpty(field)) {
             throw new IllegalArgumentException("supplied field is null or empty");
         }
+        checkNegativeBoost(boost);
         this.fieldsAndWeights.put(field, boost);
         return this;
     }
@@ -230,6 +232,9 @@ public class SimpleQueryStringBuilder extends AbstractQueryBuilder<SimpleQuerySt
     /** 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;
     }

+ 1 - 2
server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java

@@ -97,11 +97,10 @@ public class MultiMatchQuery extends MatchQuery {
                 // 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);

+ 9 - 0
server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java

@@ -460,6 +460,15 @@ public class MultiMatchQueryBuilderTests extends AbstractQueryTestCase<MultiMatc
         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)

+ 10 - 0
server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java

@@ -63,6 +63,7 @@ import org.elasticsearch.index.mapper.MapperService;
 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;
@@ -1471,6 +1472,15 @@ public class QueryStringQueryBuilderTests extends AbstractQueryTestCase<QueryStr
         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)

+ 10 - 0
server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java

@@ -57,6 +57,7 @@ import java.util.Locale;
 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;
@@ -718,6 +719,15 @@ public class SimpleQueryStringBuilderTests extends AbstractQueryTestCase<SimpleQ
         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)