From aa57bbd422c32e88ca7f2e12e938c9e4cc254ebf Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 22 Jul 2020 17:26:58 -0700 Subject: [PATCH] Consolidate validation for 'docvalue_fields'. (#60065) This improves modularity and also fixes some issues when `docvalues_fields` is used within `inner_hits` or the `top_hits` agg: * We previously didn't resolve wildcards in field names. * We also forgot to enforce the limit `index.max_docvalue_fields_search`. --- .../reference/migration/migrate_7_10.asciidoc | 12 +++++++++ .../search/fetch/subphase/InnerHitsIT.java | 2 +- .../index/query/InnerHitContextBuilder.java | 4 ++- .../elasticsearch/search/SearchService.java | 19 ++------------ .../metrics/TopHitsAggregatorFactory.java | 3 ++- .../fetch/subphase/FetchDocValuesContext.java | 26 ++++++++++++++++++- 6 files changed, 45 insertions(+), 21 deletions(-) diff --git a/docs/reference/migration/migrate_7_10.asciidoc b/docs/reference/migration/migrate_7_10.asciidoc index c6ac433f2dc..f09537fa998 100644 --- a/docs/reference/migration/migrate_7_10.asciidoc +++ b/docs/reference/migration/migrate_7_10.asciidoc @@ -17,4 +17,16 @@ See also <> and <>. //tag::notable-breaking-changes[] +[discrete] +[[breaking_710_search_changes]] +=== Search Changes + +[discrete] +==== Missing checks for `index.max_docvalue_fields_search` +The setting `index.max_docvalue_fields_search` limits the number of fields that +can be retrieved through `docvalue_fields` in a search. Previously we only +checked this limit when `docvalue_fields` appeared in the top-level search +request body. Now we also enforce the limit when loading doc value fields in +an `inner_hits` section or in a `top_hits` aggregation. + //end::notable-breaking-changes[] diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java index 52a02bcae17..64f5441056a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java @@ -165,7 +165,7 @@ public class InnerHitsIT extends ESIntegTestCase { .setQuery(nestedQuery("comments", matchQuery("comments.message", "fox"), ScoreMode.Avg).innerHit( new InnerHitBuilder().setHighlightBuilder(new HighlightBuilder().field("comments.message")) .setExplain(true) - .addDocValueField("comments.message") + .addDocValueField("comments.mes*") .addScriptField("script", new Script(ScriptType.INLINE, MockScriptEngine.NAME, "5", Collections.emptyMap())) .setSize(1))).get(); diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java index c55d6f1662c..fc9ab43bace 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java @@ -88,7 +88,9 @@ public abstract class InnerHitContextBuilder { innerHitsContext.storedFieldsContext(innerHitBuilder.getStoredFieldsContext()); } if (innerHitBuilder.getDocValueFields() != null) { - innerHitsContext.docValuesContext(new FetchDocValuesContext(innerHitBuilder.getDocValueFields())); + FetchDocValuesContext docValuesContext = FetchDocValuesContext.create( + queryShardContext.getMapperService(), innerHitBuilder.getDocValueFields()); + innerHitsContext.docValuesContext(docValuesContext); } if (innerHitBuilder.getScriptFields() != null) { for (SearchSourceBuilder.ScriptField field : innerHitBuilder.getScriptFields()) { diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index a605b8da26d..2f86e32dc6b 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -114,8 +114,6 @@ import org.elasticsearch.threadpool.ThreadPool.Names; import org.elasticsearch.transport.TransportRequest; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -917,21 +915,8 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv context.fetchSourceContext(source.fetchSource()); } if (source.docValueFields() != null) { - List docValueFields = new ArrayList<>(); - for (FetchDocValuesContext.FieldAndFormat format : source.docValueFields()) { - Collection fieldNames = context.mapperService().simpleMatchToFullName(format.field); - for (String fieldName: fieldNames) { - docValueFields.add(new FetchDocValuesContext.FieldAndFormat(fieldName, format.format)); - } - } - int maxAllowedDocvalueFields = context.mapperService().getIndexSettings().getMaxDocvalueFields(); - if (docValueFields.size() > maxAllowedDocvalueFields) { - throw new IllegalArgumentException( - "Trying to retrieve too many docvalue_fields. Must be less than or equal to: [" + maxAllowedDocvalueFields - + "] but was [" + docValueFields.size() + "]. This limit can be set by changing the [" - + IndexSettings.MAX_DOCVALUE_FIELDS_SEARCH_SETTING.getKey() + "] index level setting."); - } - context.docValuesContext(new FetchDocValuesContext(docValueFields)); + FetchDocValuesContext docValuesContext = FetchDocValuesContext.create(context.mapperService(), source.docValueFields()); + context.docValuesContext(docValuesContext); } if (source.highlighter() != null) { HighlightBuilder highlightBuilder = source.highlighter(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java index 142141b21b6..a0a480fbc55 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java @@ -106,7 +106,8 @@ class TopHitsAggregatorFactory extends AggregatorFactory { subSearchContext.storedFieldsContext(storedFieldsContext); } if (docValueFields != null) { - subSearchContext.docValuesContext(new FetchDocValuesContext(docValueFields)); + FetchDocValuesContext docValuesContext = FetchDocValuesContext.create(searchContext.mapperService(), docValueFields); + subSearchContext.docValuesContext(docValuesContext); } for (ScriptFieldsContext.ScriptField field : scriptFields) { subSearchContext.scriptFields().add(field); diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java index 83fdee28823..6b5daf3493c 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java @@ -28,8 +28,12 @@ import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser.Token; +import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.mapper.MapperService; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Objects; @@ -113,7 +117,27 @@ public class FetchDocValuesContext { private final List fields; - public FetchDocValuesContext(List fields) { + public static FetchDocValuesContext create(MapperService mapperService, + List fieldPatterns) { + List fields = new ArrayList<>(); + for (FieldAndFormat field : fieldPatterns) { + Collection fieldNames = mapperService.simpleMatchToFullName(field.field); + for (String fieldName: fieldNames) { + fields.add(new FieldAndFormat(fieldName, field.format)); + } + } + int maxAllowedDocvalueFields = mapperService.getIndexSettings().getMaxDocvalueFields(); + if (fields.size() > maxAllowedDocvalueFields) { + throw new IllegalArgumentException( + "Trying to retrieve too many docvalue_fields. Must be less than or equal to: [" + maxAllowedDocvalueFields + + "] but was [" + fields.size() + "]. This limit can be set by changing the [" + + IndexSettings.MAX_DOCVALUE_FIELDS_SEARCH_SETTING.getKey() + "] index level setting."); + } + + return new FetchDocValuesContext(fields); + } + + FetchDocValuesContext(List fields) { this.fields = fields; }