From b17672ac38cbc9347e1331798f4e6bc91502fdd2 Mon Sep 17 00:00:00 2001 From: Yawar Amin Date: Sat, 10 Sep 2022 11:55:02 -0400 Subject: THRIFT-5208: fix OCaml struct and exn raising/handling codegen Client: ocaml Patch: Yawar Amin --- .../cpp/src/thrift/generate/t_ocaml_generator.cc | 48 +++--- compiler/cpp/tests/CMakeLists.txt | 4 +- compiler/cpp/tests/README.md | 4 +- compiler/cpp/tests/ocaml/README.md | 16 ++ .../cpp/tests/ocaml/snapshot_exception_types_i.cc | 27 ++++ .../cpp/tests/ocaml/snapshot_service_handle_ex.cc | 161 +++++++++++++++++++++ compiler/cpp/tests/ocaml/snapshot_typedefs.cc | 22 +++ .../cpp/tests/ocaml/t_ocaml_generator_tests.cc | 111 ++++++++++++++ 8 files changed, 368 insertions(+), 25 deletions(-) create mode 100644 compiler/cpp/tests/ocaml/README.md create mode 100644 compiler/cpp/tests/ocaml/snapshot_exception_types_i.cc create mode 100644 compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc create mode 100644 compiler/cpp/tests/ocaml/snapshot_typedefs.cc create mode 100644 compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc (limited to 'compiler') diff --git a/compiler/cpp/src/thrift/generate/t_ocaml_generator.cc b/compiler/cpp/src/thrift/generate/t_ocaml_generator.cc index 5e86de4fc..747adcbac 100644 --- a/compiler/cpp/src/thrift/generate/t_ocaml_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_ocaml_generator.cc @@ -61,12 +61,9 @@ public: out_dir_base_ = "gen-ocaml"; } - /** - * Init and close methods - */ + ~t_ocaml_generator() override; void init_generator() override; - void close_generator() override; /** * Program-level generation functions @@ -147,16 +144,20 @@ public: * Helper rendering functions */ - std::string ocaml_autogen_comment(); + /** Need to disable codegen comment for unit tests to be version-agnostic */ + virtual std::string ocaml_autogen_comment(); + std::string ocaml_imports(); std::string type_name(t_type* ttype); + std::string exception_ctor(t_type* ttype); std::string function_signature(t_function* tfunction, std::string prefix = ""); std::string function_type(t_function* tfunc, bool method = false, bool options = false); std::string argument_list(t_struct* tstruct); std::string type_to_enum(t_type* ttype); std::string render_ocaml_type(t_type* type); -private: +// Need access to output file streams for testing. +protected: /** * File streams */ @@ -216,9 +217,6 @@ void t_ocaml_generator::generate_program() { // Generate constants vector consts = program_->get_consts(); generate_consts(consts); - - // Close the generator - close_generator(); } /** @@ -262,12 +260,12 @@ string t_ocaml_generator::ocaml_imports() { return "open Thrift"; } -/** - * Closes the type files - */ -void t_ocaml_generator::close_generator() { - // Close types file +t_ocaml_generator::~t_ocaml_generator() { + f_consts_.close(); f_types_.close(); + f_types_i_.close(); + f_service_.close(); + f_service_i_.close(); } /** @@ -914,10 +912,6 @@ void t_ocaml_generator::generate_service(t_service* tservice) { generate_service_interface(tservice); generate_service_client(tservice); generate_service_server(tservice); - - // Close service file - f_service_.close(); - f_service_i_.close(); } /** @@ -1108,7 +1102,7 @@ void t_ocaml_generator::generate_service_client(t_service* tservice) { for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) { f_service_ << indent() << "(match result#get_" << (*x_iter)->get_name() << " with None -> () | Some _v ->" << endl; - indent(f_service_) << " raise (" << capitalize(type_name((*x_iter)->get_type())) + indent(f_service_) << " raise (" << capitalize(exception_ctor((*x_iter)->get_type())) << " _v));" << endl; } @@ -1270,7 +1264,7 @@ void t_ocaml_generator::generate_process_function(t_service* tservice, t_functio indent(f_service_) << "with" << endl; indent_up(); for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) { - f_service_ << indent() << "| " << capitalize(type_name((*x_iter)->get_type())) << " " + f_service_ << indent() << "| " << capitalize(exception_ctor((*x_iter)->get_type())) << " " << (*x_iter)->get_name() << " -> " << endl; indent_up(); indent_up(); @@ -1665,7 +1659,7 @@ string t_ocaml_generator::type_name(t_type* ttype) { } string name = ttype->get_name(); - if (ttype->is_service() || ttype->is_xception()) { + if (ttype->is_service()) { name = capitalize(name); } else { name = decapitalize(name); @@ -1673,6 +1667,18 @@ string t_ocaml_generator::type_name(t_type* ttype) { return prefix + name; } +string t_ocaml_generator::exception_ctor(t_type* ttype) { + string prefix = ""; + t_program* program = ttype->get_program(); + if (program != nullptr && program != program_) { + if (!ttype->is_service()) { + prefix = capitalize(program->get_name()) + "_types."; + } + } + + return prefix + capitalize(ttype->get_name()); +} + /** * Converts the parse type to a Protocol.t_type enum */ diff --git a/compiler/cpp/tests/CMakeLists.txt b/compiler/cpp/tests/CMakeLists.txt index b8b2777a9..d9c520913 100644 --- a/compiler/cpp/tests/CMakeLists.txt +++ b/compiler/cpp/tests/CMakeLists.txt @@ -80,7 +80,7 @@ set(thrift_compiler_SOURCES ${THRIFT_COMPILER_SOURCE_DIR}/src/thrift/generate/validator_parser.h ${THRIFT_COMPILER_SOURCE_DIR}/src/thrift/parse/t_typedef.cc ${THRIFT_COMPILER_SOURCE_DIR}/src/thrift/parse/parse.cc - ${THRIFT_COMPILER_SOURCE_DIR}/thrift/version.h + ${THRIFT_COMPILER_SOURCE_DIR}/src/thrift/version.h ) # This macro adds an option THRIFT_COMPILER_${NAME} @@ -128,7 +128,7 @@ THRIFT_ADD_COMPILER(js "Enable compiler for JavaScript" OFF) THRIFT_ADD_COMPILER(json "Enable compiler for JSON" OFF) THRIFT_ADD_COMPILER(lua "Enable compiler for Lua" OFF) THRIFT_ADD_COMPILER(netstd "Enable compiler for .NET Standard" ON) -THRIFT_ADD_COMPILER(ocaml "Enable compiler for OCaml" OFF) +THRIFT_ADD_COMPILER(ocaml "Enable compiler for OCaml" ON) THRIFT_ADD_COMPILER(perl "Enable compiler for Perl" OFF) THRIFT_ADD_COMPILER(php "Enable compiler for PHP" OFF) THRIFT_ADD_COMPILER(py "Enable compiler for Python 2.0" OFF) diff --git a/compiler/cpp/tests/README.md b/compiler/cpp/tests/README.md index 91c0625a3..e45e29858 100644 --- a/compiler/cpp/tests/README.md +++ b/compiler/cpp/tests/README.md @@ -26,7 +26,7 @@ Also, maybe, later it will be migrated to Catch2 (https://github.com/philsquared ## How to add your tests - Open **CMakeLists.txt** -- Set **On** to call of **THRIFT_ADD_COMPILER** for your language +- Set call of `THRIFT_ADD_COMPILER` for your language to `ON` ``` cmake THRIFT_ADD_COMPILER(netstd "Enable compiler for .NET Standard" ON) @@ -85,4 +85,4 @@ cd cmake-vs cmake .. cmake --build . ctest -C Debug -V -``` \ No newline at end of file +``` diff --git a/compiler/cpp/tests/ocaml/README.md b/compiler/cpp/tests/ocaml/README.md new file mode 100644 index 000000000..e79a88792 --- /dev/null +++ b/compiler/cpp/tests/ocaml/README.md @@ -0,0 +1,16 @@ +## Testing approach + +1. Programmatically construct parsed instances of Thrift IDLs using internal + types +2. Generate the OCaml output using the OCaml generator +3. Capture the generated output in `ostringstream` +4. Query and compare the outputs in the strings to stored snapshots in the + `snapshot_*.cc` files + +Run tests in `../tests` directory: + + # Only on changing build definition: + cmake -DCMAKE_PREFIX_PATH=/usr/local/opt/bison -DCMAKE_CXX_STANDARD=11 . + + # On each iteration: + rm -rf gen-ocaml; cmake --build . && ctest --output-on-failure diff --git a/compiler/cpp/tests/ocaml/snapshot_exception_types_i.cc b/compiler/cpp/tests/ocaml/snapshot_exception_types_i.cc new file mode 100644 index 000000000..a7d908f6e --- /dev/null +++ b/compiler/cpp/tests/ocaml/snapshot_exception_types_i.cc @@ -0,0 +1,27 @@ +// 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. + +const char* snapshot(R"""( +open Thrift +class serverError : +object ('a) + method copy : 'a + method write : Protocol.t -> unit +end +exception ServerError of serverError +val read_serverError : Protocol.t -> serverError +)"""); diff --git a/compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc b/compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc new file mode 100644 index 000000000..f20d6987f --- /dev/null +++ b/compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc @@ -0,0 +1,161 @@ +// 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. + +const char* snapshot(R"""( +open Thrift +open Service_types + +(* HELPER FUNCTIONS AND STRUCTURES *) + +class ping_args = +object (self) + method copy = + let _new = Oo.copy self in + _new + method write (oprot : Protocol.t) = + oprot#writeStructBegin "ping_args"; + oprot#writeFieldStop; + oprot#writeStructEnd +end +let rec read_ping_args (iprot : Protocol.t) = + let _str2 = new ping_args in + ignore(iprot#readStructBegin); + (try while true do + let (_,_t3,_id4) = iprot#readFieldBegin in + if _t3 = Protocol.T_STOP then + raise Break + else (); + (match _id4 with + | _ -> iprot#skip _t3); + iprot#readFieldEnd; + done; () + with Break -> ()); + iprot#readStructEnd; + _str2 + +class ping_result = +object (self) + val mutable _serverError : Errors_types.serverError option = None + method get_serverError = _serverError + method grab_serverError = match _serverError with None->raise (Field_empty "ping_result.serverError") | Some _x5 -> _x5 + method set_serverError _x5 = _serverError <- Some _x5 + method unset_serverError = _serverError <- None + method reset_serverError = _serverError <- None + + method copy = + let _new = Oo.copy self in + if _serverError <> None then + _new#set_serverError self#grab_serverError#copy; + _new + method write (oprot : Protocol.t) = + oprot#writeStructBegin "ping_result"; + (match _serverError with None -> () | Some _v -> + oprot#writeFieldBegin("serverError",Protocol.T_STRUCT,1); + _v#write(oprot); + oprot#writeFieldEnd + ); + oprot#writeFieldStop; + oprot#writeStructEnd +end +let rec read_ping_result (iprot : Protocol.t) = + let _str8 = new ping_result in + ignore(iprot#readStructBegin); + (try while true do + let (_,_t9,_id10) = iprot#readFieldBegin in + if _t9 = Protocol.T_STOP then + raise Break + else (); + (match _id10 with + | 1 -> (if _t9 = Protocol.T_STRUCT then + _str8#set_serverError (Errors_types.read_serverError iprot) + else + iprot#skip _t9) + | _ -> iprot#skip _t9); + iprot#readFieldEnd; + done; () + with Break -> ()); + iprot#readStructEnd; + _str8 + +class virtual iface = +object (self) + method virtual ping : unit +end + +class client (iprot : Protocol.t) (oprot : Protocol.t) = +object (self) + val mutable seqid = 0 + method ping = + self#send_ping; + self#recv_ping + method private send_ping = + oprot#writeMessageBegin ("ping", Protocol.CALL, seqid); + let args = new ping_args in + args#write oprot; + oprot#writeMessageEnd; + oprot#getTransport#flush + method private recv_ping = + let (fname, mtype, rseqid) = iprot#readMessageBegin in + (if mtype = Protocol.EXCEPTION then + let x = Application_Exn.read iprot in + (iprot#readMessageEnd; raise (Application_Exn.E x)) + else ()); + let result = read_ping_result iprot in + iprot#readMessageEnd; + (match result#get_serverError with None -> () | Some _v -> + raise (Errors_types.ServerError _v)); + () +end + +class processor (handler : iface) = +object (self) + inherit Processor.t + + val processMap = Hashtbl.create 1 + method process iprot oprot = + let (name, typ, seqid) = iprot#readMessageBegin in + if Hashtbl.mem processMap name then + (Hashtbl.find processMap name) (seqid, iprot, oprot) + else ( + iprot#skip(Protocol.T_STRUCT); + iprot#readMessageEnd; + let x = Application_Exn.create Application_Exn.UNKNOWN_METHOD ("Unknown function "^name) in + oprot#writeMessageBegin(name, Protocol.EXCEPTION, seqid); + x#write oprot; + oprot#writeMessageEnd; + oprot#getTransport#flush + ); + true + method private process_ping (seqid, iprot, oprot) = + let _ = read_ping_args iprot in + iprot#readMessageEnd; + let result = new ping_result in + (try + (handler#ping); + with + | Errors_types.ServerError serverError -> + result#set_serverError serverError + ); + oprot#writeMessageBegin ("ping", Protocol.REPLY, seqid); + result#write oprot; + oprot#writeMessageEnd; + oprot#getTransport#flush + initializer + Hashtbl.add processMap "ping" self#process_ping; +end + +)"""); diff --git a/compiler/cpp/tests/ocaml/snapshot_typedefs.cc b/compiler/cpp/tests/ocaml/snapshot_typedefs.cc new file mode 100644 index 000000000..473b2e819 --- /dev/null +++ b/compiler/cpp/tests/ocaml/snapshot_typedefs.cc @@ -0,0 +1,22 @@ +// 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. + +const char* snapshot(R"""( +open Thrift +type decimal = string + +)"""); diff --git a/compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc b/compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc new file mode 100644 index 000000000..ea788fcd7 --- /dev/null +++ b/compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc @@ -0,0 +1,111 @@ +// 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. + +#include +#include +#include "../catch/catch.hpp" +#include + +using defs_t = std::list; + +/** This subclass is meant to allow accessing the Thrift generated OCaml outputs + and keep the tests stable across Thrift versions (as much as possible), but + otherwise is identical to the standard OCaml generator. */ +class t_test_ocaml_generator : public t_ocaml_generator { +public: + t_test_ocaml_generator(t_program* program) : t_ocaml_generator(program, {}, "") {} + + /** Override and turn off comment generation which contains a version number + to make tests version-independent. */ + std::string ocaml_autogen_comment() override { return ""; } + + // Allow inspecting the generated code. + + string types() { return f_types_.str(); } + string consts() { return f_consts_.str(); } + string service() { return f_service_.str(); } + string types_i() { return f_types_i_.str(); } + string service_i() { return f_service_i_.str(); } +}; + +/** Helper to add a list of definitions to a Thrift 'program' (i.e. + representation of the IDL) and generate the OCaml outputs. */ +void gen_program(t_generator& gen, t_program& program, defs_t defs) { + for (auto def : defs) { + if (def->is_typedef()) program.add_typedef(static_cast(def)); + else if (def->is_enum()) program.add_enum(static_cast(def)); + else if (def->is_struct()) program.add_struct(static_cast(def)); + else if (def->is_xception()) program.add_xception(static_cast(def)); + else if (def->is_service()) program.add_service(static_cast(def)); + } + + gen.generate_program(); +} + +TEST_CASE( "t_ocaml_generator - typedefs", "[functional]" ) +{ + t_program program("Typedefs.thrift", "Typedefs"); + t_base_type ty_string("string", t_base_type::TYPE_STRING); + t_typedef tydef_decimal(&program, &ty_string, "Decimal"); + t_test_ocaml_generator gen(&program); + + gen_program(gen, program, defs_t { + &tydef_decimal + }); + + #include "snapshot_typedefs.cc" + REQUIRE( snapshot == gen.types() ); +} + +TEST_CASE( "t_ocaml_generator - handle exception from different module", "[functional]" ) +{ + t_program errors_thrift("Errors.thrift", "Errors"); + t_struct server_error(&errors_thrift, "ServerError"); + server_error.set_xception(true); + + t_test_ocaml_generator errors_gen(&errors_thrift); + gen_program(errors_gen, errors_thrift, defs_t { + &server_error + }); + + { + #include "snapshot_exception_types_i.cc" + REQUIRE( snapshot == errors_gen.types_i() ); + } + + t_program service_thrift("Service.thrift", "Service"); + t_service service(&service_thrift); + service.set_name("Service"); + t_base_type ret_type("void", t_base_type::TYPE_VOID); + t_struct args(&service_thrift, "ping_args"); + t_struct throws(&service_thrift, "ping_throws"); + t_field ex_server_error(&server_error, "serverError", 1); + throws.append(&ex_server_error); + t_function ping(&ret_type, "ping", &args, &throws); + service.add_function(&ping); + + t_test_ocaml_generator service_gen(&service_thrift); + + gen_program(service_gen, service_thrift, defs_t { + &service + }); + + { + #include "snapshot_service_handle_ex.cc" + REQUIRE( snapshot == service_gen.service() ); + } +} -- cgit v1.2.1