From 29c131f79036fc73a9bd909754d1b35aac7c59ac Mon Sep 17 00:00:00 2001 From: Feso Date: Sat, 13 Oct 2018 19:17:26 +0100 Subject: [PATCH] Refactored based on code review --- test/cpp/util/grpc_tool.cc | 79 ++++++---------- test/cpp/util/proto_file_parser.cc | 139 ++++++++++------------------- test/cpp/util/proto_file_parser.h | 64 +++++++------ 3 files changed, 107 insertions(+), 175 deletions(-) diff --git a/test/cpp/util/grpc_tool.cc b/test/cpp/util/grpc_tool.cc index b4eeee1a9b5..a82e7418aba 100644 --- a/test/cpp/util/grpc_tool.cc +++ b/test/cpp/util/grpc_tool.cc @@ -193,8 +193,9 @@ void ReadResponse(CliCall* call, const grpc::string& method_name, fprintf(stderr, "got response.\n"); if (!FLAGS_binary_output) { gpr_mu_lock(parser_mu); - serialized_response_proto = parser->GetTextFormatFromMethod( - method_name, serialized_response_proto, false /* is_request */); + serialized_response_proto = parser->GetFormattedStringFromMethod( + method_name, serialized_response_proto, false /* is_request */, + FLAGS_json_output); if (parser->HasError() && print_mode) { fprintf(stderr, "Failed to parse response.\n"); } @@ -555,12 +556,9 @@ bool GrpcTool::CallMethod(int argc, const char** argv, request_text.clear(); } else { gpr_mu_lock(&parser_mu); - serialized_request_proto = - FLAGS_json_input ? - parser->GetSerializedProtoFromMethodJsonFormat( - method_name, request_text, true /* is_request */) : - parser->GetSerializedProtoFromMethodTextFormat( - method_name, request_text, true /* is_request */); + serialized_request_proto = parser->GetSerializedProtoFromMethod( + method_name, request_text, true /* is_request */, + FLAGS_json_input); request_text.clear(); if (parser->HasError()) { if (print_mode) { @@ -643,12 +641,9 @@ bool GrpcTool::CallMethod(int argc, const char** argv, serialized_request_proto = request_text; request_text.clear(); } else { - serialized_request_proto = - FLAGS_json_input ? - parser->GetSerializedProtoFromMethodJsonFormat( - method_name, request_text, true /* is_request */) : - parser->GetSerializedProtoFromMethodTextFormat( - method_name, request_text, true /* is_request */); + serialized_request_proto = parser->GetSerializedProtoFromMethod( + method_name, request_text, true /* is_request */, + FLAGS_json_input); request_text.clear(); if (parser->HasError()) { if (print_mode) { @@ -684,14 +679,10 @@ bool GrpcTool::CallMethod(int argc, const char** argv, break; } } else { - grpc::string response_text = - FLAGS_json_output ? - parser->GetJsonFormatFromMethod(method_name, + grpc::string response_text = parser->GetFormattedStringFromMethod(method_name, serialized_response_proto, - false /* is_request */) : - parser->GetTextFormatFromMethod(method_name, - serialized_response_proto, - false /* is_request */); + false /* is_request */, + FLAGS_json_output); if (parser->HasError() && print_mode) { fprintf(stderr, "Failed to parse response.\n"); @@ -748,12 +739,8 @@ bool GrpcTool::CallMethod(int argc, const char** argv, if (FLAGS_binary_input) { serialized_request_proto = request_text; } else { - serialized_request_proto = - FLAGS_json_input ? - parser->GetSerializedProtoFromMethodJsonFormat( - method_name, request_text, true /* is_request */) : - parser->GetSerializedProtoFromMethodTextFormat( - method_name, request_text, true /* is_request */); + serialized_request_proto = parser->GetSerializedProtoFromMethod( + method_name, request_text, true /* is_request */, FLAGS_json_input); if (parser->HasError()) { fprintf(stderr, "Failed to parse request.\n"); return false; @@ -776,18 +763,14 @@ bool GrpcTool::CallMethod(int argc, const char** argv, &serialized_response_proto, receive_initial_metadata ? &server_initial_metadata : nullptr); receive_initial_metadata = false) { - if (FLAGS_json_output) { - serialized_response_proto = parser->GetJsonFormatFromMethod( - method_name, serialized_response_proto, false /* is_request */); - } else if (!FLAGS_binary_output) { - serialized_response_proto = parser->GetTextFormatFromMethod( - method_name, serialized_response_proto, false /* is_request */); - } - if (FLAGS_json_output || !FLAGS_binary_output) { - if (parser->HasError()) { - fprintf(stderr, "Failed to parse response.\n"); - return false; - } + if (!FLAGS_binary_output) { + serialized_response_proto = parser->GetFormattedStringFromMethod( + method_name, serialized_response_proto, false /* is_request */, + FLAGS_json_output); + if (parser->HasError()) { + fprintf(stderr, "Failed to parse response.\n"); + return false; + } } if (receive_initial_metadata) { @@ -878,12 +861,8 @@ bool GrpcTool::ParseMessage(int argc, const char** argv, if (FLAGS_binary_input) { serialized_request_proto = message_text; } else { - serialized_request_proto = - FLAGS_json_input ? - parser->GetSerializedProtoFromMessageTypeJsonFormat(type_name, - message_text) : - parser->GetSerializedProtoFromMessageTypeTextFormat(type_name, - message_text); + serialized_request_proto = parser->GetSerializedProtoFromMessageType( + type_name, message_text, FLAGS_json_input); if (parser->HasError()) { fprintf(stderr, "Failed to serialize the message.\n"); return false; @@ -894,14 +873,8 @@ bool GrpcTool::ParseMessage(int argc, const char** argv, output_ss << serialized_request_proto; } else { grpc::string output_text; - if (FLAGS_json_output) { - output_text = parser->GetJsonFormatFromMessageType( - type_name, serialized_request_proto); - } else { - output_text = parser->GetTextFormatFromMessageType( - type_name, serialized_request_proto); - } - + output_text = parser->GetFormattedStringFromMessageType( + type_name, serialized_request_proto, FLAGS_json_output); if (parser->HasError()) { fprintf(stderr, "Failed to deserialize the message.\n"); return false; diff --git a/test/cpp/util/proto_file_parser.cc b/test/cpp/util/proto_file_parser.cc index 997808cbc36..71bda1005da 100644 --- a/test/cpp/util/proto_file_parser.cc +++ b/test/cpp/util/proto_file_parser.cc @@ -216,55 +216,33 @@ bool ProtoFileParser::IsStreaming(const grpc::string& method, bool is_request) { : method_desc->server_streaming(); } -grpc::string ProtoFileParser::GetSerializedProtoFromMethodTextFormat( - const grpc::string& method, const grpc::string& text_format_proto, - bool is_request) { +grpc::string ProtoFileParser::GetSerializedProtoFromMethod( + const grpc::string& method, const grpc::string& formatted_proto, + bool is_request, bool is_json_format) { has_error_ = false; grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request); if (has_error_) { return ""; } - return GetSerializedProtoFromMessageTypeTextFormat(message_type_name, - text_format_proto); + return GetSerializedProtoFromMessageType(message_type_name, formatted_proto, + is_json_format); } -grpc::string ProtoFileParser::GetSerializedProtoFromMethodJsonFormat( - const grpc::string& method, const grpc::string& json_format_proto, - bool is_request) { - has_error_ = false; - grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request); - if (has_error_) { - return ""; - } - return GetSerializedProtoFromMessageTypeJsonFormat(message_type_name, - json_format_proto); -} - -grpc::string ProtoFileParser::GetTextFormatFromMethod( - const grpc::string& method, const grpc::string& serialized_proto, - bool is_request) { - has_error_ = false; - grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request); - if (has_error_) { - return ""; - } - return GetTextFormatFromMessageType(message_type_name, serialized_proto); -} - -grpc::string ProtoFileParser::GetJsonFormatFromMethod( +grpc::string ProtoFileParser::GetFormattedStringFromMethod( const grpc::string& method, const grpc::string& serialized_proto, - bool is_request) { + bool is_request, bool is_json_format) { has_error_ = false; grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request); if (has_error_) { return ""; } - return GetJsonFormatFromMessageType(message_type_name, serialized_proto); + return GetFormattedStringFromMessageType(message_type_name, serialized_proto, is_json_format); } -grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeTextFormat( +grpc::string ProtoFileParser::GetSerializedProtoFromMessageType( const grpc::string& message_type_name, - const grpc::string& text_format_proto) { + const grpc::string& formatted_proto, + bool is_json_format) { has_error_ = false; grpc::string serialized; const protobuf::Descriptor* desc = @@ -275,38 +253,24 @@ grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeTextFormat( } std::unique_ptr msg( dynamic_factory_->GetPrototype(desc)->New()); - bool ok = protobuf::TextFormat::ParseFromString(text_format_proto, msg.get()); - if (!ok) { - LogError("Failed to parse text format to proto."); - return ""; - } - ok = msg->SerializeToString(&serialized); - if (!ok) { - LogError("Failed to serialize proto."); - return ""; - } - return serialized; -} -grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeJsonFormat( - const grpc::string& message_type_name, - const grpc::string& json_format_proto) { - has_error_ = false; - grpc::string serialized; - const protobuf::Descriptor* desc = - desc_pool_->FindMessageTypeByName(message_type_name); - if (!desc) { - LogError("Message type not found"); - return ""; + bool ok; + if (is_json_format) { + ok = grpc::protobuf::json::JsonStringToMessage(formatted_proto, msg.get()) + .ok(); + if (!ok) { + LogError("Failed to convert json format to proto."); + return ""; + } + } else { + ok = protobuf::TextFormat::ParseFromString(formatted_proto, msg.get()); + if (!ok) { + LogError("Failed to convert text format to proto."); + return ""; + } } - std::unique_ptr msg( - dynamic_factory_->GetPrototype(desc)->New()); - if (!grpc::protobuf::json::JsonStringToMessage(json_format_proto, msg.get()).ok()) { - LogError("Failed to parse json format to proto."); - return ""; - } - bool ok = msg->SerializeToString(&serialized); + ok = msg->SerializeToString(&serialized); if (!ok) { LogError("Failed to serialize proto."); return ""; @@ -314,9 +278,10 @@ grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeJsonFormat( return serialized; } -grpc::string ProtoFileParser::GetTextFormatFromMessageType( +grpc::string ProtoFileParser::GetFormattedStringFromMessageType( const grpc::string& message_type_name, - const grpc::string& serialized_proto) { + const grpc::string& serialized_proto, + bool is_json_format) { has_error_ = false; const protobuf::Descriptor* desc = desc_pool_->FindMessageTypeByName(message_type_name); @@ -330,38 +295,24 @@ grpc::string ProtoFileParser::GetTextFormatFromMessageType( LogError("Failed to deserialize proto."); return ""; } - grpc::string text_format; - if (!protobuf::TextFormat::PrintToString(*msg.get(), &text_format)) { - LogError("Failed to print proto message to text format"); - return ""; - } - return text_format; -} + grpc::string formatted_string; -grpc::string ProtoFileParser::GetJsonFormatFromMessageType( - const grpc::string& message_type_name, - const grpc::string& serialized_proto) { - has_error_ = false; - const protobuf::Descriptor* desc = - desc_pool_->FindMessageTypeByName(message_type_name); - if (!desc) { - LogError("Message type not found"); - return ""; - } - std::unique_ptr msg( - dynamic_factory_->GetPrototype(desc)->New()); - if (!msg->ParseFromString(serialized_proto)) { - LogError("Failed to deserialize proto."); - return ""; - } - grpc::string json_format; - grpc::protobuf::json::JsonPrintOptions jsonPrintOptions; - jsonPrintOptions.add_whitespace = true; - if (!grpc::protobuf::json::MessageToJsonString(*msg.get(), &json_format, jsonPrintOptions).ok()) { - LogError("Failed to print proto message to json format"); - return ""; + if (is_json_format) { + grpc::protobuf::json::JsonPrintOptions jsonPrintOptions; + jsonPrintOptions.add_whitespace = true; + if (!grpc::protobuf::json::MessageToJsonString(*msg.get(), + &formatted_string, + jsonPrintOptions).ok()) { + LogError("Failed to print proto message to json format"); + return ""; + } + } else { + if (!protobuf::TextFormat::PrintToString(*msg.get(), &formatted_string)) { + LogError("Failed to print proto message to text format"); + return ""; + } } - return json_format; + return formatted_string; } void ProtoFileParser::LogError(const grpc::string& error_msg) { diff --git a/test/cpp/util/proto_file_parser.h b/test/cpp/util/proto_file_parser.h index d2b2bd753e4..0758d2f9f31 100644 --- a/test/cpp/util/proto_file_parser.h +++ b/test/cpp/util/proto_file_parser.h @@ -53,37 +53,45 @@ class ProtoFileParser { // used as the argument of Stub::Call() grpc::string GetFormattedMethodName(const grpc::string& method); - grpc::string GetSerializedProtoFromMethodTextFormat( - const grpc::string& method, const grpc::string& text_format_proto, - bool is_request); - - grpc::string GetSerializedProtoFromMethodJsonFormat( - const grpc::string& method, const grpc::string& json_format_proto, - bool is_request); - - grpc::string GetTextFormatFromMethod(const grpc::string& method, - const grpc::string& serialized_proto, - bool is_request); - - grpc::string GetJsonFormatFromMethod(const grpc::string& method, - const grpc::string& serialized_proto, - bool is_request); - - grpc::string GetSerializedProtoFromMessageTypeTextFormat( + /// Converts a text or json string to its binary proto representation for the + /// given method's input or return type. + /// \param method the name of the method (does not need to be fully qualified name) + /// \param formatted_proto the text- or json-formatted proto string + /// \param is_request if \c true the resolved type is that of the input parameter of + /// the method, otherwise it is the output type + /// \param is_json_format if \c true the \c formatted_proto is treated as a + /// json-formatted proto, otherwise it is treated as a text-formatted proto + /// \return the serialised binary proto represenation of \c formatted_proto + grpc::string GetSerializedProtoFromMethod( + const grpc::string& method, const grpc::string& formatted_proto, + bool is_request, bool is_json_format); + + /// Converts a text or json string to its proto representation for the given + /// message type. + /// \param formatted_proto the text- or json-formatted proto string + /// \return the serialised binary proto represenation of \c formatted_proto + grpc::string GetSerializedProtoFromMessageType( const grpc::string& message_type_name, - const grpc::string& text_format_proto); - - grpc::string GetSerializedProtoFromMessageTypeJsonFormat( - const grpc::string& message_type_name, - const grpc::string& json_format_proto); - - grpc::string GetTextFormatFromMessageType( - const grpc::string& message_type_name, - const grpc::string& serialized_proto); + const grpc::string& formatted_proto, + bool is_json_format); + + /// Converts a binary proto string to its text or json string representation + /// for the given method's input or return type. + /// \param method the name of the method (does not need to be a fully qualified name) + /// \param the serialised binary proto representation of type \c message_type_name + /// \return the text- or json-formatted proto string of \c serialized_proto + grpc::string GetFormattedStringFromMethod(const grpc::string& method, + const grpc::string& serialized_proto, + bool is_request, bool is_json_format); - grpc::string GetJsonFormatFromMessageType( + /// Converts a binary proto string to its text or json string representation + /// for the given message type. + /// \param the serialised binary proto representation of type \c message_type_name + /// \return the text- or json-formatted proto string of \c serialized_proto + grpc::string GetFormattedStringFromMessageType( const grpc::string& message_type_name, - const grpc::string& serialized_proto); + const grpc::string& serialized_proto, + bool is_json_format); bool IsStreaming(const grpc::string& method, bool is_request);