From b6014d971c408145c0bca78117a5bbee609052f7 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Thu, 22 Nov 2018 06:08:48 -0500 Subject: [PATCH] Forbid negative scores in functon_score query (#35709) * Forbid negative scores in functon_score query - Throw an exception when scores are negative in field_value_factor function - Throw an exception when scores are negative in script_score function Relates to #33309 --- .../migration/migrate_7_0/search.asciidoc | 10 ++++- .../query-dsl/function-score-query.asciidoc | 6 +++ .../rest-api-spec/test/painless/30_search.yml | 38 +++++++++++++++++++ .../function/FieldValueFactorFunction.java | 4 ++ .../search/function/FunctionScoreQuery.java | 2 +- .../search/function/ScriptScoreFunction.java | 3 ++ .../functionscore/FunctionScoreTests.java | 14 +++++++ 7 files changed, 75 insertions(+), 2 deletions(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index efd2d2c271a..2576a0e5136 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -148,4 +148,12 @@ instead which is a more appropriate value for a scenario where scores are not av ==== 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. \ No newline at end of file +To deboost a specific query you can use a `boost` comprise between 0 and 1. + +[float] +==== Negative scores are not allowed in Function Score Query + +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 diff --git a/docs/reference/query-dsl/function-score-query.asciidoc b/docs/reference/query-dsl/function-score-query.asciidoc index 7c5ca95623e..71fa61ee085 100644 --- a/docs/reference/query-dsl/function-score-query.asciidoc +++ b/docs/reference/query-dsl/function-score-query.asciidoc @@ -153,6 +153,9 @@ GET /_search // CONSOLE // TEST[setup:twitter] +NOTE: Scores produced by the `script_score` function must be non-negative, +otherwise an error will be thrown. + On top of the different scripting field values and expression, the `_score` script parameter can be used to retrieve the score based on the wrapped query. @@ -324,6 +327,9 @@ There are a number of options for the `field_value_factor` function: values of the field with a range filter to avoid this, or use `log1p` and `ln1p`. + NOTE: Scores produced by the `field_value_score` function must be non-negative, + otherwise an error will be thrown. + [[function-decay]] ==== Decay functions diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml index 0c0e980d95a..8decbffe84d 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml @@ -442,3 +442,41 @@ - match: { error.root_cause.0.reason: "Iterable object is self-referencing itself (ScriptBytesValues value)" } - match: { error.type: "search_phase_execution_exception" } - match: { error.reason: "all shards failed" } + + +--- + +"Exception on negative score": + - skip: + version: " - 6.99.99" + reason: "check on negative scores was added from 7.0.0 on" + + - do: + index: + index: test + type: test + id: 1 + body: { "test": "value beck", "num1": 1.0 } + - do: + indices.refresh: {} + + - do: + catch: bad_request + search: + index: test + body: + query: + function_score: + query: + term: + test: value + "functions": [{ + "script_score": { + "script": { + "lang": "painless", + "source": "doc['num1'].value - 10.0" + } + } + }] + - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.reason: "script score function must not produce negative scores, but got: [-9.0]"} diff --git a/server/src/main/java/org/elasticsearch/common/lucene/search/function/FieldValueFactorFunction.java b/server/src/main/java/org/elasticsearch/common/lucene/search/function/FieldValueFactorFunction.java index fb5a82bc098..580ea97de15 100644 --- a/server/src/main/java/org/elasticsearch/common/lucene/search/function/FieldValueFactorFunction.java +++ b/server/src/main/java/org/elasticsearch/common/lucene/search/function/FieldValueFactorFunction.java @@ -83,6 +83,10 @@ public class FieldValueFactorFunction extends ScoreFunction { } double val = value * boostFactor; double result = modifier.apply(val); + if (result < 0f) { + throw new IllegalArgumentException("field value function must not produce negative scores, but got: [" + + result + "] for field value: [" + value + "]"); + } return result; } diff --git a/server/src/main/java/org/elasticsearch/common/lucene/search/function/FunctionScoreQuery.java b/server/src/main/java/org/elasticsearch/common/lucene/search/function/FunctionScoreQuery.java index fe0901c6cdb..95fbed64a25 100644 --- a/server/src/main/java/org/elasticsearch/common/lucene/search/function/FunctionScoreQuery.java +++ b/server/src/main/java/org/elasticsearch/common/lucene/search/function/FunctionScoreQuery.java @@ -382,7 +382,7 @@ public class FunctionScoreQuery extends Query { } double factor = computeScore(docId, subQueryScore); float finalScore = scoreCombiner.combine(subQueryScore, factor, maxBoost); - if (finalScore == Float.NEGATIVE_INFINITY || Float.isNaN(finalScore)) { + if (finalScore < 0f || Float.isNaN(finalScore)) { /* These scores are invalid for score based {@link org.apache.lucene.search.TopDocsCollector}s. See {@link org.apache.lucene.search.TopScoreDocCollector} for details. diff --git a/server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java b/server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java index 3893572aa44..8e51bc5951d 100644 --- a/server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java +++ b/server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java @@ -69,6 +69,9 @@ public class ScriptScoreFunction extends ScoreFunction { scorer.docid = docId; scorer.score = subQueryScore; double result = leafScript.execute(); + if (result < 0f) { + throw new IllegalArgumentException("script score function must not produce negative scores, but got: [" + result + "]"); + } return result; } diff --git a/server/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreTests.java b/server/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreTests.java index 9b11017e598..fa0a372a2ad 100644 --- a/server/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreTests.java @@ -824,6 +824,20 @@ public class FunctionScoreTests extends ESTestCase { assertThat(exc.getMessage(), containsString("function score query returned an invalid score: " + Float.NEGATIVE_INFINITY)); } + + public void testExceptionOnNegativeScores() { + IndexSearcher localSearcher = new IndexSearcher(reader); + TermQuery termQuery = new TermQuery(new Term(FIELD, "out")); + + // test that field_value_factor function throws an exception on negative scores + FieldValueFactorFunction.Modifier modifier = FieldValueFactorFunction.Modifier.NONE; + final ScoreFunction fvfFunction = new FieldValueFactorFunction(FIELD, -10, modifier, 1.0, new IndexNumericFieldDataStub()); + FunctionScoreQuery fsQuery1 = + new FunctionScoreQuery(termQuery, fvfFunction, CombineFunction.REPLACE, null, Float.POSITIVE_INFINITY); + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> localSearcher.search(fsQuery1, 1)); + assertThat(exc.getMessage(), containsString("field value function must not produce negative scores")); + } + private static class DummyScoreFunction extends ScoreFunction { protected DummyScoreFunction(CombineFunction scoreCombiner) { super(scoreCombiner);