THRIFT-4555 Optionally disable copies of binary fields in constructors, getters and setters.
Client: java
This closes #1540.
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 @@
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 @@
} 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 @@
bool undated_generated_annotations_;
bool suppress_generated_annotations_;
bool handle_runtime_exceptions_;
+ bool unsafe_binaries_;
};
@@ -934,8 +938,12 @@
<< "(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 @@
<< 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 @@
}
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 @@
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 @@
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 @@
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 @@
<< 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 @@
"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.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 @@
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 @@
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 @@
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;
+}