Allow multiple HTTP rules for the same RPC method

Move HTTP body prefix of the HTTP rule from method to the path matcher
node to fix this. Having the HTTP body prefix in the method results in
incorrect json=>proto translation when there are multiple rules for the
same method with different body prefixes.

Change-Id: Iee539c4d01395cda0127d749ba771f1459386daa

src/api_manager/config.cc

@@ -143,9 +143,8 @@ bool Config::LoadHttpMethods(ApiManagerEnvInterface *env,
     }
 
     MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(selector);
-    mi->set_body_field_path(rule.body());
 
-    if (!pmb->Register(service_.name(), http_method, *url, mi)) {
+    if (!pmb->Register(service_.name(), http_method, *url, rule.body(), mi)) {
       string error("Invalid HTTP template: ");
       error += *url;
       env->LogError(error.c_str());
@@ -187,7 +186,7 @@ bool Config::AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env,
   mi->set_allow_unregistered_calls(true);
 
   for (auto url : all_urls) {
-    if (!pmb->Register(service_.name(), http_options, url, mi)) {
+    if (!pmb->Register(service_.name(), http_options, url, std::string(), mi)) {
       env->LogError(
           std::string("Failed to add http options template for url: " + url));
     }
@@ -227,7 +226,7 @@ bool Config::LoadRpcMethods(ApiManagerEnvInterface *env,
       mi->set_response_type_url(method.response_type_url());
       mi->set_response_streaming(method.response_streaming());
 
-      if (!pmb->Register(service_.name(), http_post, path, mi)) {
+      if (!pmb->Register(service_.name(), http_post, path, std::string(), mi)) {
         string error("Invalid method: ");
         error += selector;
         env->LogError(error.c_str());
@@ -431,7 +430,8 @@ MethodCallInfo Config::GetMethodCallInfo(const std::string &http_method,
     call_info.method_info = nullptr;
   } else {
     call_info.method_info = static_cast<MethodInfo *>(path_matcher_->Lookup(
-        service_.name(), http_method, url, &call_info.variable_bindings));
+        service_.name(), http_method, url, &call_info.variable_bindings,
+        &call_info.body_field_path));
   }
   return call_info;
 }

src/api_manager/config_test.cc

@@ -559,7 +559,6 @@ TEST(Config, RpcMethodsWithHttpRules) {
   EXPECT_EQ("types.googleapis.com/Bookstore.ListShelvesResponse",
             list_shelves->response_type_url());
   EXPECT_EQ(false, list_shelves->response_streaming());
-  EXPECT_EQ("", list_shelves->body_field_path());
 
   const MethodInfo *create_shelves =
       config->GetMethodInfo("POST", "/Bookstore/CreateShelves");
@@ -572,7 +571,6 @@ TEST(Config, RpcMethodsWithHttpRules) {
   EXPECT_EQ("types.googleapis.com/Bookstore.Shelf",
             create_shelves->response_type_url());
   EXPECT_EQ(true, create_shelves->response_streaming());
-  EXPECT_EQ("", create_shelves->body_field_path());
 
   // Matching through http rule path must yield the same method
   EXPECT_EQ(list_shelves, config->GetMethodInfo("GET", "/shelves"));
@@ -617,6 +615,11 @@ TEST(Config, RpcMethodsWithHttpRulesAndVariableBindings) {
           post: "/shelves/{shelf=*}/books"
           body: "book"
         }
+        rules {
+          selector: "Bookstore.CreateBook"
+          post: "/shelves/{shelf=*}/books/{book.id}/{book.author}"
+          body: "book.title"
+        }
       }
     )";
 
@@ -634,7 +637,7 @@ TEST(Config, RpcMethodsWithHttpRulesAndVariableBindings) {
   EXPECT_EQ("types.googleapis.com/Bookstore.ListShelvesResponse",
             list_shelves.method_info->response_type_url());
   EXPECT_EQ(false, list_shelves.method_info->response_streaming());
-  EXPECT_EQ("", list_shelves.method_info->body_field_path());
+  EXPECT_EQ("", list_shelves.body_field_path);
   EXPECT_EQ(0, list_shelves.variable_bindings.size());
 
   MethodCallInfo list_books =
@@ -649,7 +652,7 @@ TEST(Config, RpcMethodsWithHttpRulesAndVariableBindings) {
   EXPECT_EQ("types.googleapis.com/Bookstore.ListBooksResponse",
             list_books.method_info->response_type_url());
   EXPECT_EQ(false, list_books.method_info->response_streaming());
-  EXPECT_EQ("", list_books.method_info->body_field_path());
+  EXPECT_EQ("", list_books.body_field_path);
   ASSERT_EQ(1, list_books.variable_bindings.size());
   EXPECT_EQ(std::vector<std::string>(1, "shelf"),
             list_books.variable_bindings[0].field_path);
@@ -667,11 +670,38 @@ TEST(Config, RpcMethodsWithHttpRulesAndVariableBindings) {
   EXPECT_EQ("types.googleapis.com/Bookstore.Book",
             create_book.method_info->response_type_url());
   EXPECT_EQ(false, create_book.method_info->response_streaming());
-  EXPECT_EQ("book", create_book.method_info->body_field_path());
+  EXPECT_EQ("book", create_book.body_field_path);
   ASSERT_EQ(1, create_book.variable_bindings.size());
   EXPECT_EQ(std::vector<std::string>(1, "shelf"),
             create_book.variable_bindings[0].field_path);
   EXPECT_EQ("99", create_book.variable_bindings[0].value);
+
+  MethodCallInfo create_book_1 =
+      config->GetMethodCallInfo("POST", "/shelves/77/books/88/auth");
+
+  ASSERT_NE(nullptr, create_book_1.method_info);
+  EXPECT_EQ("/Bookstore/CreateBook",
+            create_book_1.method_info->rpc_method_full_name());
+  EXPECT_EQ("types.googleapis.com/Bookstore.CreateBookRequest",
+            create_book_1.method_info->request_type_url());
+  EXPECT_EQ(false, create_book_1.method_info->request_streaming());
+  EXPECT_EQ("types.googleapis.com/Bookstore.Book",
+            create_book_1.method_info->response_type_url());
+  EXPECT_EQ(false, create_book_1.method_info->response_streaming());
+  EXPECT_EQ("book.title", create_book_1.body_field_path);
+  ASSERT_EQ(3, create_book_1.variable_bindings.size());
+
+  EXPECT_EQ(std::vector<std::string>(1, "shelf"),
+            create_book_1.variable_bindings[0].field_path);
+  EXPECT_EQ("77", create_book_1.variable_bindings[0].value);
+
+  EXPECT_EQ((std::vector<std::string>{"book", "id"}),
+            create_book_1.variable_bindings[1].field_path);
+  EXPECT_EQ("88", create_book_1.variable_bindings[1].value);
+
+  EXPECT_EQ((std::vector<std::string>{"book", "author"}),
+            create_book_1.variable_bindings[2].field_path);
+  EXPECT_EQ("auth", create_book_1.variable_bindings[2].value);
 }
 
 TEST(Config, TestHttpOptions) {

src/api_manager/method.h

@@ -88,9 +88,6 @@ class MethodInfo {
 
   // Get whether response is streaming
   virtual bool response_streaming() const = 0;
-
-  // The field in the request message to be mapped to the HTTP body
-  virtual const std::string &body_field_path() const = 0;
 };
 
 }  // namespace api_manager

src/api_manager/method_call_info.h

@@ -54,6 +54,8 @@ struct MethodCallInfo {
   const MethodInfo* method_info;
   // Variable bindings
   std::vector<VariableBinding> variable_bindings;
+  // Body prefix (the field of the message where the HTTP body should go)
+  std::string body_field_path;
 };
 
 }  // namespace api_manager

src/api_manager/method_impl.h

@@ -81,8 +81,6 @@ class MethodInfoImpl : public MethodInfo {
 
   bool response_streaming() const { return response_streaming_; }
 
-  const std::string &body_field_path() const { return body_field_path_; }
-
   // Adds allowed audiences (comma delimated, no space) for the issuer.
   // audiences_list can be empty.
   void addAudiencesForIssuer(const std::string &issuer,
@@ -127,10 +125,6 @@ class MethodInfoImpl : public MethodInfo {
     response_streaming_ = response_streaming;
   }
 
-  void set_body_field_path(std::string body_field_path) {
-    body_field_path_ = std::move(body_field_path);
-  }
-
  private:
   // Method name.
   std::string name_;
@@ -170,9 +164,6 @@ class MethodInfoImpl : public MethodInfo {
 
   // Whether the response is streaming or not.
   bool response_streaming_;
-
-  // The field in the request message to be mapped to the HTTP body
-  std::string body_field_path_;
 };
 
 typedef std::unique_ptr<MethodInfoImpl> MethodInfoImplPtr;

src/api_manager/path_matcher.cc

@@ -237,9 +237,10 @@ PathMatcher::~PathMatcher() { utils::STLDeleteValues(&root_ptr_map_); }
 // values parsed from the path.
 // TODO: cache results by adding get/put methods here (if profiling reveals
 // benefit)
-void* PathMatcher::Lookup(
-    const string& service_name, const string& http_method, const string& path,
-    std::vector<VariableBinding>* variable_bindings) const {
+void* PathMatcher::Lookup(const string& service_name, const string& http_method,
+                          const string& path,
+                          std::vector<VariableBinding>* variable_bindings,
+                          std::string* body_field_path) const {
   const vector<string> parts = ExtractRequestParts(path);
 
   PathMatcherNode* root_ptr = utils::FindPtrOrNull(root_ptr_map_, service_name);
@@ -270,12 +271,15 @@ void* PathMatcher::Lookup(
   if (variable_bindings != nullptr) {
     ExtractVariableBindings(method->variables, parts, variable_bindings);
   }
+  if (body_field_path != nullptr) {
+    *body_field_path = method->body_field_path;
+  }
   return method->data;
 }
 
 void* PathMatcher::Lookup(const string& service_name, const string& http_method,
                           const string& path) const {
-  return Lookup(service_name, http_method, path, nullptr);
+  return Lookup(service_name, http_method, path, nullptr, nullptr);
 }
 
 // Initializes the builder with a root Path Segment
@@ -309,7 +313,8 @@ void PathMatcherBuilder::InsertPathToNode(const PathMatcherNode::PathInfo& path,
 // This wrapper converts the |http_rule| into a HttpTemplate. Then, inserts the
 // template into the trie.
 bool PathMatcherBuilder::Register(string service_name, string http_method,
-                                  string http_template, void* method_data) {
+                                  string http_template, string body_field_path,
+                                  void* method_data) {
   std::unique_ptr<HttpTemplate> ht(HttpTemplate::Parse(http_template));
   if (nullptr == ht) {
     return false;
@@ -323,6 +328,7 @@ bool PathMatcherBuilder::Register(string service_name, string http_method,
   auto method_info = std::unique_ptr<MethodInfo>(new MethodInfo());
   method_info->data = method_data;
   method_info->variables = std::move(ht->Variables());
+  method_info->body_field_path = std::move(body_field_path);
   // Don't mark batch methods as duplicates, since we insert them into each
   // service, and their graphs are all the same. We'll just use the first one
   // as the default. This allows batch requests on any service name to work.

src/api_manager/path_matcher.h

@@ -72,7 +72,8 @@ class PathMatcher {
 
   void* Lookup(const std::string& service_name, const std::string& http_method,
                const std::string& path,
-               std::vector<VariableBinding>* variable_bindings) const;
+               std::vector<VariableBinding>* variable_bindings,
+               std::string* body_field_path) const;
 
   void* Lookup(const std::string& service_name, const std::string& http_method,
                const std::string& path) const;
@@ -91,6 +92,7 @@ class PathMatcher {
   struct MethodInfo {
     void* data;
     std::vector<HttpTemplate::Variable> variables;
+    std::string body_field_path;
   };
   // The info associated with each method. The path matcher nodes
   // will hold pointers to MethodInfo objects in this vector.
@@ -117,7 +119,8 @@ class PathMatcherBuilder {
   // is stored.
   // Return false if path is an invalid http template.
   bool Register(std::string service_name, std::string http_method,
-                std::string path, void* method_data);
+                std::string path, std::string body_field_path,
+                void* method_data);
 
   // Returns a shared_ptr to a thread safe PathMatcher that contains all
   // registered path-WrapperGraph pairs.

src/api_manager/path_matcher_test.cc

@@ -79,27 +79,43 @@ class PathMatcherTest : public ::testing::Test {
   PathMatcherTest() : builder_(false) {}
   ~PathMatcherTest() { utils::STLDeleteElements(&stored_strings_); }
 
-  void* AddPath(std::string http_method, std::string http_template) {
+  void* AddPathWithBodyFieldPath(std::string http_method,
+                                 std::string http_template,
+                                 std::string body_field_path) {
     std::string* data = new std::string(http_method);
-    if (!builder_.Register("", http_method, http_template, data)) {
+    if (!builder_.Register("", http_method, http_template, body_field_path,
+                           data)) {
       delete data;
       return nullptr;
     }
     stored_strings_.push_back(data);
     return data;
   }
 
+  void* AddPath(std::string http_method, std::string http_template) {
+    return AddPathWithBodyFieldPath(http_method, http_template, std::string());
+  }
+
   void* AddGetPath(std::string path) { return AddPath("GET", path); }
 
   void Build() { matcher_ = builder_.Build(); }
 
+  void* LookupWithBodyFieldPath(std::string method, std::string path,
+                                Bindings* bindings,
+                                std::string* body_field_path) {
+    return matcher_->Lookup("", method, path, bindings, body_field_path);
+  }
+
   void* Lookup(std::string method, std::string path, Bindings* bindings) {
-    return matcher_->Lookup("", method, path, bindings);
+    std::string body_field_path;
+    return matcher_->Lookup("", method, path, bindings, &body_field_path);
   }
 
   void* LookupNoBindings(std::string method, std::string path) {
     Bindings bindings;
-    auto result = matcher_->Lookup("", method, path, &bindings);
+    std::string body_field_path;
+    auto result =
+        matcher_->Lookup("", method, path, &bindings, &body_field_path);
     EXPECT_EQ(0, bindings.size());
     return result;
   }
@@ -596,6 +612,20 @@ TEST_F(PathMatcherTest, DifferentHttpMethod) {
   EXPECT_EQ(LookupNoBindings("POST", "/a/b"), nullptr);
 }
 
+TEST_F(PathMatcherTest, BodyFieldPathTest) {
+  auto a = AddPathWithBodyFieldPath("GET", "/a", "b");
+  auto cd = AddPathWithBodyFieldPath("GET", "/c/d", "e.f.g");
+  Build();
+  EXPECT_NE(nullptr, a);
+  EXPECT_NE(nullptr, cd);
+  std::string body_field_path;
+  EXPECT_EQ(LookupWithBodyFieldPath("GET", "/a", nullptr, &body_field_path), a);
+  EXPECT_EQ("b", body_field_path);
+  EXPECT_EQ(LookupWithBodyFieldPath("GET", "/c/d", nullptr, &body_field_path),
+            cd);
+  EXPECT_EQ("e.f.g", body_field_path);
+}
+
 }  // namespace
 
 }  // namespace api_manager

src/api_manager/transcoding/transcoder_factory.cc

@@ -102,7 +102,7 @@ pbutil::Status MethodCallInfoToRequestInfo(TypeHelper* type_helper,
   }
 
   // Copy the body field path
-  request_info->body_field_path = call_info.method_info->body_field_path();
+  request_info->body_field_path = call_info.body_field_path;
 
   // Resolve the field paths of the bindings and add to the request_info
   for (const auto& unresolved_binding : call_info.variable_bindings) {

src/api_manager/transcoding/transcoder_test.cc

@@ -147,6 +147,7 @@ class TranscoderTest : public ::testing::Test {
     MethodCallInfo call_info;
     call_info.method_info = method_info_.get();
     call_info.variable_bindings = std::move(variable_bindings_);
+    call_info.body_field_path = method_info_->body_field_path();
 
     return transcoder_factory_->Create(call_info, request_input, response_input,
                                        transcoder);