fix: HollowMapWriteRecord serialization should sort on key and value (#…

…688)

* fix: HollowMapWriteRecord serialization should sort on key and value

* nit: drop unused logger

hollow/src/main/java/com/netflix/hollow/core/write/HollowMapWriteRecord.java

@@ -31,7 +31,11 @@ public class HollowMapWriteRecord implements HollowHashableWriteRecord {
 
     private static final Comparator<HollowMapEntry> MAP_ENTRY_COMPARATOR = new Comparator<HollowMapEntry>() {
         public int compare(HollowMapEntry o1, HollowMapEntry o2) {
-            return o1.getKeyOrdinal() - o2.getKeyOrdinal();
+            int res = o1.getKeyOrdinal() - o2.getKeyOrdinal();
+            if (res == 0) {
+                res =  o1.getValueOrdinal() - o2.getValueOrdinal();
+            }
+            return res;
         }
     };
 

hollow/src/test/java/com/netflix/hollow/core/read/map/HollowMapDeltaTest.java

@@ -16,13 +16,27 @@
  */
 package com.netflix.hollow.core.read.map;
 
+import static org.junit.Assert.assertEquals;
+
 import com.netflix.hollow.core.AbstractStateEngineTest;
+import com.netflix.hollow.core.memory.ByteDataArray;
+import com.netflix.hollow.core.read.engine.HollowReadStateEngine;
+import com.netflix.hollow.core.read.engine.HollowTypeReadState;
 import com.netflix.hollow.core.read.engine.map.HollowMapTypeReadState;
+import com.netflix.hollow.core.read.engine.object.HollowObjectTypeReadState;
 import com.netflix.hollow.core.read.iterator.HollowMapEntryOrdinalIterator;
 import com.netflix.hollow.core.schema.HollowMapSchema;
+import com.netflix.hollow.core.util.StateEngineRoundTripper;
+import com.netflix.hollow.core.write.HollowBlobWriter;
 import com.netflix.hollow.core.write.HollowMapTypeWriteState;
 import com.netflix.hollow.core.write.HollowMapWriteRecord;
+import com.netflix.hollow.core.write.HollowWriteStateEngine;
+import com.netflix.hollow.core.write.objectmapper.HollowObjectMapper;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -51,7 +65,7 @@ public void test() throws IOException {
         assertMap(typeState, 3, 100, 200, 300, 400, 500, 600, 700, 800);
         assertMap(typeState, 4, 1, 2, 3, 4);
 
-        Assert.assertEquals(4, typeState.maxOrdinal());
+        assertEquals(4, typeState.maxOrdinal());
 
         roundTripDelta();
 
@@ -61,14 +75,14 @@ public void test() throws IOException {
         assertMap(typeState, 3, 100, 200, 300, 400, 500, 600, 700, 800); /// "ghost"
         assertMap(typeState, 4, 1, 2, 3, 4); /// "ghost"
 
-        Assert.assertEquals(4, typeState.maxOrdinal());
+        assertEquals(4, typeState.maxOrdinal());
 
         addRecord(634, 54732);
         addRecord(1, 2, 3, 4);
 
         roundTripDelta();
 
-        Assert.assertEquals(1, typeState.maxOrdinal());
+        assertEquals(1, typeState.maxOrdinal());
         assertMap(typeState, 0, 634, 54732); /// now, since all maps were removed, we can recycle the ordinal "0", even though it was a "ghost" in the last cycle.
         assertMap(typeState, 1, 1, 2, 3, 4);  /// even though 1, 2, 3 had an equivalent map in the previous cycle at ordinal "4", it is now assigned to recycled ordinal "1".
     }
@@ -101,8 +115,8 @@ public void testSingleEmptyMap() throws IOException {
 
         HollowMapTypeReadState typeState = (HollowMapTypeReadState) readStateEngine.getTypeState("TestMap");
 
-        Assert.assertEquals(0, typeState.maxOrdinal());
-        Assert.assertEquals(0, typeState.size(0));
+        assertEquals(0, typeState.maxOrdinal());
+        assertEquals(0, typeState.size(0));
     }
 
     @Test
@@ -113,14 +127,14 @@ public void testSingleMapWith0KeyValueOrdinals() throws IOException {
 
         HollowMapTypeReadState typeState = (HollowMapTypeReadState) readStateEngine.getTypeState("TestMap");
 
-        Assert.assertEquals(0, typeState.maxOrdinal());
-        Assert.assertEquals(1, typeState.size(0));
+        assertEquals(0, typeState.maxOrdinal());
+        assertEquals(1, typeState.size(0));
 
         HollowMapEntryOrdinalIterator ordinalIterator = typeState.ordinalIterator(0);
 
         Assert.assertTrue(ordinalIterator.next());
-        Assert.assertEquals(0, ordinalIterator.getKey());
-        Assert.assertEquals(0, ordinalIterator.getValue());
+        assertEquals(0, ordinalIterator.getKey());
+        assertEquals(0, ordinalIterator.getValue());
         Assert.assertFalse(ordinalIterator.next());
     }
 
@@ -140,6 +154,79 @@ public void testStaleReferenceException() throws IOException {
         } catch(NullPointerException expected) { }
     }
 
+    @Test
+    public void testMapSerializationSortOnKeyAndValueOrdinals() {
+        // for cases where duplicate keys exist in the map (for e.g. equals/hashcode not overridden in data model)
+        // sorting on both key and value ordinals ensures consistent serialization
+        HollowMapWriteRecord mapWriteRecord1 = new HollowMapWriteRecord();
+        mapWriteRecord1.addEntry(0, 0);
+        mapWriteRecord1.addEntry(0, 1);
+
+        ByteDataArray byteDataArray1 = new ByteDataArray();
+        mapWriteRecord1.writeDataTo(byteDataArray1);
+
+        HollowMapWriteRecord mapWriteRecord2 = new HollowMapWriteRecord();
+        mapWriteRecord2.addEntry(0, 1);
+        mapWriteRecord2.addEntry(0, 0);
+
+        ByteDataArray byteDataArray2 = new ByteDataArray();
+        mapWriteRecord2.writeDataTo(byteDataArray2);
+
+        assertEquals(byteDataArray1.length(), byteDataArray2.length());
+        for (int i=0; i<byteDataArray1.length(); i++) {
+            assertEquals(byteDataArray1.get(i), byteDataArray2.get(i));
+        }
+    }
+
+    @Test
+    public void testMapChecksumSortOnKeyAndValueOrdinals() throws Exception {
+        // for cases where duplicate keys exist in the map (for e.g. equals/hashcode not overridden in data model)
+        // sorting on both key and value ordinals ensures consistent checksum (and avoids integrity check failures)
+        final int KEY = 1;
+        final String VALUE1 = "value1";
+        final String VALUE2 = "value2";
+
+        Pojo pojo1 = new Pojo(KEY);
+        Pojo pojo2 = new Pojo(KEY); // same data, but no equals or hashCode defined
+
+        Map<Pojo, String> orderedMap1 = new LinkedHashMap<>();
+        orderedMap1.put(pojo1, VALUE1);
+        orderedMap1.put(pojo2, VALUE2);
+        PojoMap pojoMap1 = new PojoMap(orderedMap1); // {1->value1, 1->value2}
+
+        Map<Pojo, String> orderedMap2 = new LinkedHashMap<>();
+        orderedMap2.put(pojo2, VALUE2); // flip the insertion order
+        orderedMap2.put(pojo1, VALUE1);
+        PojoMap pojoMap2 = new PojoMap(orderedMap2); // {1->value2, 1->value1}
+
+        HollowWriteStateEngine writeStateEngine1 = new HollowWriteStateEngine();
+        HollowObjectMapper objectMapper1 = new HollowObjectMapper(writeStateEngine1);
+        HollowWriteStateEngine writeStateEngine2 = new HollowWriteStateEngine();
+        HollowObjectMapper objectMapper2 = new HollowObjectMapper(writeStateEngine2);
+        objectMapper1.initializeTypeState(PojoMap.class);
+        objectMapper2.initializeTypeState(PojoMap.class);
+
+        objectMapper1.add(VALUE1); // pin VALUE1 and VALUE2 to ordinals 0 and 1 consistently across both states
+        objectMapper1.add(VALUE2); // so that we can test for ordering in map type
+        objectMapper2.add(VALUE1);
+        objectMapper2.add(VALUE2);
+
+        // HollowMapTypeMapper iterates on pojo map entries to assign ordinals and add HollowMapWriteRecord entries
+        objectMapper1.add(pojoMap1); // ordinals {0,0}, {0,1}
+        objectMapper2.add(pojoMap2); // ordinals {0,1}, {0,0}
+
+        HollowReadStateEngine readStateEngine1 = new HollowReadStateEngine();
+        StateEngineRoundTripper.roundTripSnapshot(writeStateEngine1, readStateEngine1);
+
+        HollowReadStateEngine readStateEngine2 = new HollowReadStateEngine();
+        StateEngineRoundTripper.roundTripSnapshot(writeStateEngine2, readStateEngine2);
+
+        // serialization should sort those identically, i.e. on both key and value ordinal values
+        HollowTypeReadState mapTypeState1 = readStateEngine1.getTypeState("MapOfPojoToString");
+        HollowTypeReadState mapTypeState2 = readStateEngine2.getTypeState("MapOfPojoToString");
+        assertEquals(mapTypeState1.getChecksum(mapTypeState1.getSchema()), mapTypeState2.getChecksum(mapTypeState2.getSchema()));
+    }
+
     private void addRecord(int... ordinals) {
         HollowMapWriteRecord rec = new HollowMapWriteRecord();
 
@@ -151,10 +238,10 @@ private void addRecord(int... ordinals) {
     }
 
     private void assertMap(HollowMapTypeReadState readState, int ordinal, int... elements) {
-        Assert.assertEquals(elements.length / 2, readState.size(ordinal));
+        assertEquals(elements.length / 2, readState.size(ordinal));
 
         for(int i=0;i<elements.length;i+=2) {
-            Assert.assertEquals(elements[i+1], readState.get(ordinal, elements[i]));
+            assertEquals(elements[i+1], readState.get(ordinal, elements[i]));
         }
     }
 
@@ -165,3 +252,21 @@ protected void initializeTypeStates() {
     }
 
 }
+
+class Pojo {
+    public int val;
+
+    public Pojo(int val) {
+        this.val = val;
+    }
+
+    // no hashCode or equals defined
+}
+
+class PojoMap {
+    public Map<Pojo, String> map;
+
+    public PojoMap(Map<Pojo, String> map) {
+        this.map = map;
+    }
+}