Browse Source

Fix percentiles agg in slow log after transport (forward port of #70318) (#70412)

If you send `percentiles` or `percentiles_ranks` over the transport
client or over cross cluster search it breaks internal components
subtly. They mostly work so we hadn't yet noticed the break. But if you
send the request to the slow log then it'll fail to log. This fixes the
subtle internal break.
Nik Everett 4 years ago
parent
commit
17d56d361a

+ 3 - 2
server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

@@ -36,10 +36,10 @@ public abstract class AbstractPercentilesAggregationBuilder<T extends AbstractPe
     extends ValuesSourceAggregationBuilder.LeafOnly<ValuesSource, T> {
 
     public static final ParseField KEYED_FIELD = new ParseField("keyed");
+    private final ParseField valuesField;
     protected boolean keyed = true;
     protected double[] values;
     private PercentilesConfig percentilesConfig;
-    private ParseField valuesField;
 
     public static <T extends AbstractPercentilesAggregationBuilder<T>> ConstructingObjectParser<T, String> createParser(String aggName,
                                                          TriFunction<String, double[], PercentilesConfig, T> ctor,
@@ -129,8 +129,9 @@ public abstract class AbstractPercentilesAggregationBuilder<T extends AbstractPe
         this.valuesField = clone.valuesField;
     }
 
-    AbstractPercentilesAggregationBuilder(StreamInput in) throws IOException {
+    AbstractPercentilesAggregationBuilder(ParseField valuesField, StreamInput in) throws IOException {
         super(in);
+        this.valuesField = valuesField;
         values = in.readDoubleArray();
         keyed = in.readBoolean();
         if (in.getVersion().onOrAfter(Version.V_7_8_0)) {

+ 1 - 1
server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java

@@ -50,7 +50,7 @@ public class PercentileRanksAggregationBuilder extends AbstractPercentilesAggreg
     }
 
     public PercentileRanksAggregationBuilder(StreamInput in) throws IOException {
-        super(in);
+        super(VALUES_FIELD, in);
     }
 
     private PercentileRanksAggregationBuilder(PercentileRanksAggregationBuilder clone,

+ 1 - 1
server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java

@@ -52,7 +52,7 @@ public class PercentilesAggregationBuilder extends AbstractPercentilesAggregatio
     }
 
     public PercentilesAggregationBuilder(StreamInput in) throws IOException {
-        super(in);
+        super(PERCENTS_FIELD, in);
     }
 
     public PercentilesAggregationBuilder(String name) {

+ 13 - 0
server/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsTests.java

@@ -8,7 +8,9 @@
 
 package org.elasticsearch.search.aggregations.metrics;
 
+import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.search.aggregations.AggregationInitializationException;
 import org.elasticsearch.search.aggregations.AggregatorFactories;
@@ -19,7 +21,9 @@ import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType;
 import org.elasticsearch.search.sort.SortBuilders;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.AbstractQueryTestCase;
+import org.elasticsearch.test.hamcrest.ElasticsearchAssertions;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -189,4 +193,13 @@ public class TopHitsTests extends BaseAggregationTestCase<TopHitsAggregationBuil
         assertThat(e.toString(), containsString("Aggregator [top_tags_hits] of type [top_hits] cannot accept sub-aggregations"));
     }
 
+    @Override
+    protected void assertToXContentAfterSerialization(TopHitsAggregationBuilder original, TopHitsAggregationBuilder deserialized)
+        throws IOException {
+        ElasticsearchAssertions.assertToXContentEquivalent(
+            XContentHelper.toXContent(original, XContentType.JSON, false),
+            XContentHelper.toXContent(deserialized, XContentType.JSON, false),
+            XContentType.JSON
+        );
+    }
 }

+ 10 - 0
test/framework/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java

@@ -145,10 +145,20 @@ public abstract class BaseAggregationTestCase<AB extends AbstractAggregationBuil
                 assertEquals(testAgg, deserialized);
                 assertEquals(testAgg.hashCode(), deserialized.hashCode());
                 assertNotSame(testAgg, deserialized);
+                @SuppressWarnings("unchecked") // They are .equal so its safe
+                AB castDeserialized = (AB) deserialized;
+                assertToXContentAfterSerialization(testAgg, castDeserialized);
             }
         }
     }
 
+    /**
+     * Make sure serialization preserves toXContent.
+     */
+    protected void assertToXContentAfterSerialization(AB original, AB deserialized) throws IOException {
+        assertEquals(Strings.toString(original), Strings.toString(deserialized));
+    }
+
     public void testEqualsAndHashcode() throws IOException {
         // TODO we only change name and boost, we should extend by any sub-test supplying a "mutate" method that randomly changes one
         // aspect of the object under test