patch svn r34953

Change-Id: I5cf6ae18d3cf0826da2d5d4efa2939e0d6e7d6b7

brpc/policy/http_rpc_protocol.cpp

@@ -390,7 +390,8 @@ void SerializeHttpRequest(base::IOBuf* /*not used*/,
             // Serialize content as json
             std::string err;
             json2pb::Pb2JsonOptions opt;
-            opt.enum_option = (FLAGS_pb_enum_as_number ? json2pb::OUTPUT_ENUM_BY_NUMBER
+            opt.enum_option = (FLAGS_pb_enum_as_number
+                               ? json2pb::OUTPUT_ENUM_BY_NUMBER
                                : json2pb::OUTPUT_ENUM_BY_NAME);
             if (!json2pb::ProtoMessageToJson(*request, &wrapper, opt, &err)) {
                 cntl->request_attachment().clear();
@@ -574,49 +575,43 @@ static void SendHttpResponse(Controller *cntl,
     // Notice: Not check res->IsInitialized() which should be checked in the
     // conversion function.
     if (res != NULL &&
+        cntl->response_attachment().empty() &&
+        // ^ user did not fill the body yet.
         res->GetDescriptor()->field_count() > 0 &&
-        // ^ a pb service must have fields in response.
+        // ^ a pb service
         !cntl->Failed()) {
         // ^ pb response in failed RPC is undefined, no need to convert.
-
-        if (!cntl->response_attachment().empty()) {
-            if (res->ByteSize() != 0) { // fields in `res' were set.
-                LOG(ERROR) << "Service on " << req_header->uri().path()
-                           << " sets both response_attachment and response(pb)"
-                    ", you can set only one of them.";
-            } // else no fields in `res' were set, user is intended to fill
-            // the http body by him/herself.
-        } else {
-            base::IOBufAsZeroCopyOutputStream wrapper(&cntl->response_attachment());
-            const std::string* content_type_str = &res_header->content_type();
-            if (content_type_str->empty()) {
-                content_type_str = &req_header->content_type();
-            }
-            const HttpContentType content_type = ParseContentType(*content_type_str);
-            if (content_type == HTTP_CONTENT_PROTO) {
-                if (res->SerializeToZeroCopyStream(&wrapper)) {
-                    // Set content-type if user did not
-                    if (res_header->content_type().empty()) {
-                        res_header->set_content_type(common->CONTENT_TYPE_PROTO);
-                    }
-                } else {
-                    cntl->SetFailed(ERESPONSE, "Fail to serialize %s",
-                                    res->GetTypeName().c_str());
+        
+        base::IOBufAsZeroCopyOutputStream wrapper(&cntl->response_attachment());
+        const std::string* content_type_str = &res_header->content_type();
+        if (content_type_str->empty()) {
+            content_type_str = &req_header->content_type();
+        }
+        const HttpContentType content_type = ParseContentType(*content_type_str);
+        if (content_type == HTTP_CONTENT_PROTO) {
+            if (res->SerializeToZeroCopyStream(&wrapper)) {
+                // Set content-type if user did not
+                if (res_header->content_type().empty()) {
+                    res_header->set_content_type(common->CONTENT_TYPE_PROTO);
                 }
             } else {
-                std::string err;
-                json2pb::Pb2JsonOptions opt;
-                opt.enum_option = (FLAGS_pb_enum_as_number ? json2pb::OUTPUT_ENUM_BY_NUMBER
-                                   : json2pb::OUTPUT_ENUM_BY_NAME);
-                if (json2pb::ProtoMessageToJson(*res, &wrapper, opt, &err)) {
-                    // Set content-type if user did not
-                    if (res_header->content_type().empty()) {
-                        res_header->set_content_type(common->CONTENT_TYPE_JSON);
-                    }
-                } else {
-                    cntl->SetFailed(ERESPONSE, "Fail to convert response to json, %s",
-                                    err.c_str());
+                cntl->SetFailed(ERESPONSE, "Fail to serialize %s",
+                                res->GetTypeName().c_str());
+            }
+        } else {
+            std::string err;
+            json2pb::Pb2JsonOptions opt;
+            opt.enum_option = (FLAGS_pb_enum_as_number
+                               ? json2pb::OUTPUT_ENUM_BY_NUMBER
+                               : json2pb::OUTPUT_ENUM_BY_NAME);
+            if (json2pb::ProtoMessageToJson(*res, &wrapper, opt, &err)) {
+                // Set content-type if user did not
+                if (res_header->content_type().empty()) {
+                    res_header->set_content_type(common->CONTENT_TYPE_JSON);
                 }
+            } else {
+                cntl->SetFailed(ERESPONSE, "Fail to convert response to json, %s",
+                                err.c_str());
             }
         }
     }
@@ -1220,7 +1215,8 @@ void ProcessHttpRequest(InputMessageBase *msg) {
         cntl->SetFailed("Fail to new req or res");
         return SendHttpResponse(cntl.release(), server, method_status);
     }
-    if (method->input_type()->field_count() > 0) {
+    if (sp->allow_http_body_to_pb &&
+        method->input_type()->field_count() > 0) {
         // A protobuf service. No matter if Content-type is set to
         // applcation/json or body is empty, we have to treat body as a json
         // and try to convert it to pb, which guarantees that a protobuf

brpc/restful.cpp

@@ -247,6 +247,7 @@ RestfulMap::~RestfulMap() {
 bool RestfulMap::AddMethod(const RestfulMethodPath& path,
                            google::protobuf::Service* service,
                            bool is_tabbed,
+                           bool allow_http_body_to_pb,
                            const std::string& method_name,
                            MethodStatus* status) {
     if (service == NULL) {
@@ -278,6 +279,7 @@ bool RestfulMap::AddMethod(const RestfulMethodPath& path,
     info.is_builtin_service = false;
     info.own_method_status = false;
     info.is_tabbed = is_tabbed;
+    info.allow_http_body_to_pb = allow_http_body_to_pb;
     info.service = service;
     info.method = md;
     info.status = status;

brpc/restful.h

@@ -61,6 +61,7 @@ public:
     bool AddMethod(const RestfulMethodPath& path,
                    google::protobuf::Service* service,
                    bool is_tabbed,
+                   bool allow_http_body_to_pb,
                    const std::string& method_name,
                    MethodStatus* status);
 

brpc/server.cpp

@@ -140,6 +140,7 @@ Server::MethodProperty::MethodProperty()
     : is_builtin_service(false)
     , own_method_status(false)
     , is_tabbed(false)
+    , allow_http_body_to_pb(true)
     , http_url(NULL)
     , service(NULL)
     , method(NULL)
@@ -1061,9 +1062,8 @@ int Server::Join() {
 }
 
 int Server::AddServiceInternal(google::protobuf::Service* service,
-                               ServiceOwnership ownership,
                                bool is_builtin_service,
-                               base::StringPiece restful_mappings) {
+                               const ServiceOptions& svc_opt) {
     if (NULL == service) {
         LOG(ERROR) << "Parameter[service] is NULL!";
         return -1;
@@ -1108,6 +1108,7 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
         mp.is_builtin_service = is_builtin_service;
         mp.own_method_status = true;
         mp.is_tabbed = !!tabbed;
+        mp.allow_http_body_to_pb = svc_opt.allow_http_body_to_pb;
         mp.service = service;
         mp.method = md;
         mp.status = new MethodStatus;
@@ -1132,7 +1133,8 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
         }
     }
 
-    const ServiceProperty ss = { is_builtin_service, ownership, service, NULL };
+    const ServiceProperty ss = {
+        is_builtin_service, svc_opt.ownership, service, NULL };
     _fullname_service_map[sd->full_name()] = ss;
     _service_map[sd->name()] = ss;
     if (is_builtin_service) {
@@ -1142,7 +1144,8 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
             _first_service = service;
         }
     }
-    
+
+    base::StringPiece restful_mappings = svc_opt.restful_mappings;
     restful_mappings.trim_spaces();
     if (!restful_mappings.empty()) {
         // Parse the mappings.
@@ -1190,6 +1193,7 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
                 }
                 if (!_global_restful_map->AddMethod(
                         mappings[i].path, service, !!tabbed,
+                        svc_opt.allow_http_body_to_pb,
                         mappings[i].method_name, mp->status)) {
                     LOG(ERROR) << "Fail to map `" << mappings[i].path
                                << "' to `" << full_method_name << '\'';
@@ -1222,6 +1226,7 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
                 m = sp->restful_map;
             }
             if (!m->AddMethod(mappings[i].path, service, !!tabbed,
+                              svc_opt.allow_http_body_to_pb,
                               mappings[i].method_name, mp->status)) {
                 LOG(ERROR) << "Fail to map `" << mappings[i].path << "' to `"
                            << sd->full_name() << '.' << mappings[i].method_name
@@ -1271,16 +1276,37 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
     return 0;
 }
 
+ServiceOptions::ServiceOptions()
+    : ownership(SERVER_DOESNT_OWN_SERVICE)
+    , allow_http_body_to_pb(true) {
+}
+
 int Server::AddService(google::protobuf::Service* service,
                        ServiceOwnership ownership) {
-    return AddServiceInternal(service, ownership, false/*non-builtin*/, "");
+    ServiceOptions options;
+    options.ownership = ownership;
+    return AddServiceInternal(service, false, options);
 }
 
 int Server::AddService(google::protobuf::Service* service,
                        ServiceOwnership ownership,
                        const base::StringPiece& restful_mappings) {
-    return AddServiceInternal(service, ownership, false/*non-builtin*/,
-                              restful_mappings);
+    ServiceOptions options;
+    options.ownership = ownership;
+    // TODO: This is weird
+    options.restful_mappings = restful_mappings.as_string();
+    return AddServiceInternal(service, false, options);
+}
+
+int Server::AddService(google::protobuf::Service* service,
+                       const ServiceOptions& options) {
+    return AddServiceInternal(service, false, options);
+}
+
+int Server::AddBuiltinService(google::protobuf::Service* service) {
+    ServiceOptions options;
+    options.ownership = SERVER_OWNS_SERVICE;
+    return AddServiceInternal(service, true, options);
 }
 
 void Server::RemoveMethodsOf(google::protobuf::Service* service) {

brpc/server.h

@@ -295,6 +295,34 @@ enum ServiceOwnership {
     SERVER_DOESNT_OWN_SERVICE
 };
 
+struct ServiceOptions {
+    ServiceOptions(); // constructed with default options.
+
+    // SERVER_OWNS_SERVICE: the service will be deleted by the server.
+    // SERVER_DOESNT_OWN_SERVICE: the service shall be deleted by user after
+    // stopping the server.
+    // Default: SERVER_DOESNT_OWN_SERVICE
+    ServiceOwnership ownership;
+    
+    // If this option is non-empty, methods in the service will be exposed
+    // on specified paths instead of default "/SERVICE/METHOD".
+    // Mappings are in form of: "PATH1 => NAME1, PATH2 => NAME2 ..." where
+    // PATHs are valid http paths, NAMEs are method names in the service.
+    // Default: empty
+    std::string restful_mappings;
+
+    // [ Not recommended to change this option ]
+    // If this flag is true, the service will convert http body to protobuf
+    // when the pb schema is non-empty in http servings. The body must be
+    // valid json or protobuf(wire-format) otherwise the request is rejected.
+    // This option does not affect pure-http services (pb schema is empty).
+    // Services that use older versions of baidu-rpc may need to turn this
+    // conversion off and handle http requests by their own to keep compatible
+    // with existing clients.
+    // Default: true
+    bool allow_http_body_to_pb;
+};
+
 // Represent ports inside [min_port, max_port]
 struct PortRange {
     int min_port;
@@ -336,6 +364,7 @@ public:
         bool is_builtin_service;
         bool own_method_status;
         bool is_tabbed;
+        bool allow_http_body_to_pb;
         // NULL if service of the method was never added as restful.
         // "@path1 @path2 ..." if the method was mapped from paths.
         std::string* http_url;
@@ -402,14 +431,7 @@ public:
     // function may block indefinitely.
     void RunUntilAskedToQuit();
 
-    // Add a service.
-    // If `ownership' is SERVER_OWNS_SERVICE, the service will be deleted along
-    // with this server, otherwise user shall delete the service after stopping
-    // this server.
-    // If `restful_mappings' is not empty, the methods in the service will be
-    // exposed on the specified paths instead of fixed "/SERVICE/METHOD". The
-    // mapping should be in form of: "PATH1 => NAME1, PATH2 => NAME2 ..." where
-    // PATHs are valid http URI paths, NAMEs are method names in the service.
+    // Add a service. Arguments are explained in ServiceOptions above.
     // NOTE: Adding a service while server is running is forbidden.
     // Returns 0 on success, -1 otherwise.
     int AddService(google::protobuf::Service* service,
@@ -417,6 +439,8 @@ public:
     int AddService(google::protobuf::Service* service,
                    ServiceOwnership ownership,
                    const base::StringPiece& restful_mappings);
+    int AddService(google::protobuf::Service* service,
+                   const ServiceOptions& options);
 
     // Remove a service from this server.
     // NOTE: removing a service while server is running is forbidden.
@@ -533,13 +557,10 @@ friend class ServerPrivateAccessor;
 friend class Controller;
 
     int AddServiceInternal(google::protobuf::Service* service,
-                           ServiceOwnership ownership,
                            bool is_builtin_service,
-                           base::StringPiece restful_mappings);
+                           const ServiceOptions& options);
 
-    int AddBuiltinService(google::protobuf::Service* service) {
-        return AddServiceInternal(service, SERVER_OWNS_SERVICE, true, "");
-    }
+    int AddBuiltinService(google::protobuf::Service* service);
 
     // Remove all methods of `service' from internal structures.
     void RemoveMethodsOf(google::protobuf::Service* service);

test/brpc_server_unittest.cpp

@@ -130,7 +130,7 @@ protected:
         brpc::Server server;
         EvilService evil(conflict_sd);
         EXPECT_EQ(0, server.AddServiceInternal(
-                      &evil, brpc::SERVER_DOESNT_OWN_SERVICE, false, ""));
+                      &evil, false, brpc::ServiceOptions()));
         EXPECT_EQ(-1, server.AddBuiltinServices());
     }
 };
@@ -206,12 +206,18 @@ public:
                     , ncalled_echo5(0)
     {}
     virtual ~EchoServiceV1() {}
-    virtual void Echo(google::protobuf::RpcController*,
+    virtual void Echo(google::protobuf::RpcController* cntl_base,
                       const v1::EchoRequest* request,
                       v1::EchoResponse* response,
                       google::protobuf::Closure* done) {
+        brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
         brpc::ClosureGuard done_guard(done);
-        response->set_message(request->message() + "_v1");
+        if (request->has_message()) {
+            response->set_message(request->message() + "_v1");
+        } else {
+            CHECK_EQ(brpc::PROTOCOL_HTTP, cntl->request_protocol());
+            cntl->response_attachment() = cntl->request_attachment();
+        }
         ncalled.fetch_add(1);
     }
     virtual void Echo2(google::protobuf::RpcController*,
@@ -441,6 +447,76 @@ TEST_F(ServerTest, various_forms_of_uri_paths) {
     server1.Join();
 }
 
+TEST_F(ServerTest, missing_required_fields) {
+    const int port = 9200;
+    brpc::Server server1;
+    EchoServiceV1 service_v1;
+    ASSERT_EQ(0, server1.AddService(&service_v1, brpc::SERVER_DOESNT_OWN_SERVICE));
+    ASSERT_EQ(0, server1.Start(port, NULL));
+    brpc::Channel http_channel;
+    brpc::ChannelOptions chan_options;
+    chan_options.protocol = "http";
+    ASSERT_EQ(0, http_channel.Init("0.0.0.0", port, &chan_options));
+    brpc::Controller cntl;
+    cntl.http_request().uri() = "/EchoService/Echo";
+    http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+    ASSERT_TRUE(cntl.Failed());
+    ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
+    ASSERT_EQ(brpc::HTTP_STATUS_BAD_REQUEST, cntl.http_response().status_code());
+    ASSERT_EQ(0, service_v1.ncalled.load());
+
+    cntl.Reset();
+    cntl.http_request().uri() = "/EchoService/Echo";
+    cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
+    http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+    ASSERT_TRUE(cntl.Failed());
+    ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
+    ASSERT_EQ(brpc::HTTP_STATUS_BAD_REQUEST, cntl.http_response().status_code());
+    ASSERT_EQ(0, service_v1.ncalled.load());
+
+    cntl.Reset();
+    cntl.http_request().uri() = "/EchoService/Echo";
+    cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
+    cntl.request_attachment().append("{\"message2\":\"foo\"}");
+    http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+    ASSERT_TRUE(cntl.Failed());
+    ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
+    ASSERT_EQ(brpc::HTTP_STATUS_BAD_REQUEST, cntl.http_response().status_code());
+    ASSERT_EQ(0, service_v1.ncalled.load());
+}
+
+TEST_F(ServerTest, disallow_http_body_to_pb) {
+    const int port = 9200;
+    brpc::Server server1;
+    EchoServiceV1 service_v1;
+    brpc::ServiceOptions svc_opt;
+    svc_opt.allow_http_body_to_pb = false;
+    svc_opt.restful_mappings = "/access_echo1=>Echo";
+    ASSERT_EQ(0, server1.AddService(&service_v1, svc_opt));
+    ASSERT_EQ(0, server1.Start(port, NULL));
+    brpc::Channel http_channel;
+    brpc::ChannelOptions chan_options;
+    chan_options.protocol = "http";
+    ASSERT_EQ(0, http_channel.Init("0.0.0.0", port, &chan_options));
+    brpc::Controller cntl;
+    cntl.http_request().uri() = "/access_echo1";
+    http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+    ASSERT_TRUE(cntl.Failed());
+    ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
+    ASSERT_EQ(brpc::HTTP_STATUS_INTERNAL_SERVER_ERROR,
+              cntl.http_response().status_code());
+    ASSERT_EQ(1, service_v1.ncalled.load());
+
+    cntl.Reset();
+    cntl.http_request().uri() = "/access_echo1";
+    cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
+    cntl.request_attachment().append("heheda");
+    http_channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+    ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
+    ASSERT_EQ("heheda", cntl.response_attachment());
+    ASSERT_EQ(2, service_v1.ncalled.load());
+}
+
 TEST_F(ServerTest, restful_mapping) {
     const int port = 9200;
     EchoServiceV1 service_v1;