From 22242ec881003c21534fc9442e3385bda3b698ee Mon Sep 17 00:00:00 2001 From: Xiang Chen Date: Fri, 19 Aug 2016 23:24:17 +0800 Subject: [PATCH] Fix request cache key for search * Make sure indexBoost is serialized in a consistent order * remove hasIndexBoost by using indexBoost size * Make sure phrase suggester's collateParams is serialized in consistent order * Make StreamOutput writer to serialize maps in consistent order --- .../common/io/stream/StreamOutput.java | 31 +++++++++++ .../search/builder/SearchSourceBuilder.java | 38 +++++++++----- .../phrase/PhraseSuggestionBuilder.java | 2 +- .../common/io/stream/BytesStreamsTests.java | 51 +++++++++++++++++++ .../builder/SearchSourceBuilderTests.java | 21 ++++++++ 5 files changed, 128 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index 1176f54e88a..c1932fd4215 100644 --- a/core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -52,6 +52,7 @@ import java.nio.file.NotDirectoryException; import java.util.Collections; import java.util.Date; import java.util.HashMap; +import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -413,6 +414,30 @@ public abstract class StreamOutput extends OutputStream { writeGenericValue(map); } + /** + * write map to stream with consistent order + * to make sure every map generated bytes order are same. + * This method is compatible with {@code StreamInput.readMap} and {@code StreamInput.readGenericValue} + * This method only will handle the map keys order, not maps contained within the map + */ + public void writeMapWithConsistentOrder(@Nullable Map map) + throws IOException { + if (map == null) { + writeByte((byte) -1); + return; + } + assert false == (map instanceof LinkedHashMap); + this.writeByte((byte) 10); + this.writeVInt(map.size()); + Iterator> iterator = + map.entrySet().stream().sorted((a, b) -> a.getKey().compareTo(b.getKey())).iterator(); + while (iterator.hasNext()) { + Map.Entry next = iterator.next(); + this.writeString(next.getKey()); + this.writeGenericValue(next.getValue()); + } + } + /** * Write a {@link Map} of {@code K}-type keys to {@code V}-type {@link List}s. * 

@@ -553,6 +578,12 @@ public abstract class StreamOutput extends OutputStream {
         WRITERS = Collections.unmodifiableMap(writers);
     }
 
