THRIFT-882. java: deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)

This patch ensures that binary fields are copied correctly.



git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@998275 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 57 insertions, 9 deletions

diff --git a/compiler/cpp/src/generate/t_java_generator.cc b/compiler/cpp/src/generate/t_java_generator.cc
index a7504ff53..d93eb6760 100644
--- a/compiler/cpp/src/generate/t_java_generator.cc
+++ b/compiler/cpp/src/generate/t_java_generator.cc
@@ -1393,7 +1393,7 @@ void t_java_generator::generate_java_struct_compare_to(ofstream& out, t_struct*
     indent(out) << "  return lastComparison;" << endl;
     indent(out) << "}" << endl;
 
-    indent(out) << "if (" << generate_isset_check(field) << ") {";
+    indent(out) << "if (" << generate_isset_check(field) << ") {" << endl;
     indent(out) << "  lastComparison = TBaseHelper.compareTo(this." << field->get_name() << ", typedOther." << field->get_name() << ");" << endl;
     indent(out) << "  if (lastComparison != 0) {" << endl;
     indent(out) << "    return lastComparison;" << endl;
@@ -3732,14 +3732,12 @@ void t_java_generator::generate_deep_copy_container(ofstream &out, std::string s
 
 void t_java_generator::generate_deep_copy_non_container(ofstream& out, std::string source_name, std::string dest_name, t_type* type) {
   if (type->is_base_type() || type->is_enum() || type->is_typedef()) {
-    // binary fields need to be copied with System.arraycopy
-    if (((t_base_type*)type)->is_binary()){
-      out << "ByteBuffer.wrap(new byte[" << source_name << ".limit() - " << source_name << ".arrayOffset()]);" << endl;
-      indent(out) << "System.arraycopy(" << source_name << ".array(), " << source_name << ".arrayOffset(), " << dest_name << ".array(), 0, " << source_name << ".limit() - " << source_name << ".arrayOffset())";
-    }
-    // everything else can be copied directly
-    else
+    if (((t_base_type*)type)->is_binary()) {
+      out << "TBaseHelper.copyBinary(" << source_name << ");" << endl;
+    } else {
+      // everything else can be copied directly
       out << source_name;
+    }
   } else {
     out << "new " << type_name(type, true, true) << "(" << source_name << ")";
   }
diff --git a/lib/java/src/org/apache/thrift/TBaseHelper.java b/lib/java/src/org/apache/thrift/TBaseHelper.java
index 46075d3ee..211ea6206 100644
--- a/lib/java/src/org/apache/thrift/TBaseHelper.java
+++ b/lib/java/src/org/apache/thrift/TBaseHelper.java
@@ -28,7 +28,9 @@ import java.util.SortedSet;
 import java.util.TreeMap;
 import java.util.TreeSet;
 
-public class TBaseHelper {
+public final class TBaseHelper {
+
+  private TBaseHelper(){}
 
   private static final Comparator comparator = new NestedStructureComparator();
 
@@ -273,4 +275,21 @@ public class TBaseHelper {
     }
     return ByteBuffer.wrap(byteBufferToByteArray(in));
   }
+
+  public static ByteBuffer copyBinary(final ByteBuffer orig) {
+    ByteBuffer copy = ByteBuffer.wrap(new byte[orig.remaining()]);
+    if (orig.hasArray()) {
+      System.arraycopy(orig.array(), orig.arrayOffset() + orig.position(), copy.array(), 0, orig.remaining());
+    } else {
+      orig.slice().get(copy.array());
+    }
+
+    return copy;
+  }
+
+  public static byte[] copyBinary(final byte[] orig) {
+    byte[] copy = new byte[orig.length];
+    System.arraycopy(orig, 0, copy, 0, orig.length);
+    return copy;
+  }
 }
diff --git a/lib/java/test/org/apache/thrift/TestTBaseHelper.java b/lib/java/test/org/apache/thrift/TestTBaseHelper.java
index 2c8caac00..a66e78923 100644
--- a/lib/java/test/org/apache/thrift/TestTBaseHelper.java
+++ b/lib/java/test/org/apache/thrift/TestTBaseHelper.java
@@ -150,4 +150,35 @@ public class TestTBaseHelper extends TestCase {
     assertEquals(3, b3.length);
     assertEquals(ByteBuffer.wrap(b1, 1, 3), ByteBuffer.wrap(b3));
   }
+
+  public void testCopyBinaryWithByteBuffer() throws Exception {
+    byte[] bytes = new byte[]{0, 1, 2, 3, 4, 5};
+    ByteBuffer b = ByteBuffer.wrap(bytes);
+    ByteBuffer bCopy = TBaseHelper.copyBinary(b);
+    assertEquals(b, bCopy);
+    assertEquals(0, b.position());
+
+    b = ByteBuffer.allocateDirect(6);
+    b.put(bytes);
+    b.position(0);
+    bCopy = TBaseHelper.copyBinary(b);
+    assertEquals(6, b.remaining());
+    assertEquals(0, b.position());
+    assertEquals(b, bCopy);
+
+    b.mark();
+    b.get();
+    bCopy = TBaseHelper.copyBinary(b);
+    assertEquals(ByteBuffer.wrap(bytes, 1, 5), bCopy);
+    assertEquals(1, b.position());
+    b.reset();
+    assertEquals(0, b.position());
+  }
+
+  public void testCopyBinaryWithByteArray() throws Exception {
+    byte[] bytes = new byte[]{0, 1, 2, 3, 4, 5};
+    byte[] copy = TBaseHelper.copyBinary(bytes);
+    assertEquals(ByteBuffer.wrap(bytes), ByteBuffer.wrap(copy));
+    assertNotSame(bytes, copy);
+  }
 }