From 27ff942b5c248719c9937db66b26bc2d60e1d465 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Mon, 21 Nov 2022 23:21:51 +0100 Subject: THRIFT-5669 "required" keyword is illegal in a "throws" clause --- compiler/cpp/src/thrift/parse/t_function.h | 2 + compiler/cpp/src/thrift/parse/t_struct.h | 138 +++++++++++++++++++---------- 2 files changed, 95 insertions(+), 45 deletions(-) (limited to 'compiler') diff --git a/compiler/cpp/src/thrift/parse/t_function.h b/compiler/cpp/src/thrift/parse/t_function.h index bc0ae465b..a3b6f48e3 100644 --- a/compiler/cpp/src/thrift/parse/t_function.h +++ b/compiler/cpp/src/thrift/parse/t_function.h @@ -40,6 +40,7 @@ public: xceptions_(new t_struct(nullptr)), own_xceptions_(true), oneway_(oneway) { + xceptions_->set_method_xcepts(true); if (oneway_ && (!returntype_->is_void())) { pwarning(1, "Oneway methods should return void.\n"); } @@ -56,6 +57,7 @@ public: xceptions_(xceptions), own_xceptions_(false), oneway_(oneway) { + xceptions_->set_method_xcepts(true); if (oneway_ && !xceptions_->get_members().empty()) { throw std::string("Oneway methods can't throw exceptions."); } diff --git a/compiler/cpp/src/thrift/parse/t_struct.h b/compiler/cpp/src/thrift/parse/t_struct.h index 7e1e6caf0..d990eadd5 100644 --- a/compiler/cpp/src/thrift/parse/t_struct.h +++ b/compiler/cpp/src/thrift/parse/t_struct.h @@ -44,64 +44,40 @@ public: : t_type(program), is_xception_(false), is_union_(false), - members_validated(false), - members_with_value(0), + is_method_xcepts_(false), + union_validated_(false), + xcepts_validated_(false), + members_with_value_(0), xsd_all_(false) {} t_struct(t_program* program, const std::string& name) : t_type(program, name), is_xception_(false), is_union_(false), - members_validated(false), - members_with_value(0), + is_method_xcepts_(false), + union_validated_(false), + xcepts_validated_(false), + members_with_value_(0), xsd_all_(false) {} void set_name(const std::string& name) override { name_ = name; - validate_union_members(); + union_validated_= false; + validate_members(); } void set_xception(bool is_xception) { is_xception_ = is_xception; } - void validate_union_member(t_field* field) { - if (is_union_ && (!name_.empty())) { - - // 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); - } - - // unions may have up to one member defaulted, but not more - if (field->get_value() != nullptr) { - if (1 < ++members_with_value) { - throw "Error: Field " + field->get_name() + " provides another default value for union " - + name_; - } - } - } - } - - void validate_union_members() { - if (is_union_ && (!name_.empty()) && (!members_validated)) { - members_type::const_iterator m_iter; - for (m_iter = members_in_id_order_.begin(); m_iter != members_in_id_order_.end(); ++m_iter) { - validate_union_member(*m_iter); - } - members_validated = true; - } + void set_method_xcepts(bool is_method_xcepts) { + is_method_xcepts_ = is_method_xcepts; + xcepts_validated_ = false; + validate_members(); } void set_union(bool is_union) { is_union_ = is_union; - validate_union_members(); + union_validated_= false; + validate_members(); } void set_xsd_all(bool xsd_all) { xsd_all_ = xsd_all; } @@ -123,7 +99,11 @@ public: } members_.push_back(elem); members_in_id_order_.insert(bounds.second, elem); - validate_union_member(elem); + if (needs_validation()) { + validate_members(); + } else { + validate_member_field(elem); + } return true; } @@ -154,12 +134,80 @@ public: private: members_type members_; members_type members_in_id_order_; - bool is_xception_; - bool is_union_; - bool members_validated; - int members_with_value; + bool is_xception_; // struct is an IDL exception + bool is_union_; // struct is an IDL union + bool is_method_xcepts_; // struct holds the exceptions declared at a service method + bool union_validated_; + bool xcepts_validated_; + int members_with_value_; bool xsd_all_; + + void validate_member_field(t_field* field) { + validate_union_member(field); + validate_method_exception_field(field); + } + + void validate_union_member(t_field* field) { + if (is_union_ && (!name_.empty())) { + union_validated_ = true; + + // 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); + } + + // unions may have up to one member defaulted, but not more + if (field->get_value() != nullptr) { + if (1 < ++members_with_value_) { + throw "Error: Field " + field->get_name() + " provides another default value for union " + + name_; + } + } + } + } + + void validate_method_exception_field(t_field* field) { + if (is_method_xcepts_) { + xcepts_validated_ = true; + + // THRIFT-5669: "required" makes no sense at "throws" clauses + if (field->get_req() == t_field::T_REQUIRED) { + field->set_req(t_field::T_OPT_IN_REQ_OUT); + pwarning(1, + "Exception field %s: \"required\" is illegal here, ignoring.\n", + field->get_name().c_str()); + } + } + } + + bool needs_validation() { + if (is_method_xcepts_) { + return !xcepts_validated_; + } + if (is_union_) { + return !union_validated_; + } + return false; + } + + void validate_members() { + if (needs_validation()) { + members_type::const_iterator m_iter; + for (m_iter = members_in_id_order_.begin(); m_iter != members_in_id_order_.end(); ++m_iter) { + validate_member_field(*m_iter); + } + } + } + }; #endif -- cgit v1.2.1