+    /**
+     * Notice: when serialization a map, the stream out map with the stream in map maybe have the
+     * different key-value orders, they will maybe have different stream order.
+     * If want to keep stream out map and stream in map have the same stream order when stream,
+     * can use {@code writeMapWithConsistentOrder}
+     */
     public void writeGenericValue(@Nullable Object value) throws IOException {
         if (value == null) {
             writeByte((byte) -1);
diff --git a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
index 2b934ea2fba..7cfc998836d 100644
--- a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
@@ -20,13 +20,13 @@
 package org.elasticsearch.search.builder;
 
 import com.carrotsearch.hppc.ObjectFloatHashMap;
-import com.carrotsearch.hppc.cursors.ObjectCursor;
 import org.elasticsearch.action.support.ToXContentToBytes;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
@@ -63,6 +63,10 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+
+import static org.elasticsearch.common.collect.Tuple.tuple;
 
 /**
  * A search source builder allowing to easily build search source. Simple
@@ -188,11 +192,10 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         storedFieldsContext = in.readOptionalWriteable(StoredFieldsContext::new);
         from = in.readVInt();
         highlightBuilder = in.readOptionalWriteable(HighlightBuilder::new);
-        boolean hasIndexBoost = in.readBoolean();
-        if (hasIndexBoost) {
-            int size = in.readVInt();
-            indexBoost = new ObjectFloatHashMap<>(size);
-            for (int i = 0; i < size; i++) {
+        int indexBoostSize = in.readVInt();
+        if (indexBoostSize > 0) {
+            indexBoost = new ObjectFloatHashMap<>(indexBoostSize);
+            for (int i = 0; i < indexBoostSize; i++) {
                 indexBoost.put(in.readString(), in.readFloat());
             }
         }
@@ -248,14 +251,10 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         out.writeOptionalWriteable(storedFieldsContext);
         out.writeVInt(from);
         out.writeOptionalWriteable(highlightBuilder);
-        boolean hasIndexBoost = indexBoost != null;
-        out.writeBoolean(hasIndexBoost);
-        if (hasIndexBoost) {
-            out.writeVInt(indexBoost.size());
-            for (ObjectCursor key : indexBoost.keys()) {
-                out.writeString(key.value);
-                out.writeFloat(indexBoost.get(key.value));
-            }
+        int indexBoostSize = indexBoost == null ? 0 : indexBoost.size();
+        out.writeVInt(indexBoostSize);
+        if (indexBoostSize > 0) {
+            writeIndexBoost(out);
         }
         out.writeOptionalFloat(minScore);
         out.writeOptionalNamedWriteable(postQueryBuilder);
@@ -304,6 +303,17 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         out.writeOptionalWriteable(sliceBuilder);
     }
 
+    private void writeIndexBoost(StreamOutput out) throws IOException {
+        List> ibs = StreamSupport
+            .stream(indexBoost.spliterator(), false)
+            .map(i -> tuple(i.key, i.value)).sorted((o1, o2) -> o1.v1().compareTo(o2.v1()))
+            .collect(Collectors.toList());
+        for (Tuple ib : ibs) {
+            out.writeString(ib.v1());
+            out.writeFloat(ib.v2());
+        }
+    }
+
     /**
      * Sets the search query for this request.
      *
diff --git a/core/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestionBuilder.java b/core/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestionBuilder.java
index 94ad7b8fad0..b0cd6a20499 100644
--- a/core/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestionBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestionBuilder.java
@@ -173,7 +173,7 @@ public class PhraseSuggestionBuilder extends SuggestionBuilder> entry : this.generators.entrySet()) {
diff --git a/core/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java b/core/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java
index 52676444d3c..f1e6c1ba67e 100644
--- a/core/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java
+++ b/core/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java
@@ -32,9 +32,14 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.TreeMap;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 import static org.hamcrest.Matchers.closeTo;
 import static org.hamcrest.Matchers.endsWith;
@@ -620,4 +625,50 @@ public class BytesStreamsTests extends ESTestCase {
             out.writeBoolean(value);
         }
     }
+
+    public void testWriteMapWithConsistentOrder() throws IOException {
+        Map map =
+            randomMap(new TreeMap<>(), randomIntBetween(2, 20),
+                () -> randomAsciiOfLength(5),
+                () -> randomAsciiOfLength(5));
+
+        Map reverseMap = new TreeMap<>(Collections.reverseOrder());
+        reverseMap.putAll(map);
+
+        List mapKeys = map.entrySet().stream().map(Map.Entry::getKey).collect(Collectors.toList());
+        List reverseMapKeys = reverseMap.entrySet().stream().map(Map.Entry::getKey).collect(Collectors.toList());
+
+        assertNotEquals(mapKeys, reverseMapKeys);
+
+        BytesStreamOutput output = new BytesStreamOutput();
+        BytesStreamOutput reverseMapOutput = new BytesStreamOutput();
+        output.writeMapWithConsistentOrder(map);
+        reverseMapOutput.writeMapWithConsistentOrder(reverseMap);
+
+        assertEquals(output.bytes(), reverseMapOutput.bytes());
+    }
+
+    public void testReadMapByUsingWriteMapWithConsistentOrder() throws IOException {
+        Map streamOutMap =
+            randomMap(new HashMap<>(), randomIntBetween(2, 20),
+                () -> randomAsciiOfLength(5),
+                () -> randomAsciiOfLength(5));
+        BytesStreamOutput streamOut = new BytesStreamOutput();
+        streamOut.writeMapWithConsistentOrder(streamOutMap);
+        StreamInput in = StreamInput.wrap(BytesReference.toBytes(streamOut.bytes()));
+        Map streamInMap = in.readMap();
+        assertEquals(streamOutMap, streamInMap);
+    }
+
+    public void testWriteMapWithConsistentOrderWithLinkedHashMapShouldThrowAssertError() throws IOException {
+        BytesStreamOutput output = new BytesStreamOutput();
+        Map map = new LinkedHashMap<>();
+        Throwable e = expectThrows(AssertionError.class, () -> output.writeMapWithConsistentOrder(map));
+        assertEquals(AssertionError.class, e.getClass());
+    }
+
+    private static  Map randomMap(Map map, int size, Supplier keyGenerator, Supplier valueGenerator) {
+        IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get()));
+        return map;
+    }
 }
diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java
index c2cd5126a85..0742fca357d 100644
--- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java
+++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java
@@ -666,4 +666,25 @@ public class SearchSourceBuilderTests extends ESTestCase {
         String query = "{ \"query\": {} }";
         assertParseSearchSource(builder, new BytesArray(query), ParseFieldMatcher.EMPTY);
     }
+
+    public void testSearchRequestBuilderSerializationWithIndexBoost() throws Exception {
+        SearchSourceBuilder searchSourceBuilder = createSearchSourceBuilder();
+        createIndexBoost(searchSourceBuilder);
+        try (BytesStreamOutput output = new BytesStreamOutput()) {
+            searchSourceBuilder.writeTo(output);
+            try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), namedWriteableRegistry)) {
+                SearchSourceBuilder deserializedSearchSourceBuilder = new SearchSourceBuilder(in);
+                BytesStreamOutput deserializedOutput = new BytesStreamOutput();
+                deserializedSearchSourceBuilder.writeTo(deserializedOutput);
+                assertEquals(output.bytes(), deserializedOutput.bytes());
+            }
+        }
+    }
+
+    private void createIndexBoost(SearchSourceBuilder searchSourceBuilder) {
+        int indexBoostSize = randomIntBetween(1, 10);
+        for (int i = 0; i < indexBoostSize; i++) {
+            searchSourceBuilder.indexBoost(randomAsciiOfLengthBetween(5, 20), randomFloat() * 10);
+        }
+    }
 }