diff --git a/docs/changelog/129962.yaml b/docs/changelog/129962.yaml new file mode 100644 index 0000000000000..dd06742a74791 --- /dev/null +++ b/docs/changelog/129962.yaml @@ -0,0 +1,5 @@ +pr: 129962 +summary: Simplified Linear & RRF Retrievers - Return error on empty fields param +area: Search +type: bug +issues: [] diff --git a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/MultiFieldsInnerRetrieverUtils.java b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/MultiFieldsInnerRetrieverUtils.java index 0715b4fa67544..8aa5dbf366a7a 100644 --- a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/MultiFieldsInnerRetrieverUtils.java +++ b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/MultiFieldsInnerRetrieverUtils.java @@ -56,7 +56,7 @@ public record WeightedRetrieverSource(CompoundRetrieverBuilder.RetrieverSource r */ public static ActionRequestValidationException validateParams( List innerRetrievers, - List fields, + @Nullable List fields, @Nullable String query, String retrieverName, String retrieversParamName, @@ -64,7 +64,7 @@ public static ActionRequestValidationException validateParams( String queryParamName, ActionRequestValidationException validationException ) { - if (fields.isEmpty() == false || query != null) { + if (fields != null || query != null) { // Using the multi-fields query format if (query == null) { // Return early here because the following validation checks assume a query param value is provided @@ -87,6 +87,13 @@ public static ActionRequestValidationException validateParams( ); } + if (fields != null && fields.isEmpty()) { + validationException = addValidationError( + String.format(Locale.ROOT, "[%s] [%s] cannot be empty", retrieverName, fieldsParamName), + validationException + ); + } + if (innerRetrievers.isEmpty() == false) { validationException = addValidationError( String.format(Locale.ROOT, "[%s] cannot combine [%s] and [%s]", retrieverName, retrieversParamName, queryParamName), @@ -131,13 +138,15 @@ public static ActionRequestValidationException validateParams( * @return The inner retriever tree as a {@code RetrieverBuilder} list */ public static List generateInnerRetrievers( - List fieldsAndWeights, + @Nullable List fieldsAndWeights, String query, Collection indicesMetadata, Function, CompoundRetrieverBuilder> innerNormalizerGenerator, @Nullable Consumer weightValidator ) { - Map parsedFieldsAndWeights = QueryParserHelper.parseFieldsAndWeights(fieldsAndWeights); + Map parsedFieldsAndWeights = fieldsAndWeights != null + ? QueryParserHelper.parseFieldsAndWeights(fieldsAndWeights) + : Map.of(); if (weightValidator != null) { parsedFieldsAndWeights.values().forEach(weightValidator); } diff --git a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java index f0c36f9819af8..c1a3f7d174487 100644 --- a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java +++ b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java @@ -163,7 +163,7 @@ public LinearRetrieverBuilder( throw new IllegalArgumentException("The number of normalizers must match the number of inner retrievers"); } - this.fields = fields == null ? List.of() : List.copyOf(fields); + this.fields = fields == null ? null : List.copyOf(fields); this.query = query; this.normalizer = normalizer; this.weights = weights; @@ -400,7 +400,7 @@ public void doToXContent(XContentBuilder builder, Params params) throws IOExcept builder.endArray(); } - if (fields.isEmpty() == false) { + if (fields != null) { builder.startArray(FIELDS_FIELD.getPreferredName()); for (String field : fields) { builder.value(field); diff --git a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRetrieverBuilder.java b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRetrieverBuilder.java index 9fb7fdea21bb9..c05fd3bd0a11f 100644 --- a/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRetrieverBuilder.java +++ b/x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRetrieverBuilder.java @@ -115,7 +115,7 @@ public RRFRetrieverBuilder( ) { // Use a mutable list for childRetrievers so that we can use addChild super(childRetrievers == null ? new ArrayList<>() : new ArrayList<>(childRetrievers), rankWindowSize); - this.fields = fields == null ? List.of() : List.copyOf(fields); + this.fields = fields == null ? null : List.copyOf(fields); this.query = query; this.rankConstant = rankConstant; } @@ -293,7 +293,7 @@ public void doToXContent(XContentBuilder builder, Params params) throws IOExcept builder.endArray(); } - if (fields.isEmpty() == false) { + if (fields != null) { builder.startArray(FIELDS_FIELD.getPreferredName()); for (String field : fields) { builder.value(field); diff --git a/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/20_linear_retriever_simplified.yml b/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/20_linear_retriever_simplified.yml index dea4608c13dd1..7ab0d727a7383 100644 --- a/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/20_linear_retriever_simplified.yml +++ b/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/20_linear_retriever_simplified.yml @@ -465,6 +465,18 @@ setup: - contains: { error.root_cause.0.reason: "[linear] [query] cannot be empty" } + - do: + catch: bad_request + search: + index: test-index + body: + retriever: + linear: + fields: [ ] + query: "foo" + + - contains: { error.root_cause.0.reason: "[linear] [fields] cannot be empty" } + - do: catch: bad_request search: diff --git a/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/rrf/310_rrf_retriever_simplified.yml b/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/rrf/310_rrf_retriever_simplified.yml index 0fd15a0ad2b73..cd03280691929 100644 --- a/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/rrf/310_rrf_retriever_simplified.yml +++ b/x-pack/plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/rrf/310_rrf_retriever_simplified.yml @@ -359,6 +359,18 @@ setup: - contains: { error.root_cause.0.reason: "[rrf] [query] cannot be empty" } + - do: + catch: bad_request + search: + index: test-index + body: + retriever: + rrf: + fields: [ ] + query: "foo" + + - contains: { error.root_cause.0.reason: "[rrf] [fields] cannot be empty" } + - do: catch: bad_request search: