Fix custom verb issue (#50)

qiwzhang · web-flow · commit 909368951e9a · 2021-02-23T16:14:01.000-08:00
* Fix custom verb issue

Signed-off-by: Wayne Zhang &lt;qiwzhang@google.com&gt;

* remove unused include

Signed-off-by: Wayne Zhang &lt;qiwzhang@google.com&gt;

* Add more lookup pattern in the new test

Signed-off-by: Wayne Zhang &lt;qiwzhang@google.com&gt;

* add a comment

Signed-off-by: Wayne Zhang &lt;qiwzhang@google.com&gt;
diff --git a/src/http_template.cc b/src/http_template.cc
@@ -335,12 +335,6 @@ class Parser {
         // this will allow the matcher code to reconstruct the variable
         // value based on the url segments.
         var.end_segment = (var.end_segment - segments_.size() - 1);
-
-        if (!verb_.empty()) {
-          // a custom verb will add an additional segment, so
-          // the end_postion needs a -1
-          --var.end_segment;
-        }
       }
     }
   }
diff --git a/src/include/grpc_transcoding/path_matcher.h b/src/include/grpc_transcoding/path_matcher.h
@@ -369,7 +369,8 @@ void ExtractBindingsFromQueryParameters(
 // - Strips off query string: "/a?foo=bar" --> "/a"
 // - Collapses extra slashes: "///" --> "/"
 std::vector<std::string> ExtractRequestParts(
-    std::string path, const std::unordered_set<std::string>& custom_verbs) {
+    std::string path, const std::unordered_set<std::string>& custom_verbs,
+    std::string& verb) {
   // Remove query parameters.
   path = path.substr(0, path.find_first_of('?'));
 
@@ -378,11 +379,11 @@ std::vector<std::string> ExtractRequestParts(
   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) {
-    std::string verb = path.substr(last_colon_pos + 1);
+    std::string tmp_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] = '/';
+    if (custom_verbs.find(tmp_verb) != custom_verbs.end()) {
+      verb = tmp_verb;
+      path = path.substr(0, last_colon_pos);
     }
   }
 
@@ -412,9 +413,6 @@ PathMatcherNode::PathInfo TransformHttpTemplate(const HttpTemplate& ht) {
   for (const std::string& part : ht.segments()) {
     builder.AppendLiteralNode(part);
   }
-  if (!ht.verb().empty()) {
-    builder.AppendLiteralNode(ht.verb());
-  }
 
   return builder.Build();
 }
@@ -443,8 +441,9 @@ Method PathMatcher<Method>::Lookup(
     const std::string& query_params,
     std::vector<VariableBinding>* variable_bindings,
     std::string* body_field_path) const {
+  std::string verb;
   const std::vector<std::string> parts =
-      ExtractRequestParts(path, custom_verbs_);
+      ExtractRequestParts(path, custom_verbs_, verb);
 
   // 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.
@@ -453,7 +452,7 @@ Method PathMatcher<Method>::Lookup(
   }
 
   PathMatcherLookupResult lookup_result =
-      LookupInPathMatcherNode(*root_ptr_, parts, http_method);
+      LookupInPathMatcherNode(*root_ptr_, parts, http_method + verb);
   // Return nullptr if nothing is found or the result is marked for duplication.
   if (lookup_result.data == nullptr || lookup_result.is_multiple) {
     return nullptr;
@@ -477,8 +476,9 @@ Method PathMatcher<Method>::Lookup(
 template <class Method>
 Method PathMatcher<Method>::Lookup(const std::string& http_method,
                                    const std::string& path) const {
+  std::string verb;
   const std::vector<std::string> parts =
-      ExtractRequestParts(path, custom_verbs_);
+      ExtractRequestParts(path, custom_verbs_, verb);
 
   // 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.
@@ -487,7 +487,7 @@ Method PathMatcher<Method>::Lookup(const std::string& http_method,
   }
 
   PathMatcherLookupResult lookup_result =
-      LookupInPathMatcherNode(*root_ptr_, parts, http_method);
+      LookupInPathMatcherNode(*root_ptr_, parts, http_method + verb);
   // Return nullptr if nothing is found or the result is marked for duplication.
   if (lookup_result.data == nullptr || lookup_result.is_multiple) {
     return nullptr;
@@ -540,7 +540,7 @@ bool PathMatcherBuilder<Method>::Register(
   method_data->body_field_path = body_field_path;
   method_data->system_query_parameter_names = system_query_parameter_names;
 
-  InsertPathToNode(path_info, method_data.get(), http_method, true,
+  InsertPathToNode(path_info, method_data.get(), http_method + ht->verb(), true,
                    root_ptr_.get());
   // Add the method_data to the methods_ vector for cleanup
   methods_.emplace_back(std::move(method_data));
diff --git a/test/http_template_test.cc b/test/http_template_test.cc
@@ -233,7 +233,7 @@ TEST(HttpTemplate, ParseTest16) {
   ASSERT_EQ(Segments({"a", "c", "**", "d", "e"}), ht->segments());
   ASSERT_EQ("verb", ht->verb());
   ASSERT_EQ(Variables({
-                Variable{1, -3, FieldPath{"b"}, true},
+                Variable{1, -2, FieldPath{"b"}, true},
             }),
             ht->Variables());
 }
@@ -399,7 +399,7 @@ TEST(HttpTemplate, VariableAndCustomVerbTests) {
   ASSERT_EQ(Segments({"**"}), ht->segments());
   ASSERT_EQ("myverb", ht->verb());
   ASSERT_EQ(Variables({
-                Variable{0, -2, FieldPath{"x"}, true},
+                Variable{0, -1, FieldPath{"x"}, true},
             }),
             ht->Variables());
 
@@ -409,7 +409,7 @@ TEST(HttpTemplate, VariableAndCustomVerbTests) {
   ASSERT_EQ(Segments({"**"}), ht->segments());
   ASSERT_EQ("myverb", ht->verb());
   ASSERT_EQ(Variables({
-                Variable{0, -2, FieldPath{"x", "y", "z"}, true},
+                Variable{0, -1, FieldPath{"x", "y", "z"}, true},
             }),
             ht->Variables());
 
@@ -419,7 +419,7 @@ TEST(HttpTemplate, VariableAndCustomVerbTests) {
   ASSERT_EQ(Segments({"a", "**", "b"}), ht->segments());
   ASSERT_EQ("custom", ht->verb());
   ASSERT_EQ(Variables({
-                Variable{0, -2, FieldPath{"x", "y", "z"}, true},
+                Variable{0, -1, FieldPath{"x", "y", "z"}, true},
             }),
             ht->Variables());
 
@@ -429,7 +429,7 @@ TEST(HttpTemplate, VariableAndCustomVerbTests) {
   ASSERT_EQ(Segments({"a", "**", "b", "c", "d"}), ht->segments());
   ASSERT_EQ("custom", ht->verb());
   ASSERT_EQ(Variables({
-                Variable{0, -4, FieldPath{"x", "y", "z"}, true},
+                Variable{0, -3, FieldPath{"x", "y", "z"}, true},
             }),
             ht->Variables());
 }
diff --git a/test/path_matcher_test.cc b/test/path_matcher_test.cc
@@ -53,6 +53,9 @@ bool operator==(const Binding& b1, const Binding& b2) {
   return b1.field_path == b2.field_path && b1.value == b2.value;
 }
 
+// These comment out code will be useful when debugging. It can be added as:
+//   std::cerr << Bingings;
+#if 0
 std::string FieldPathToString(const FieldPath& fp) {
   std::string s;
   for (const auto& f : fp) {
@@ -74,6 +77,7 @@ std::ostream& operator<<(std::ostream& os, const Bindings& bindings) {
   }
   return os;
 }
+#endif
 
 }  // namespace
 
@@ -418,6 +422,43 @@ TEST_F(PathMatcherTest,
   MultiSegmentMatchWithReservedCharactersBase("!#$&'()*+,/:;=?@[]");
 }
 
+TEST_F(PathMatcherTest, CustomVerbIssue) {
+  MethodInfo* list_person = AddGetPath("/person");
+  MethodInfo* get_person = AddGetPath("/person/{id=*}");
+  MethodInfo* verb = AddGetPath("/{x=**}:verb");
+  Build();
+
+  EXPECT_NE(nullptr, list_person);
+  EXPECT_NE(nullptr, get_person);
+  EXPECT_NE(nullptr, verb);
+
+  Bindings bindings;
+  // with the verb
+  EXPECT_EQ(Lookup("GET", "/person:verb", &bindings), verb);
+  EXPECT_EQ(Bindings({Binding{FieldPath{"x"}, "person"}}), bindings);
+  EXPECT_EQ(Lookup("GET", "/person/jason:verb", &bindings), verb);
+  EXPECT_EQ(Bindings({Binding{FieldPath{"x"}, "person/jason"}}), bindings);
+
+  // with the verb but with a different prefix
+  EXPECT_EQ(Lookup("GET", "/animal:verb", &bindings), verb);
+  EXPECT_EQ(Bindings({Binding{FieldPath{"x"}, "animal"}}), bindings);
+  EXPECT_EQ(Lookup("GET", "/animal/cat:verb", &bindings), verb);
+  EXPECT_EQ(Bindings({Binding{FieldPath{"x"}, "animal/cat"}}), bindings);
+
+  // without a verb
+  EXPECT_EQ(Lookup("GET", "/person", &bindings), list_person);
+  EXPECT_EQ(Lookup("GET", "/person/jason", &bindings), get_person);
+  EXPECT_EQ(Lookup("GET", "/animal", &bindings), nullptr);
+  EXPECT_EQ(Lookup("GET", "/animal/cat", &bindings), nullptr);
+
+  // with a non-verb
+  EXPECT_EQ(Lookup("GET", "/person:other", &bindings), nullptr);
+  EXPECT_EQ(Lookup("GET", "/person/jason:other", &bindings), get_person);
+  EXPECT_EQ(Bindings({Binding{FieldPath{"id"}, "jason:other"}}), bindings);
+  EXPECT_EQ(Lookup("GET", "/animal:other", &bindings), nullptr);
+  EXPECT_EQ(Lookup("GET", "/animal/cat:other", &bindings), nullptr);
+}
+
 TEST_F(PathMatcherTest, VariableBindingsWithCustomVerb) {
   MethodInfo* a_verb = AddGetPath("/a/{y=*}:verb");
   MethodInfo* ad__verb = AddGetPath("/a/{y=d/**}:verb");

Original file line number	Diff line number	Diff line change
`@@ -335,12 +335,6 @@ class Parser {`
`335`	`335`	`// this will allow the matcher code to reconstruct the variable`
`336`	`336`	`// value based on the url segments.`
`337`	`337`	`var.end_segment = (var.end_segment - segments_.size() - 1);`
`338`		`-`
`339`		`- if (!verb_.empty()) {`
`340`		`- // a custom verb will add an additional segment, so`
`341`		`- // the end_postion needs a -1`
`342`		`- --var.end_segment;`
`343`		`- }`
`344`	`338`	`}`
`345`	`339`	`}`
`346`	`340`	`}`