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

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


The following commit(s) were added to refs/heads/master by this push:
     new 50bfc56  THRIFT-4555 Optionally disable copies of binary fields in 
constructors, getters and setters.
50bfc56 is described below

commit 50bfc56d5d337a08a2dd3a6f60b0ed656719c6ed
Author: Ben Podgursky <bpodgur...@gmail.com>
AuthorDate: Mon Apr 16 23:21:46 2018 -0700

    THRIFT-4555 Optionally disable copies of binary fields in constructors, 
getters and setters.
    
    Client: java
    
    This closes #1540.
---
 .../cpp/src/thrift/generate/t_java_generator.cc    |  68 ++++++++--
 lib/java/gradle/generateTestThrift.gradle          |  12 +-
 .../test/org/apache/thrift/TestUnsafeBinaries.java | 146 +++++++++++++++++++++
 test/JavaTypes.thrift                              |   5 +
 test/Makefile.am                                   |   1 +
 test/UnsafeTypes.thrift                            |  24 ++++
 6 files changed, 241 insertions(+), 15 deletions(-)

diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc 
b/compiler/cpp/src/thrift/generate/t_java_generator.cc
index 754601f..34ba65f 100644
--- a/compiler/cpp/src/thrift/generate/t_java_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_java_generator.cc
@@ -72,6 +72,7 @@ public:
     undated_generated_annotations_  = false;
     suppress_generated_annotations_ = false;
     handle_runtime_exceptions_ = false;
+    unsafe_binaries_ = false;
     for( iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
       if( iter->first.compare("beans") == 0) {
         bean_style_ = true;
@@ -103,6 +104,8 @@ public:
         } else {
           throw "unknown option java:" + iter->first + "=" + iter->second;
         }
+      } else if( iter->first.compare("unsafe_binaries") == 0) {
+        unsafe_binaries_ = true;
       } else {
         throw "unknown option java:" + iter->first;
       }
@@ -411,6 +414,7 @@ private:
   bool undated_generated_annotations_;
   bool suppress_generated_annotations_;
   bool handle_runtime_exceptions_;
+  bool unsafe_binaries_;
 
 };
 
@@ -934,8 +938,12 @@ void 
t_java_generator::generate_union_constructor(ofstream& out, t_struct* tstru
                   << "(byte[] value) {" << endl;
       indent(out) << "  " << type_name(tstruct) << " x = new " << 
type_name(tstruct) << "();"
                   << endl;
-      indent(out) << "  x.set" << get_cap_name((*m_iter)->get_name())
-                  << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+      indent(out) << "  x.set" << get_cap_name((*m_iter)->get_name());
+      if(unsafe_binaries_) {
+        indent(out) << "(java.nio.ByteBuffer.wrap(value));" << endl;
+      }else{
+        indent(out) << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+      }
       indent(out) << "  return x;" << endl;
       indent(out) << "}" << endl << endl;
     }
@@ -977,9 +985,17 @@ void 
t_java_generator::generate_union_getters_and_setters(ofstream& out, t_struc
                   << get_cap_name(field->get_name()) << "() {" << endl;
       indent(out) << "  if (getSetField() == _Fields." << 
constant_name(field->get_name()) << ") {"
                   << endl;
-      indent(out)
+
+      if(unsafe_binaries_){
+        indent(out)
+          << "    return (java.nio.ByteBuffer)getFieldValue();"
+          << endl;
+      }else{
+        indent(out)
           << "    return 
org.apache.thrift.TBaseHelper.copyBinary((java.nio.ByteBuffer)getFieldValue());"
           << endl;
+      }
+
       indent(out) << "  } else {" << endl;
       indent(out) << "    throw new java.lang.RuntimeException(\"Cannot get 
field '" << field->get_name()
                   << "' because union is currently set to \" + 
getFieldDesc(getSetField()).name);"
@@ -1013,8 +1029,14 @@ void 
t_java_generator::generate_union_getters_and_setters(ofstream& out, t_struc
       }
       indent(out) << "public void set" << get_cap_name(field->get_name()) << 
"(byte[] value) {"
                   << endl;
