b/170423927: #2cleanup dead code for snakeName to jsonName conversion (…

…#372)

api/envoy/v9/http/path_matcher/config.proto

@@ -26,15 +26,10 @@ message PathMatcherRule {
   // Operation name (selector) used as a unique ID for the operation.
   string operation = 2 [(validate.rules).string.min_bytes = 1];
 
-  // If set, the path parameters in the `pattern` must be extracted. Ref:
+  // If true, the path parameters in the `pattern` must be extracted.
+  // Ref:
   // https://cloud.google.com/endpoints/docs/openapi/openapi-extensions#understanding_path_translation
-  PathParameterExtractionRule path_parameter_extraction = 3;
-}
-
-message PathParameterExtractionRule {
-  // Each rule has a mapping of snake_case segments to jsonCase segments.
-  // If the map is empty, the path params still need to be extracted.
-  map<string, string> snake_to_json_segments = 2;
+  bool extract_path_parameters = 3;
 }
 
 message FilterConfig {

examples/testdata/path_matcher/envoy_config.json

@@ -205,8 +205,8 @@
                         "@type": "type.googleapis.com/espv2.api.envoy.v9.http.path_matcher.FilterConfig",
                         "rules": [
                           {
+                            "extractPathParameters": true,
                             "operation": "1.examples_path_matcher_endpoints_cloudesf_testing_cloud_goog.GetShelf",
-                            "pathParameterExtraction": {},
                             "pattern": {
                               "httpMethod": "GET",
                               "uriTemplate": "/libraries/{library_id}/shelves/{shelfId}"
@@ -248,8 +248,8 @@
                             }
                           },
                           {
+                            "extractPathParameters": true,
                             "operation": "1.examples_path_matcher_endpoints_cloudesf_testing_cloud_goog.ESPv2_Autogenerated_CORS_GetShelf",
-                            "pathParameterExtraction": {},
                             "pattern": {
                               "httpMethod": "OPTIONS",
                               "uriTemplate": "/libraries/{library_id}/shelves/{shelf_id}"

src/api_proxy/path_matcher/variable_binding_utils.cc

@@ -21,25 +21,17 @@ namespace api_proxy {
 namespace path_matcher {
 
 const std::string VariableBindingsToQueryParameters(
-    const std::vector<VariableBinding>& variable_bindings,
-    const ::google::protobuf::Map<std::string, std::string>& snake_to_json) {
+    const std::vector<VariableBinding>& variable_bindings) {
   std::string query_params;
   for (size_t i = 0; i < variable_bindings.size(); i++) {
     const VariableBinding& variable_binding = variable_bindings[i];
     for (size_t j = 0; j < variable_binding.field_path.size(); j++) {
+      // This segment should be camel case instead of snake case.
+      // We can add validation here but it will be unnecessary after we have
+      // syntax parser in the control plane to ensure the correctness of url
+      // template.
       const std::string& segment = variable_binding.field_path[j];
-
-      // If the segment has JSON name, use JSON name instead.
-      if (absl::StrContains(segment, "_")) {
-        auto json_name_it = snake_to_json.find(segment);
-        if (json_name_it != snake_to_json.end()) {
-          query_params.append(json_name_it->second);
-        } else {
-          query_params.append(segment);
-        }
-      } else {
-        query_params.append(segment);
-      }
+      query_params.append(segment);
 
       if (j < variable_binding.field_path.size() - 1) {
         query_params.append(".");

src/api_proxy/path_matcher/variable_binding_utils.h

@@ -26,15 +26,13 @@ namespace api_proxy {
 namespace path_matcher {
 
 // Converts `VariableBinding`s to a query parameter string.
-// For example, given the following `VariableBinding`s and
-// snake-cased to JSON map {"foo_bar": "fooBar"}:
-//  * {"foo_bar"}  : "42"
+// For example, given the following variableBinding map:
+//  * {"fooBar"}  : "42"
 //  * {"foo", "bar"} : "42"
 //  * {"a", "b", "c"}: "xyz"
 // it returns "fooBar=42&foo.bar=42&a.b.c=xyz".
 const std::string VariableBindingsToQueryParameters(
-    const std::vector<VariableBinding>& variable_bindings,
-    const ::google::protobuf::Map<std::string, std::string>& snake_to_json);
+    const std::vector<VariableBinding>& variable_bindings);
 
 }  // namespace path_matcher
 }  // namespace api_proxy

src/api_proxy/path_matcher/variable_binding_utils_test.cc

@@ -20,65 +20,29 @@ namespace espv2 {
 namespace api_proxy {
 namespace path_matcher {
 
-TEST(VariableBindingsToQueryParameters, WithoutSnakeToJsonNameConversion) {
-  EXPECT_EQ(VariableBindingsToQueryParameters(/*variable_bindings=*/{},
-                                              /*snake_to_json=*/{}),
-            "");
+TEST(VariableBindingsToQueryParameters, Test) {
+  EXPECT_EQ(VariableBindingsToQueryParameters(/*variable_bindings=*/{}), "");
   EXPECT_EQ(VariableBindingsToQueryParameters(
                 /*variable_bindings=*/
                 {
                     {{"id"}, "42"},
-                },
-                /*snake_to_json=*/
-                {}),
+                }),
             "id=42");
   EXPECT_EQ(VariableBindingsToQueryParameters(
                 /*variable_bindings=*/
                 {
                     {{"foo_bar"}, "42"},
-                },
-                /*snake_to_json=*/
-                {}),
+                }),
             "foo_bar=42");
   EXPECT_EQ(VariableBindingsToQueryParameters(
                 /*variable_bindings=*/
                 {
                     {{"id"}, "42"},
                     {{"foo", "bar", "baz"}, "value"},
                     {{"x", "y"}, "abc"},
-                },
-                /*snake_to_json=*/
-                {}),
+                }),
             "id=42&foo.bar.baz=value&x.y=abc");
 }
-
-TEST(VariableBindingsToQueryParameters, WithSnakeToJsonNameConversion) {
-  ::google::protobuf::Map<std::string, std::string> snake_to_json;
-  snake_to_json["foo_bar"] = "fooBar";
-  EXPECT_EQ(VariableBindingsToQueryParameters(
-                /*variable_bindings=*/
-                {
-                    {{"foo_bar"}, "42"},
-                },
-                /*snake_to_json=*/snake_to_json),
-            "fooBar=42");
-
-  snake_to_json.clear();
-  snake_to_json["foo_foo"] = "fooFoo";
-  snake_to_json["bar_bar"] = "barBar";
-  snake_to_json["book_shelf"] = "bookShelf";
-  snake_to_json["book_id"] = "bookId";
-  snake_to_json["non_exist"] = "nonExist";
-  EXPECT_EQ(VariableBindingsToQueryParameters(
-                /*variable_bindings=*/
-                {
-                    {{"foo_foo", "bar_bar"}, "value"},
-                    {{"book_shelf", "book_id"}, "42"},
-                },
-                /*snake_to_json=*/snake_to_json),
-            "fooFoo.barBar=value&bookShelf.bookId=42");
-}
-
 }  // namespace path_matcher
 }  // namespace api_proxy
 }  // namespace espv2

src/envoy/http/path_matcher/filter.cc

@@ -29,7 +29,6 @@ namespace path_matcher {
 
 using ::Envoy::Http::RequestHeaderMap;
 using ::espv2::api::envoy::v9::http::path_matcher::PathMatcherRule;
-using ::espv2::api::envoy::v9::http::path_matcher::PathParameterExtractionRule;
 using ::espv2::api_proxy::path_matcher::VariableBinding;
 using ::espv2::api_proxy::path_matcher::VariableBindingsToQueryParameters;
 using ::google::protobuf::util::Status;
@@ -94,18 +93,14 @@ Envoy::Http::FilterHeadersStatus Filter::decodeHeaders(
   utils::setStringFilterState(filter_state, utils::kFilterStateOperation,
                               operation);
 
-  if (rule->has_path_parameter_extraction()) {
-    const PathParameterExtractionRule& param_rule =
-        rule->path_parameter_extraction();
-
+  if (rule->extract_path_parameters()) {
     std::vector<VariableBinding> variable_bindings;
     config_->findRule(method, path, &variable_bindings);
 
     if (!variable_bindings.empty()) {
-      const std::string query_params = VariableBindingsToQueryParameters(
-          variable_bindings, param_rule.snake_to_json_segments());
-      utils::setStringFilterState(filter_state, utils::kFilterStateQueryParams,
-                                  query_params);
+      utils::setStringFilterState(
+          filter_state, utils::kFilterStateQueryParams,
+          VariableBindingsToQueryParameters(variable_bindings));
     }
   }
 

src/envoy/http/path_matcher/filter_config_test.cc

@@ -28,7 +28,6 @@ namespace path_matcher {
 namespace {
 
 using ::espv2::api::envoy::v9::http::path_matcher::PathMatcherRule;
-using ::espv2::api::envoy::v9::http::path_matcher::PathParameterExtractionRule;
 using ::espv2::api_proxy::path_matcher::VariableBinding;
 using ::google::protobuf::TextFormat;
 using VariableBindings = std::vector<VariableBinding>;
@@ -54,6 +53,7 @@ rules {
 }
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Foo"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
     uri_template: "/foo/{id}"
@@ -79,6 +79,7 @@ TEST(FilterConfigTest, VariableBinding) {
   const char kFilterConfigBasic[] = R"(
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Foo"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
     uri_template: "/foo/{id}"
@@ -106,75 +107,19 @@ rules {
                       }));
 }
 
-TEST(FilterConfigTest, PathParameterExtraction) {
-  const char kFilterConfigBasic[] = R"(
-rules {
-  operation: "1.cloudesf_testing_cloud_goog.Bar"
-  pattern {
-    http_method: "GET"
-    uri_template: "/bar/{shelf_id}"
-  }
-  path_parameter_extraction {
-    snake_to_json_segments {
-      key: "shelf_id"
-      value: "shelfId"
-    }
-    snake_to_json_segments {
-      key: "foo_bar"
-      value: "fooBar"
-    }
-  }
-}
-rules {
-  operation: "1.cloudesf_testing_cloud_goog.Foo"
-  pattern {
-    http_method: "GET"
-    uri_template: "/foo/{id}"
-  }
-  path_parameter_extraction {}
-}
-rules {
-  operation: "1.cloudesf_testing_cloud_goog.Baz"
-  pattern {
-    http_method: "GET"
-    uri_template: "/baz/{id}"
-  }
-})";
-
-  ::espv2::api::envoy::v9::http::path_matcher::FilterConfig config_pb;
-  ASSERT_TRUE(TextFormat::ParseFromString(kFilterConfigBasic, &config_pb));
-  ::testing::NiceMock<Envoy::Server::Configuration::MockFactoryContext>
-      mock_factory;
-  FilterConfig cfg(config_pb, Envoy::EMPTY_STRING, mock_factory);
-
-  const PathMatcherRule* matcher_rule = cfg.findRule("GET", "/baz/1");
-  EXPECT_FALSE(matcher_rule->has_path_parameter_extraction());
-
-  matcher_rule = cfg.findRule("GET", "/foo/2");
-  ASSERT_TRUE(matcher_rule->has_path_parameter_extraction());
-  PathParameterExtractionRule param_rule =
-      matcher_rule->path_parameter_extraction();
-  EXPECT_TRUE(param_rule.snake_to_json_segments().empty());
-
-  matcher_rule = cfg.findRule("GET", "/bar/3");
-  ASSERT_TRUE(matcher_rule->has_path_parameter_extraction());
-  param_rule = matcher_rule->path_parameter_extraction();
-  EXPECT_FALSE(param_rule.snake_to_json_segments().empty());
-  EXPECT_EQ(param_rule.snake_to_json_segments().at("shelf_id"), "shelfId");
-  EXPECT_EQ(param_rule.snake_to_json_segments().at("foo_bar"), "fooBar");
-}
-
 TEST(FilterConfigTest, DuplicatedPatterns) {
   const char kFilterConfig[] = R"(
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Bar"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
     uri_template: "/bar/{id}"
   }
 }
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Bar1"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
     uri_template: "/bar/{x}"
@@ -195,6 +140,7 @@ TEST(FilterConfigTest, InvalidPattern) {
   const char kFilterConfig[] = R"(
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Bar"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
     uri_template: "/bar/{id{x}}"

src/envoy/http/path_matcher/filter_test.cc

@@ -42,15 +42,10 @@ rules {
 }
 rules {
   operation: "1.cloudesf_testing_cloud_goog.Foo"
+  extract_path_parameters: true
   pattern {
     http_method: "GET"
-    uri_template: "/foo/{foo_bar}"
-  }
-  path_parameter_extraction {
-    snake_to_json_segments {
-      key: "foo_bar"
-      value: "fooBar"
-    }
+    uri_template: "/foo/{fooBar}"
   }
 }
 rules {

src/go/configgenerator/listener_generator.go

@@ -344,7 +344,7 @@ func makePathMatcherFilter(serviceInfo *sc.ServiceInfo) *hcmpb.HttpFilter {
 					Pattern:   httpRule,
 				}
 				if method.BackendInfo != nil && method.BackendInfo.TranslationType == confpb.BackendRule_CONSTANT_ADDRESS && hasPathParameter(newHttpRule.Pattern.UriTemplate) {
-					newHttpRule.PathParameterExtraction = &pmpb.PathParameterExtractionRule{}
+					newHttpRule.ExtractPathParameters = true
 				}
 				rules = append(rules, newHttpRule)
 			}

src/go/configgenerator/listener_generator_test.go

@@ -1259,11 +1259,11 @@ func TestPathMatcherFilter(t *testing.T) {
          },
          {
             "operation":"1.cloudesf_testing_cloud_goog.Foo",
+            "extractPathParameters": true,
             "pattern":{
                "httpMethod":"GET",
                "uriTemplate":"foo/{id}"
-            },
-            "pathParameterExtraction":{}
+            }
          }
       ]
    }
@@ -1410,15 +1410,15 @@ func TestPathMatcherFilter(t *testing.T) {
       "rules":[
          {
             "operation":"1.cloudesf_testing_cloud_goog.Baz",
-            "pathParameterExtraction":{},
+            "extractPathParameters": true,
             "pattern":{
                "httpMethod":"GET",
                "uriTemplate":"baz/{bazBaz}"
             }
          },
          {
             "operation":"1.cloudesf_testing_cloud_goog.Foo",
-            "pathParameterExtraction":{},
+            "extractPathParameters": true,
             "pattern":{
                "httpMethod":"GET",
                "uriTemplate":"foo/{fooBar}"

tests/fuzz/corpus/path_matcher_filter/invalid-missing-headers.prototxt

@@ -1,15 +1,10 @@
 config {
   rules {
     operation: "1.cloudesf_testing_cloud_goog.Foo"
+    extract_path_parameters: true
     pattern {
       http_method: "GET"
-      uri_template: "/foo/{foo_bar}"
-    }
-    path_parameter_extraction {
-      snake_to_json_segments {
-        key: "foo_bar"
-        value: "fooBar"
-      }
+      uri_template: "/foo/{fooBar}"
     }
   }
 }

tests/fuzz/corpus/path_matcher_filter/valid-extract-path-params.prototxt

@@ -1,15 +1,10 @@
 config {
   rules {
     operation: "1.cloudesf_testing_cloud_goog.Foo"
+    extract_path_parameters: true
     pattern {
       http_method: "GET"
-      uri_template: "/foo/{foo_bar}"
-    }
-    path_parameter_extraction {
-      snake_to_json_segments {
-        key: "foo_bar"
-        value: "fooBar"
-      }
+      uri_template: "/foo/{fooBar}"
     }
   }
 }