Merge pull request #43 from euroelessar/fix-verb-support

Fix verb support

Author: qiwzhang

src/include/grpc_transcoding/path_matcher.h

@@ -332,7 +332,8 @@ void ExtractBindingsFromQueryParameters(
 //
 // - Strips off query string: "/a?foo=bar" --> "/a"
 // - Collapses extra slashes: "///" --> "/"
-std::vector<std::string> ExtractRequestParts(std::string path) {
+std::vector<std::string> ExtractRequestParts(
+    std::string path, const std::unordered_set<std::string>& custom_verbs) {
   // Remove query parameters.
   path = path.substr(0, path.find_first_of('?'));
 
@@ -341,7 +342,12 @@ std::vector<std::string> ExtractRequestParts(std::string path) {
   std::size_t last_colon_pos = path.find_last_of(':');
   std::size_t last_slash_pos = path.find_last_of('/');
   if (last_colon_pos != std::string::npos && last_colon_pos > last_slash_pos) {
-    path[last_colon_pos] = '/';
+    std::string verb = path.substr(last_colon_pos + 1);
+    // only verb in the configured custom verbs, treat it as verb
+    // replace ":" with / as a separate segment.
+    if (custom_verbs.find(verb) != custom_verbs.end()) {
+      path[last_colon_pos] = '/';
+    }
   }
 
   std::vector<std::string> result;
@@ -400,7 +406,8 @@ Method PathMatcher<Method>::Lookup(
     const std::string& query_params,
     std::vector<VariableBinding>* variable_bindings,
     std::string* body_field_path) const {
-  const std::vector<std::string> parts = ExtractRequestParts(path);
+  const std::vector<std::string> parts =
+      ExtractRequestParts(path, custom_verbs_);
 
   // If service_name has not been registered to ESP and strict_service_matching_
   // is set to false, tries to lookup the method in all registered services.
@@ -432,7 +439,8 @@ Method PathMatcher<Method>::Lookup(
 template <class Method>
 Method PathMatcher<Method>::Lookup(const std::string& http_method,
                                    const std::string& path) const {
-  const std::vector<std::string> parts = ExtractRequestParts(path);
+  const std::vector<std::string> parts =
+      ExtractRequestParts(path, custom_verbs_);
 
   // If service_name has not been registered to ESP and strict_service_matching_
   // is set to false, tries to lookup the method in all registered services.
@@ -500,6 +508,9 @@ bool PathMatcherBuilder<Method>::Register(
                    root_ptr_.get());
   // Add the method_data to the methods_ vector for cleanup
   methods_.emplace_back(std::move(method_data));
+  if (!ht->verb().empty()) {
+    custom_verbs_.insert(ht->verb());
+  }
   return true;
 }
 

test/path_matcher_test.cc

@@ -523,19 +523,24 @@ TEST_F(PathMatcherTest, CustomVerbMatches) {
 }
 
 TEST_F(PathMatcherTest, CustomVerbMatch2) {
-  MethodInfo* verb = AddGetPath("/*/*:verb");
+  MethodInfo* verb = AddGetPath("/{a=*}/{b=*}:verb");
   Build();
-  EXPECT_EQ(LookupNoBindings("GET", "/some:verb/const:verb"), verb);
+  Bindings bindings;
+  EXPECT_EQ(Lookup("GET", "/some:verb/const:verb", &bindings), verb);
+  EXPECT_EQ(bindings.size(), 2);
+  EXPECT_EQ(bindings[0].value, "some:verb");
+  EXPECT_EQ(bindings[1].value, "const");
 }
 
 TEST_F(PathMatcherTest, CustomVerbMatch3) {
-  EXPECT_NE(nullptr, AddGetPath("/foo/*"));
+  MethodInfo* verb = AddGetPath("/foo/{a=*}");
   Build();
 
-  // This should match. But due to an implementation bug which
-  // blinkdly replacing last : with /, it will use /foo/other/verb
-  // to match /foo/* which will fail.
-  EXPECT_EQ(LookupNoBindings("GET", "/foo/other:verb"), nullptr);
+  // This is not custom verb since it was not configured.
+  Bindings bindings;
+  EXPECT_EQ(Lookup("GET", "/foo/other:verb", &bindings), verb);
+  EXPECT_EQ(bindings.size(), 1);
+  EXPECT_EQ(bindings[0].value, "other:verb");
 }
 
 TEST_F(PathMatcherTest, CustomVerbMatch4) {
@@ -548,6 +553,24 @@ TEST_F(PathMatcherTest, CustomVerbMatch4) {
   EXPECT_EQ(LookupNoBindings("GET", "/foo/other:verb/hello"), a);
 }
 
+TEST_F(PathMatcherTest, CustomVerbMatch5) {
+  MethodInfo* verb = AddGetPath("/{a=**}:verb");
+  MethodInfo* non_verb = AddGetPath("/{a=**}");
+  Build();
+  Bindings bindings;
+  EXPECT_EQ(Lookup("GET", "/some:verb/const:verb", &bindings), verb);
+  EXPECT_EQ(bindings.size(), 1);
+  EXPECT_EQ(bindings[0].value, "some:verb/const");
+  bindings.clear();
+  EXPECT_EQ(Lookup("GET", "/some:verb/const", &bindings), non_verb);
+  EXPECT_EQ(bindings.size(), 1);
+  EXPECT_EQ(bindings[0].value, "some:verb/const");
+  bindings.clear();
+  EXPECT_EQ(Lookup("GET", "/some:verb2/const:verb2", &bindings), non_verb);
+  EXPECT_EQ(bindings.size(), 1);
+  EXPECT_EQ(bindings[0].value, "some:verb2/const:verb2");
+}
+
 TEST_F(PathMatcherTest, RejectPartialMatches) {
   MethodInfo* prefix_middle_suffix = AddGetPath("/prefix/middle/suffix");
   MethodInfo* prefix_middle = AddGetPath("/prefix/middle");