From bf9bd0dd7845b9c34ad8283d3d523412f6d27882 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 28 Jan 2019 16:08:15 +0100 Subject: [PATCH 1/3] Forbid negative field boosts in analyzed queries 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 --- .../reference/migration/migrate_6_0/search.asciidoc | 8 ++++---- .../index/query/AbstractQueryBuilder.java | 13 +++++++++---- .../index/query/MultiMatchQueryBuilder.java | 9 ++++++++- .../index/query/QueryStringQueryBuilder.java | 9 ++++++++- .../index/query/SimpleQueryStringBuilder.java | 5 +++++ .../elasticsearch/index/search/MultiMatchQuery.java | 3 +-- .../index/query/MultiMatchQueryBuilderTests.java | 10 ++++++++++ .../index/query/QueryStringQueryBuilderTests.java | 10 ++++++++++ .../index/query/SimpleQueryStringBuilderTests.java | 10 ++++++++++ 9 files changed, 65 insertions(+), 12 deletions(-) diff --git a/docs/reference/migration/migrate_6_0/search.asciidoc b/docs/reference/migration/migrate_6_0/search.asciidoc index 6283e571cc3ea..c0ed716809f54 100644 --- a/docs/reference/migration/migrate_6_0/search.asciidoc +++ b/docs/reference/migration/migrate_6_0/search.asciidoc @@ -153,10 +153,10 @@ information. [float] ===== Negative scores are deprecated in Function Score Query -Negative scores in the Function Score Query are deprecated. If a negative +Negative scores in the Function Score Query are deprecated. If a negative score is produced as a result of computation (e.g. in `script_score` or `field_value_factor` functions), a deprecation warning will be issued in -this major version, and an error will be thrown in the next major version. +this major version, and an error will be thrown in the next major version. [float] ==== Fielddata on `_uid` @@ -266,8 +266,8 @@ rewrite any prefix query on the field to a a single term query that matches the [float] ==== Negative boosts are deprecated -Setting a negative `boost` in a query is deprecated and will throw an error in the next 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 is deprecated and will throw an error in the next +major version. To deboost a specific query or field you can use a `boost` comprise between 0 and 1. [float] ==== Limit the number of open scroll contexts diff --git a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index 183d586454ca1..573d0324822dd 100644 --- a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -70,6 +70,7 @@ protected AbstractQueryBuilder() { protected AbstractQueryBuilder(StreamInput in) throws IOException { boost = in.readFloat(); + checkNegativeBoost(boost); queryName = in.readOptionalString(); } @@ -158,6 +159,13 @@ public final float boost() { return this.boost; } + protected final void checkNegativeBoost(float boost) { + if (Float.compare(boost, 0f) < 0) { + deprecationLogger.deprecatedAndMaybeLog("negative boost", "setting a negative [boost] on a query " + + "is deprecated and will throw an error in the next version. You can 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. @@ -165,10 +173,7 @@ public final float boost() { @SuppressWarnings("unchecked") @Override public final QB boost(float boost) { - if (Float.compare(boost, 0f) < 0) { - deprecationLogger.deprecatedAndMaybeLog("negative boost", "setting a negative [boost] on a query " + - "is deprecated and will throw an error in the next version. You can use a value between 0 and 1 to deboost."); - } + checkNegativeBoost(boost); this.boost = boost; return (QB) this; } diff --git a/server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java index fe3a4be57fbb0..19e8d2845aa6c 100644 --- a/server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java @@ -213,7 +213,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); @@ -293,6 +296,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; } @@ -301,6 +305,9 @@ public MultiMatchQueryBuilder field(String field, float boost) { * Add several fields to run the query against with a specific boost. */ public MultiMatchQueryBuilder fields(Map fields) { + for (float fieldBoost : fields.values()) { + checkNegativeBoost(fieldBoost); + } this.fieldsBoosts.putAll(fields); return this; } diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java index 00bb8606c75ff..177a57a918073 100644 --- a/server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java @@ -184,7 +184,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(); @@ -339,6 +342,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; } @@ -347,6 +351,9 @@ public QueryStringQueryBuilder field(String field, float boost) { * Add several fields to run the query against with a specific boost. */ public QueryStringQueryBuilder fields(Map fields) { + for (float fieldBoost : fields.values()) { + checkNegativeBoost(fieldBoost); + } this.fieldsAndWeights.putAll(fields); return this; } diff --git a/server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java b/server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java index fe53fc355578a..667e91b21523d 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java @@ -163,6 +163,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); @@ -258,6 +259,7 @@ 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; } @@ -265,6 +267,9 @@ public SimpleQueryStringBuilder field(String field, float boost) { /** Add several fields to run the query against with a specific boost. */ public SimpleQueryStringBuilder fields(Map fields) { Objects.requireNonNull(fields, "fields cannot be null"); + for (float fieldBoost : fields.values()) { + checkNegativeBoost(fieldBoost); + } this.fieldsAndWeights.putAll(fields); return this; } diff --git a/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java b/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java index 9d5f38348350c..0354df1eaaedc 100644 --- a/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java +++ b/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java @@ -96,11 +96,10 @@ private List buildFieldQueries(MultiMatchQueryBuilder.Type type, Map new MultiMatchQueryBuilder("the quick fox") + .field(STRING_FIELD_NAME, -1.0f) + .field(STRING_FIELD_NAME_2) + .toQuery(createShardContext())); + assertThat(exc.getMessage(), containsString("negative [boost]")); +>>>>>>> 89e57f53acc... Forbid negative field boosts in analyzed queries } private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) { diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index a5502359ba47a..33ad1bd99aae1 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -59,6 +59,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 org.joda.time.DateTimeZone; @@ -1488,6 +1489,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) diff --git a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java index 99ab165fb3af4..5ad6c15d0a0fc 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java @@ -58,6 +58,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; @@ -739,6 +740,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) From b59763502765c8a9b5f0a26a38b6466e7895d0d0 Mon Sep 17 00:00:00 2001 From: jimczi Date: Fri, 1 Feb 2019 12:24:04 +0100 Subject: [PATCH 2/3] add unmerged change --- .../index/query/AbstractQueryBuilder.java | 2 +- .../query/MultiMatchQueryBuilderTests.java | 17 ++++++++--------- .../query/QueryStringQueryBuilderTests.java | 15 +++++++-------- .../query/SimpleQueryStringBuilderTests.java | 15 +++++++-------- 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index 573d0324822dd..f77e6cc16ca3c 100644 --- a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -162,7 +162,7 @@ public final float boost() { protected final void checkNegativeBoost(float boost) { if (Float.compare(boost, 0f) < 0) { deprecationLogger.deprecatedAndMaybeLog("negative boost", "setting a negative [boost] on a query " + - "is deprecated and will throw an error in the next version. You can use a value between 0 and 1 to deboost."); + "is deprecated and will throw an error in the next major version. You can use a value between 0 and 1 to deboost."); } } 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 3c6fcf3e7622b..f1335d840b5c4 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MultiMatchQueryBuilderTests.java @@ -483,10 +483,9 @@ public void testWithStopWords() throws Exception { assertEquals(expected, query); } -<<<<<<< HEAD public void testDisMaxDeprecation() throws Exception { String json = - "{\n" + + "{\n" + " \"multi_match\" : {\n" + " \"query\" : \"foo:bar\",\n" + " \"use_dis_max\" : true\n" + @@ -495,15 +494,15 @@ public void testDisMaxDeprecation() throws Exception { parseQuery(json); assertWarnings("Deprecated field [use_dis_max] used, replaced by [use tie_breaker instead]"); -======= - public void testNegativeFieldBoost() { - IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, - () -> new MultiMatchQueryBuilder("the quick fox") + } + + public void testNegativeFieldBoost() throws IOException { + Query query = new MultiMatchQueryBuilder("the quick fox") .field(STRING_FIELD_NAME, -1.0f) .field(STRING_FIELD_NAME_2) - .toQuery(createShardContext())); - assertThat(exc.getMessage(), containsString("negative [boost]")); ->>>>>>> 89e57f53acc... Forbid negative field boosts in analyzed queries + .toQuery(createShardContext()); + assertWarnings("setting a negative [boost] on a query is deprecated and will throw an error in the next major " + + "version. You can use a value between 0 and 1 to deboost."); } private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) { diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index 33ad1bd99aae1..d4f6b2e2a2fc2 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -59,7 +59,6 @@ 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 org.joda.time.DateTimeZone; @@ -1489,13 +1488,13 @@ 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]")); + public void testNegativeFieldBoost() throws IOException { + Query query = new QueryStringQueryBuilder("the quick fox") + .field(STRING_FIELD_NAME, -1.0f) + .field(STRING_FIELD_NAME_2) + .toQuery(createShardContext()); + assertWarnings("setting a negative [boost] on a query is deprecated and will throw an error in the next major " + + "version. You can use a value between 0 and 1 to deboost."); } private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) { diff --git a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java index 5ad6c15d0a0fc..6cbfb79452f97 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java @@ -58,7 +58,6 @@ 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; @@ -740,13 +739,13 @@ 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]")); + public void testNegativeFieldBoost() throws IOException { + Query query = new SimpleQueryStringBuilder("the quick fox") + .field(STRING_FIELD_NAME, -1.0f) + .field(STRING_FIELD_NAME_2) + .toQuery(createShardContext()); + assertWarnings("setting a negative [boost] on a query is deprecated and will throw an error in the next major " + + "version. You can use a value between 0 and 1 to deboost."); } private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) { From 6e363be572e438d079b9f4d97d5dbe0a5dd2411c Mon Sep 17 00:00:00 2001 From: jimczi Date: Fri, 1 Feb 2019 12:53:51 +0100 Subject: [PATCH 3/3] fix expectations in tests --- .../main/java/org/elasticsearch/test/AbstractQueryTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 90c3f54cd29f5..f6942107072fa 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -105,7 +105,7 @@ public void testNegativeBoosts() { QB testQuery = createTestQueryBuilder(); testQuery.boost(-0.5f); assertWarnings("setting a negative [boost] on a query" + - " is deprecated and will throw an error in the next version. You can use a value between 0 and 1 to deboost."); + " is deprecated and will throw an error in the next major version. You can use a value between 0 and 1 to deboost."); } /**