You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by dc...@apache.org on 2018/04/17 06:21:52 UTC
[thrift] branch master updated: THRIFT-4555 Optionally disable
copies of binary fields in constructors, getters and setters.
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 <bp...@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
dcelasun@apache.org.