From 4606783164233330ae9171ea998cf5c5f9ce9475 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Sun, 21 Feb 2016 15:07:51 +0100 Subject: [PATCH 1/4] THRIFT-3650 incorrect union serialization Client: Compiler (general) Patch: Jens Geyer --- compiler/cpp/src/parse/t_struct.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/cpp/src/parse/t_struct.h b/compiler/cpp/src/parse/t_struct.h index 93cb0896609..d19447c3650 100644 --- a/compiler/cpp/src/parse/t_struct.h +++ b/compiler/cpp/src/parse/t_struct.h @@ -66,12 +66,16 @@ class t_struct : public t_type { void validate_union_member(t_field* field) { if (is_union_ && (!name_.empty())) { - // unions can't have required fields - if (field->get_req() == t_field::T_REQUIRED) { - pwarning(1, - "Required field %s of union %s set to optional.\n", - field->get_name().c_str(), - name_.c_str()); + // 1) unions can't have required fields + // 2) union members are implicitly optional, otherwise bugs like THRIFT-3650 wait to happen + if (field->get_req() != t_field::T_OPTIONAL) { + // no warning on default requiredness, but do warn on anything else that is explicitly asked for + if(field->get_req() != t_field::T_OPT_IN_REQ_OUT) { + pwarning(1, + "Union %s field %s: union members must be optional, ignoring specified requiredness.\n", + name_.c_str(), + field->get_name().c_str()); + } field->set_req(t_field::T_OPTIONAL); } From beab25cb84ae77692417345dbddadfa65cfca385 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Sun, 21 Feb 2016 15:14:41 +0100 Subject: [PATCH 2/4] THRIFT-3654 incorrect serialization of optionals Client: Haxe Patch: Jens Geyer --- compiler/cpp/src/generate/t_haxe_generator.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/compiler/cpp/src/generate/t_haxe_generator.cc b/compiler/cpp/src/generate/t_haxe_generator.cc index f34a4066f21..d15958e59ee 100644 --- a/compiler/cpp/src/generate/t_haxe_generator.cc +++ b/compiler/cpp/src/generate/t_haxe_generator.cc @@ -978,6 +978,11 @@ void t_haxe_generator::generate_haxe_struct_writer(ofstream& out, t_struct* tstr indent(out) << "oprot.writeStructBegin(STRUCT_DESC);" << endl; for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { + bool could_be_unset = (*f_iter)->get_req() == t_field::T_OPTIONAL; + if (could_be_unset) { + indent(out) << "if (" << generate_isset_check(*f_iter) << ") {" << endl; + indent_up(); + } bool null_allowed = type_can_be_null((*f_iter)->get_type()); if (null_allowed) { out << indent() << "if (this." << (*f_iter)->get_name() << " != null) {" << endl; @@ -997,6 +1002,10 @@ void t_haxe_generator::generate_haxe_struct_writer(ofstream& out, t_struct* tstr indent_down(); indent(out) << "}" << endl; } + if (could_be_unset) { + indent_down(); + indent(out) << "}" << endl; + } } indent(out) << "oprot.writeFieldStop();" << endl; From 6a1fb354f24e1e48b4767ec8d54bb66ed984555a Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Sun, 21 Feb 2016 15:19:45 +0100 Subject: [PATCH 3/4] THRIFT-3652 incorrect serialization of optionals Client: AS3 Patch: Jens Geyer --- compiler/cpp/src/generate/t_as3_generator.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/compiler/cpp/src/generate/t_as3_generator.cc b/compiler/cpp/src/generate/t_as3_generator.cc index c0a03dce22d..67beaf640ab 100644 --- a/compiler/cpp/src/generate/t_as3_generator.cc +++ b/compiler/cpp/src/generate/t_as3_generator.cc @@ -926,6 +926,11 @@ void t_as3_generator::generate_as3_struct_writer(ofstream& out, t_struct* tstruc indent(out) << "oprot.writeStructBegin(STRUCT_DESC);" << endl; for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { + bool could_be_unset = (*f_iter)->get_req() == t_field::T_OPTIONAL; + if (could_be_unset) { + indent(out) << "if (" << generate_isset_check(*f_iter) << ") {" << endl; + indent_up(); + } bool null_allowed = type_can_be_null((*f_iter)->get_type()); if (null_allowed) { out << indent() << "if (this." << (*f_iter)->get_name() << " != null) {" << endl; @@ -945,6 +950,10 @@ void t_as3_generator::generate_as3_struct_writer(ofstream& out, t_struct* tstruc indent_down(); indent(out) << "}" << endl; } + if (could_be_unset) { + indent_down(); + indent(out) << "}" << endl; + } } // Write the struct map out << indent() << "oprot.writeFieldStop();" << endl << indent() << "oprot.writeStructEnd();" From 695d5cb7a75f9e2d3625e7c342d6ef5101f5f774 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Sun, 21 Feb 2016 15:23:37 +0100 Subject: [PATCH 4/4] THRIFT-3656 incorrect serialization of optionals Client: Dart Patch: Jens Geyer --- compiler/cpp/src/generate/t_dart_generator.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/compiler/cpp/src/generate/t_dart_generator.cc b/compiler/cpp/src/generate/t_dart_generator.cc index 3333d165886..7bae327880d 100644 --- a/compiler/cpp/src/generate/t_dart_generator.cc +++ b/compiler/cpp/src/generate/t_dart_generator.cc @@ -1010,6 +1010,11 @@ void t_dart_generator::generate_dart_struct_writer(ofstream& out, t_struct* tstr for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { string field_name = get_field_name((*f_iter)->get_name()); + bool could_be_unset = (*f_iter)->get_req() == t_field::T_OPTIONAL; + if (could_be_unset) { + indent(out) << "if (" << generate_isset_check(*f_iter) << ")"; + scope_up(out); + } bool null_allowed = type_can_be_null((*f_iter)->get_type()); if (null_allowed) { indent(out) << "if (this." << field_name << " != null)"; @@ -1028,6 +1033,9 @@ void t_dart_generator::generate_dart_struct_writer(ofstream& out, t_struct* tstr if (null_allowed) { scope_down(out); } + if (could_be_unset) { + scope_down(out); + } } // Write the struct map indent(out) << "oprot.writeFieldStop();" << endl << indent() << "oprot.writeStructEnd();"