Fixes bug preventing script sort working on top_hits aggregation (#21023)

Previous to this change any request using a script sort in a top_hits
aggregation would fail because the compilation of the script happened
after the QueryShardContext was frozen (after we had worked out if the
request is cachable).

This change moves the calling of build() on the SortBuilder to the
TopHitsAggregationBuilder which means that the script in the script_sort
will be compiled before we decide whether to cache the request and freeze
the context.

Closes #21022

core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java

@@ -44,6 +44,7 @@ import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
 import org.elasticsearch.search.fetch.subphase.ScriptFieldsContext;
 import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
 import org.elasticsearch.search.sort.ScoreSortBuilder;
+import org.elasticsearch.search.sort.SortAndFormats;
 import org.elasticsearch.search.sort.SortBuilder;
 import org.elasticsearch.search.sort.SortBuilders;
 import org.elasticsearch.search.sort.SortOrder;
@@ -54,6 +55,7 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 
 public class TopHitsAggregationBuilder extends AbstractAggregationBuilder<TopHitsAggregationBuilder> {
@@ -539,9 +541,15 @@ public class TopHitsAggregationBuilder extends AbstractAggregationBuilder<TopHit
                     field.fieldName(), searchScript, field.ignoreFailure()));
             }
         }
-        return new TopHitsAggregatorFactory(name, type, from, size, explain, version, trackScores, sorts, highlightBuilder,
+
-            storedFieldsContext, fieldDataFields, fields, fetchSourceContext, context,
+        final Optional<SortAndFormats> optionalSort;
-            parent, subfactoriesBuilder, metaData);
+        if (sorts == null) {
+            optionalSort = Optional.empty();
+        } else {
+            optionalSort = SortBuilder.buildSort(sorts, context.searchContext().getQueryShardContext());
+        }
+        return new TopHitsAggregatorFactory(name, type, from, size, explain, version, trackScores, optionalSort, highlightBuilder,
+                storedFieldsContext, fieldDataFields, fields, fetchSourceContext, context, parent, subfactoriesBuilder, metaData);
     }
 
     @Override

core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java

@@ -32,8 +32,6 @@ import org.elasticsearch.search.fetch.subphase.ScriptFieldsContext;
 import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
 import org.elasticsearch.search.internal.SubSearchContext;
 import org.elasticsearch.search.sort.SortAndFormats;
-import org.elasticsearch.search.sort.SortBuilder;
-
 import java.io.IOException;
 import java.util.List;
 import java.util.Map;
@@ -46,7 +44,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregato
     private final boolean explain;
     private final boolean version;
     private final boolean trackScores;
-    private final List<SortBuilder<?>> sorts;
+    private final Optional<SortAndFormats> sort;
     private final HighlightBuilder highlightBuilder;
     private final StoredFieldsContext storedFieldsContext;
     private final List<String> docValueFields;
@@ -54,7 +52,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregato
     private final FetchSourceContext fetchSourceContext;
 
     public TopHitsAggregatorFactory(String name, Type type, int from, int size, boolean explain, boolean version, boolean trackScores,
-            List<SortBuilder<?>> sorts, HighlightBuilder highlightBuilder, StoredFieldsContext storedFieldsContext,
+            Optional<SortAndFormats> sort, HighlightBuilder highlightBuilder, StoredFieldsContext storedFieldsContext,
             List<String> docValueFields, List<ScriptFieldsContext.ScriptField> scriptFields, FetchSourceContext fetchSourceContext,
             AggregationContext context, AggregatorFactory<?> parent, AggregatorFactories.Builder subFactories, Map<String, Object> metaData)
             throws IOException {
@@ -64,7 +62,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregato
         this.explain = explain;
         this.version = version;
         this.trackScores = trackScores;
-        this.sorts = sorts;
+        this.sort = sort;
         this.highlightBuilder = highlightBuilder;
         this.storedFieldsContext = storedFieldsContext;
         this.docValueFields = docValueFields;
@@ -82,11 +80,8 @@ public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregato
         subSearchContext.trackScores(trackScores);
         subSearchContext.from(from);
         subSearchContext.size(size);
-        if (sorts != null) {
+        if (sort.isPresent()) {
-            Optional<SortAndFormats> optionalSort = SortBuilder.buildSort(sorts, subSearchContext.getQueryShardContext());
+            subSearchContext.sort(sort.get());
-            if (optionalSort.isPresent()) {
-                subSearchContext.sort(optionalSort.get());
-            }
         }
         if (storedFieldsContext != null) {
             subSearchContext.storedFieldsContext(storedFieldsContext);

core/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java

@@ -48,6 +48,7 @@ import org.elasticsearch.search.aggregations.metrics.max.Max;
 import org.elasticsearch.search.aggregations.metrics.tophits.TopHits;
 import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
 import org.elasticsearch.search.fetch.subphase.highlight.HighlightField;
+import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType;
 import org.elasticsearch.search.sort.SortBuilders;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ESIntegTestCase;
@@ -1010,11 +1011,23 @@ public class TopHitsIT extends ESIntegTestCase {
         assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()
                 .getMissCount(), equalTo(0L));
 
-        // Test that a request using a script does not get cached
+        // Test that a request using a script field does not get cached
         SearchResponse r = client().prepareSearch("cache_test_idx").setSize(0)
                 .addAggregation(topHits("foo").scriptField("bar", new Script("5", ScriptType.INLINE, CustomScriptPlugin.NAME, null))).get();
         assertSearchResponse(r);
 
+        assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()
+                .getHitCount(), equalTo(0L));
+        assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()
+                .getMissCount(), equalTo(0L));
+
+        // Test that a request using a script sort does not get cached
+        r = client().prepareSearch("cache_test_idx").setSize(0)
+                .addAggregation(topHits("foo").sort(
+                        SortBuilders.scriptSort(new Script("5", ScriptType.INLINE, CustomScriptPlugin.NAME, null), ScriptSortType.STRING)))
+                .get();
+        assertSearchResponse(r);
+
         assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()
                 .getHitCount(), equalTo(0L));
         assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()