From 4e613139dc884b242adedb9a209f20c3eb0954b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 30 Jan 2017 19:28:55 +0100 Subject: [PATCH] Ensure fixed serialization order of InnerHitBuilder (#22820) Usually the order in which we serialize sets and maps of things doesn't matter, but since InnerHitBuilder is part of SearchSourceBuilder, which is in turn used as a cache key in its bytes serialization, we need to ensure the order of all these fields when writing them to an output stream. This adds tests and makes sure we iterate over the scriptField set and the childInnerHits map in a fixed order. Closes #22808 --- .../index/query/InnerHitBuilder.java | 12 ++++++-- .../index/query/InnerHitBuilderTests.java | 28 ++++++++++++++++++- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java b/core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java index 9ef9f2998b0..c1b240066ab 100644 --- a/core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java @@ -48,6 +48,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -266,8 +267,10 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl out.writeBoolean(hasScriptFields); if (hasScriptFields) { out.writeVInt(scriptFields.size()); - for (ScriptField scriptField : scriptFields) { - scriptField.writeTo(out); + Iterator iterator = scriptFields.stream() + .sorted((a, b) -> a.fieldName().compareTo(b.fieldName())).iterator(); + while (iterator.hasNext()) { + iterator.next().writeTo(out); } } out.writeOptionalWriteable(fetchSourceContext); @@ -285,7 +288,10 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl out.writeBoolean(hasChildInnerHits); if (hasChildInnerHits) { out.writeVInt(childInnerHits.size()); - for (Map.Entry entry : childInnerHits.entrySet()) { + Iterator> iterator = childInnerHits.entrySet().stream() + .sorted((a, b) -> a.getKey().compareTo(b.getKey())).iterator(); + while (iterator.hasNext()) { + Map.Entry entry = iterator.next(); out.writeString(entry.getKey()); entry.getValue().writeTo(out); } diff --git a/core/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java index 6ebe14620f9..3f480262f49 100644 --- a/core/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.join.ScoreMode; +import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -36,6 +37,7 @@ import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import org.elasticsearch.search.fetch.subphase.InnerHitsContext; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilderTests; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.search.internal.ShardSearchLocalRequest; import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; @@ -95,6 +97,30 @@ public class InnerHitBuilderTests extends ESTestCase { } } + /** + * Test that if we serialize and deserialize an object, further + * serialization leads to identical bytes representation. + * + * This is necessary to ensure because we use the serialized BytesReference + * of this builder as part of the cacheKey in + * {@link ShardSearchLocalRequest} (via + * {@link SearchSourceBuilder#collapse(org.elasticsearch.search.collapse.CollapseBuilder)}) + */ + public void testSerializationOrder() throws Exception { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + InnerHitBuilder original = randomInnerHits(); + InnerHitBuilder deserialized = serializedCopy(original); + assertEquals(deserialized, original); + assertEquals(deserialized.hashCode(), original.hashCode()); + assertNotSame(deserialized, original); + BytesStreamOutput out1 = new BytesStreamOutput(); + BytesStreamOutput out2 = new BytesStreamOutput(); + original.writeTo(out1); + deserialized.writeTo(out2); + assertEquals(out1.bytes(), out2.bytes()); + } + } + public void testFromAndToXContent() throws Exception { for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { InnerHitBuilder innerHit = randomInnerHits(true, false); @@ -114,7 +140,7 @@ public class InnerHitBuilderTests extends ESTestCase { } } - public void testEqualsAndHashcode() throws IOException { + public void testEqualsAndHashcode() { for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { checkEqualsAndHashCode(randomInnerHits(), InnerHitBuilderTests::serializedCopy, InnerHitBuilderTests::mutate); }