-      indent(out) << "  set" << get_cap_name(field->get_name())
-                  << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+      indent(out) << "  set" << get_cap_name(field->get_name());
+
+      if(unsafe_binaries_){
+        indent(out) << "(java.nio.ByteBuffer.wrap(value));" << endl;
+      }else{
+        indent(out) << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+      }
+
       indent(out) << "}" << endl;
 
       out << endl;
@@ -1544,9 +1566,15 @@ void 
t_java_generator::generate_java_struct_definition(ofstream& out,
       if ((*m_iter)->get_req() != t_field::T_OPTIONAL) {
         t_type* type = get_true_type((*m_iter)->get_type());
         if (type->is_binary()) {
-          indent(out) << "this." << (*m_iter)->get_name()
-                      << " = org.apache.thrift.TBaseHelper.copyBinary(" << 
(*m_iter)->get_name()
-                      << ");" << endl;
+          if(unsafe_binaries_){
+            indent(out) << "this." << (*m_iter)->get_name()
+                        << " = " << (*m_iter)->get_name()
+                        << ";" << endl;
+          }else{
+            indent(out) << "this." << (*m_iter)->get_name()
+                        << " = org.apache.thrift.TBaseHelper.copyBinary(" << 
(*m_iter)->get_name()
+                        << ");" << endl;
+          }
         } else {
           indent(out) << "this." << (*m_iter)->get_name() << " = " << 
(*m_iter)->get_name() << ";"
                       << endl;
@@ -2406,8 +2434,13 @@ void 
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
 
       indent(out) << "public java.nio.ByteBuffer buffer" << 
get_cap_name("for") << cap_name << "() {"
                   << endl;
-      indent(out) << "  return org.apache.thrift.TBaseHelper.copyBinary(" << 
field_name << ");"
-                  << endl;
+      if(unsafe_binaries_){
+        indent(out) << "  return " << field_name << ";"
+                    << endl;
+      }else {
+        indent(out) << "  return org.apache.thrift.TBaseHelper.copyBinary(" << 
field_name << ");"
+                    << endl;
+      }
       indent(out) << "}" << endl << endl;
     } else {
       if (optional) {
@@ -2468,8 +2501,14 @@ void 
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
         out << type_name(tstruct);
       }
       out << " set" << cap_name << "(byte[] " << field_name << ") {" << endl;
-      indent(out) << "  this." << field_name << " = " << field_name << " == 
null ? (java.nio.ByteBuffer)null"
-                  << " : java.nio.ByteBuffer.wrap(" << field_name << 
".clone());" << endl;
+      indent(out) << "  this." << field_name << " = " << field_name << " == 
null ? (java.nio.ByteBuffer)null";
+
+      if(unsafe_binaries_){
+        indent(out) << " : java.nio.ByteBuffer.wrap(" << field_name << ");" << 
endl;
+      }else{
+        indent(out) << " : java.nio.ByteBuffer.wrap(" << field_name << 
".clone());" << endl;
+      }
+                 
       if (!bean_style_) {
         indent(out) << "  return this;" << endl;
       }
@@ -2488,7 +2527,7 @@ void 
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
         << type_name(type) << " " << field_name << ") {" << endl;
     indent_up();
     indent(out) << "this." << field_name << " = ";
-    if (type->is_binary()) {
+    if (type->is_binary() && !unsafe_binaries_) {
       out << "org.apache.thrift.TBaseHelper.copyBinary(" << field_name << ")";
     } else {
       out << field_name;
@@ -5400,4 +5439,5 @@ THRIFT_REGISTER_GENERATOR(
     "set/map.\n"
     "    generated_annotations=[undated|suppress]:\n"
     "                     undated: suppress the date at @Generated 
annotations\n"
-    "                     suppress: suppress @Generated annotations 
entirely\n")
+    "                     suppress: suppress @Generated annotations entirely\n"
+    "    unsafe_binaries: Do not copy ByteBuffers in constructors, getters, 
and setters.\n")
diff --git a/lib/java/gradle/generateTestThrift.gradle 
b/lib/java/gradle/generateTestThrift.gradle
index c347917..2b53739 100644
--- a/lib/java/gradle/generateTestThrift.gradle
+++ b/lib/java/gradle/generateTestThrift.gradle
@@ -24,10 +24,11 @@ ext.genSrc = file("$buildDir/gen-java")
 ext.genBeanSrc = file("$buildDir/gen-javabean")
 ext.genReuseSrc = file("$buildDir/gen-javareuse")
 ext.genFullCamelSrc = file("$buildDir/gen-fullcamel")
+ext.genUnsafeSrc = file("$buildDir/gen-unsafe")
 
 // Add the generated code directories to the test source set
 sourceSets {
-    test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc
+    test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc, 
genUnsafeSrc
 }
 
 // ----------------------------------------------------------------------------
@@ -107,3 +108,12 @@ task generateFullCamelJava(group: 'Build') {
 
     thriftCompile(it, 'ReuseObjects.thrift', 'java:reuse-objects', genReuseSrc)
 }
+
+task generateUnsafeBinariesJava(group: 'Build') {
+    description = 'Generate the thrift gen-unsafebinaries source'
+    generate.dependsOn it
+
+    ext.outputBuffer = new ByteArrayOutputStream()
+
+    thriftCompile(it, 'UnsafeTypes.thrift', 'java:unsafe_binaries', 
genUnsafeSrc)
+}
diff --git a/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java 
b/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java
new file mode 100644
index 0000000..d1fc213
--- /dev/null
+++ b/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.thrift;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+
+import thrift.test.SafeBytes;
+import thrift.test.UnsafeBytes;
+
+//  test generating types with un-copied byte[]/ByteBuffer input/output
+//
+public class TestUnsafeBinaries extends TestStruct {
+
+  private static byte[] input() {
+    return new byte[]{1, 1};
+  }
+
+  //
+  //  verify that the unsafe_binaries option modifies behavior
+  //
+
+  //  constructor doesn't copy
+  public void testUnsafeConstructor() throws Exception {
+
+    byte[] input = input();
+    UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input));
+
+    input[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{2, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  //  getter doesn't copy
+  //  note: this behavior is the same with/without the flag, but if this 
default ever changes, the current behavior
+  //        should be retained when using this flag
+  public void testUnsafeGetter(){
+    UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input()));
+
+    byte[] val = struct.getBytes();
+    val[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{2, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  //  setter doesn't copy
+  public void testUnsafeSetter(){
+    UnsafeBytes struct = new UnsafeBytes();
+
+    byte[] val = input();
+    struct.setBytes(val);
+
+    val[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{2, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  //  buffer doens't copy
+  public void testUnsafeBufferFor(){
+    UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input()));
+
+    ByteBuffer val = struct.bufferForBytes();
+    val.array()[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{2, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  //
+  //  verify that the default generator does not change behavior
+  //
+
+  public void testSafeConstructor() {
+
+    byte[] input = input();
+    SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input));
+
+    input[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{1, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  public void testSafeSetter() {
+
+    byte[] input = input();
+    SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input));
+
+    input[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{1, 1},
+        struct.getBytes())
+    );
+
+  }
+
+  public void testSafeBufferFor(){
+    SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input()));
+
+    ByteBuffer val = struct.bufferForBytes();
+    val.array()[0] = 2;
+
+    assertTrue(Arrays.equals(
+        new byte[]{1, 1},
+        struct.getBytes())
+    );
+
+  }
+
+}
diff --git a/test/JavaTypes.thrift b/test/JavaTypes.thrift
index fcb0ab2..8c733ad 100644
--- a/test/JavaTypes.thrift
+++ b/test/JavaTypes.thrift
@@ -96,3 +96,8 @@ service AsyncNonblockingService {
     7: Map somemap,
   ) throws (1:Exception ex);
 }
+
+struct SafeBytes {
+  1: binary bytes;
+}
+
diff --git a/test/Makefile.am b/test/Makefile.am
index 7267066..68f1986 100755
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -155,6 +155,7 @@ EXTRA_DIST = \
        StressTest.thrift \
        ThriftTest.thrift \
        TypedefTest.thrift \
+       UnsafeTypes.thrift \
        known_failures_Linux.json \
        test.py \
        tests.json \
diff --git a/test/UnsafeTypes.thrift b/test/UnsafeTypes.thrift
new file mode 100644
index 0000000..b38c905
--- /dev/null
+++ b/test/UnsafeTypes.thrift
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+namespace java thrift.test
+
+struct UnsafeBytes {
+  1: binary bytes;
+}

-- 
To stop receiving notification emails like this one, please contact
dcela...@apache.org.

Reply via email to