[helix] branch master updated: Deep copy for mapFields and listFields in ZNRecord's copy constructor. (#552)

[ji...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jiajunwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git


The following commit(s) were added to refs/heads/master by this push:
     new 2a335cf  Deep copy for mapFields and listFields in ZNRecord's copy constructor. (#552)
2a335cf is described below

commit 2a335cf73ac65b53fd2b06c6b1ee8c70553d30b1
Author: Huizhi L <ih...@gmail.com>
AuthorDate: Thu Oct 31 11:03:27 2019 -0700

    Deep copy for mapFields and listFields in ZNRecord's copy constructor. (#552)
    
    Deep copy for mapFields and listFields in ZNRecord's copy constructor.
    Change list:
    1. deep copy for mapFields and listFields in ZNRecord's copy constructor.
    2. add unit test for the deep copy constructor.
---
 .../src/main/java/org/apache/helix/ZNRecord.java   |  6 ++-
 .../test/java/org/apache/helix/TestZNRecord.java   | 43 ++++++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/helix-core/src/main/java/org/apache/helix/ZNRecord.java b/helix-core/src/main/java/org/apache/helix/ZNRecord.java
index bd9acbb..de8bb5c 100644
--- a/helix-core/src/main/java/org/apache/helix/ZNRecord.java
+++ b/helix-core/src/main/java/org/apache/helix/ZNRecord.java
@@ -20,6 +20,7 @@ package org.apache.helix;
  */
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
@@ -101,8 +102,9 @@ public class ZNRecord {
   public ZNRecord(ZNRecord record, String id) {
     this(id);
     simpleFields.putAll(record.getSimpleFields());
-    mapFields.putAll(record.getMapFields());
-    listFields.putAll(record.getListFields());
+    // Deep copy for both mapFields and listFields.
+    record.getMapFields().forEach((k, v) -> mapFields.put(k, new HashMap<>(v)));
+    record.getListFields().forEach((k, v) -> listFields.put(k, new ArrayList<>(v)));
     if (record.rawPayload != null) {
       rawPayload = new byte[record.rawPayload.length];
       System.arraycopy(record.rawPayload, 0, rawPayload, 0, record.rawPayload.length);
diff --git a/helix-core/src/test/java/org/apache/helix/TestZNRecord.java b/helix-core/src/test/java/org/apache/helix/TestZNRecord.java
index 7dcb9cd..93e324e 100644
--- a/helix-core/src/test/java/org/apache/helix/TestZNRecord.java
+++ b/helix-core/src/test/java/org/apache/helix/TestZNRecord.java
@@ -23,6 +23,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+
 import org.testng.Assert;
 import org.testng.AssertJUnit;
 import org.testng.annotations.Test;
@@ -123,4 +124,46 @@ public class TestZNRecord {
     expectRecord.setMapField("mapKey2", expectMap2);
     Assert.assertEquals(record, expectRecord, "Should be equal.");
   }
+
+  @Test
+  public void testDeepCopyConstructor() {
+    ZNRecord record = new ZNRecord("record");
+
+    // set simple field
+    record.setSimpleField("simpleKey1", "simpleValue1");
+
+    // set list field
+    List<String> list1 = new ArrayList<>();
+    list1.add("list1Value1");
+    list1.add("list1Value2");
+    record.setListField("listKey1", list1);
+
+    // set map field
+    Map<String, String> map1 = new HashMap<>();
+    map1.put("map1Key1", "map1Value1");
+    record.setMapField("mapKey1", map1);
+
+    Map<String, String> map2 = new HashMap<>();
+    map2.put("map2Key1", "map2Value1");
+    record.setMapField("mapKey2", map2);
+
+    ZNRecord newRecord = new ZNRecord(record);
+
+    // Verify initial copy results.
+    Assert.assertEquals(record.getMapFields(), newRecord.getMapFields());
+    Assert.assertEquals(record.getListFields(), newRecord.getListFields());
+    Assert.assertEquals(record, newRecord);
+
+    // Change values in original record.
+    map1.put("map1Key1", "valueChanged");
+    record.setMapField("mapKey1", map1);
+
+    list1.set(0, "valueChanged");
+    record.setListField("listKey1", list1);
+
+    // Value changes in original record should not change new record.
+    Assert.assertFalse(record.getMapFields().equals(newRecord.getMapFields()));
+    Assert.assertFalse(record.getListFields().equals(newRecord.getListFields()));
+    Assert.assertFalse(record.equals(newRecord));
+  }
 }