Make method return bool.

istio · Aug 9, 2018 · 50b62ed · 50b62ed
1 parent 3681489
commit 50b62ed
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 43 deletions.
diff --git a/include/istio/control/http/check_data.h b/include/istio/control/http/check_data.h
@@ -91,10 +91,13 @@ class CheckData {
   virtual bool GetAuthenticationResult(istio::authn::Result *result) const = 0;
 
   // Get request url path, which strips query part from the http path header.
-  virtual std::string GetUrlPath() const = 0;
+  // Return true if url path is found, otherwise return false.
+  virtual bool GetUrlPath(std::string *url_path) const = 0;
 
-  // Get request queries with string map format.
-  virtual std::map<std::string, std::string> GetRequestQueryParams() const = 0;
+  // Get request queries with string map format. Return true if query params are
+  // found, otherwise return false.
+  virtual bool GetRequestQueryParams(
+      std::map<std::string, std::string> *query_params) const = 0;
 };
 
 // An interfact to update request HTTP headers with Istio attributes.

diff --git a/src/envoy/http/mixer/check_data.cc b/src/envoy/http/mixer/check_data.cc
@@ -204,23 +204,27 @@ bool CheckData::GetAuthenticationResult(istio::authn::Result* result) const {
   return Utils::Authentication::FetchResultFromHeader(headers_, result);
 }
 
-std::string CheckData::GetUrlPath() const {
-  if (headers_.Path()) {
-    const HeaderString& path = headers_.Path()->value();
-    const char* query_start = Utility::findQueryStringStart(path);
-    if (query_start != nullptr) {
-      return std::string(path.c_str(), query_start - path.c_str());
-    }
-    return std::string(path.c_str(), path.size());
+bool CheckData::GetUrlPath(std::string* url_path) const {
+  if (!headers_.Path()) {
+    return false;
   }
-  return "";
+  const HeaderString& path = headers_.Path()->value();
+  const char* query_start = Utility::findQueryStringStart(path);
+  if (query_start != nullptr) {
+    *url_path = std::string(path.c_str(), query_start - path.c_str());
+  } else {
+    *url_path = std::string(path.c_str(), path.size());
+  }
+  return true;
 }
 
-std::map<std::string, std::string> CheckData::GetRequestQueryParams() const {
-  if (headers_.Path()) {
-    return Utility::parseQueryString(headers_.Path()->value().c_str());
+bool CheckData::GetRequestQueryParams(
+    std::map<std::string, std::string>* query_params) const {
+  if (!headers_.Path()) {
+    return false;
   }
-  return std::map<std::string, std::string>();
+  *query_params = Utility::parseQueryString(headers_.Path()->value().c_str());
+  return true;
 }
 
 }  // namespace Mixer

diff --git a/src/envoy/http/mixer/check_data.h b/src/envoy/http/mixer/check_data.h
@@ -61,9 +61,10 @@ class CheckData : public ::istio::control::http::CheckData,
 
   bool GetAuthenticationResult(istio::authn::Result* result) const override;
 
-  std::string GetUrlPath() const override;
+  bool GetUrlPath(std::string* url_path) const override;
 
-  std::map<std::string, std::string> GetRequestQueryParams() const override;
+  bool GetRequestQueryParams(
+      std::map<std::string, std::string>* query_params) const override;
 
  private:
   const HeaderMap& headers_;

diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc
@@ -65,10 +65,13 @@ void AttributesBuilder::ExtractRequestHeaderAttributes(CheckData *check_data) {
     }
   }
 
-  builder.AddString(utils::AttributeName::kRequestUrlPath,
-                    check_data->GetUrlPath());
-  const auto &query_map = check_data->GetRequestQueryParams();
-  if (query_map.size() > 0) {
+  std::string query_path;
+  if (check_data->GetUrlPath(&query_path)) {
+    builder.AddString(utils::AttributeName::kRequestUrlPath, query_path);
+  }
+
+  std::map<std::string, std::string> query_map;
+  if (check_data->GetRequestQueryParams(&query_map) && query_map.size() > 0) {
     builder.AddStringMap(utils::AttributeName::kRequestQueryParams, query_map);
   }
 }

diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc
@@ -493,15 +493,16 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithoutAuthnFilter) {
   EXPECT_CALL(mock_data, GetAuthenticationResult(_))
       .WillOnce(testing::Return(false));
 
-  EXPECT_CALL(mock_data, GetUrlPath()).WillOnce(Invoke([]() -> std::string {
-    return "/books";
-  }));
-  EXPECT_CALL(mock_data, GetRequestQueryParams())
-      .WillOnce(Invoke([]() -> std::map<std::string, std::string> {
-        std::map<std::string, std::string> map;
-        map["a"] = "b";
-        map["c"] = "d";
-        return map;
+  EXPECT_CALL(mock_data, GetUrlPath(_))
+      .WillOnce(Invoke([](std::string *path) -> bool {
+        *path = "/books";
+        return true;
+      }));
+  EXPECT_CALL(mock_data, GetRequestQueryParams(_))
+      .WillOnce(Invoke([](std::map<std::string, std::string> *map) -> bool {
+        (*map)["a"] = "b";
+        (*map)["c"] = "d";
+        return true;
       }));
 
   RequestContext request;
@@ -560,15 +561,16 @@ TEST(AttributesBuilderTest, TestCheckAttributes) {
             }
             return false;
           }));
-  EXPECT_CALL(mock_data, GetUrlPath()).WillOnce(Invoke([]() -> std::string {
-    return "/books";
-  }));
-  EXPECT_CALL(mock_data, GetRequestQueryParams())
-      .WillOnce(Invoke([]() -> std::map<std::string, std::string> {
-        std::map<std::string, std::string> map;
-        map["a"] = "b";
-        map["c"] = "d";
-        return map;
+  EXPECT_CALL(mock_data, GetUrlPath(_))
+      .WillOnce(Invoke([](std::string *path) -> bool {
+        *path = "/books";
+        return true;
+      }));
+  EXPECT_CALL(mock_data, GetRequestQueryParams(_))
+      .WillOnce(Invoke([](std::map<std::string, std::string> *map) -> bool {
+        (*map)["a"] = "b";
+        (*map)["c"] = "d";
+        return true;
       }));
   EXPECT_CALL(mock_data, GetAuthenticationResult(_))
       .WillOnce(Invoke([](istio::authn::Result *result) -> bool {

diff --git a/src/istio/control/http/mock_check_data.h b/src/istio/control/http/mock_check_data.h
@@ -45,9 +45,9 @@ class MockCheckData : public CheckData {
                      bool(istio::authn::Result *result));
   MOCK_CONST_METHOD0(IsMutualTLS, bool());
   MOCK_CONST_METHOD1(GetRequestedServerName, bool(std::string *name));
-  MOCK_CONST_METHOD0(GetUrlPath, std::string());
-  MOCK_CONST_METHOD0(GetRequestQueryParams,
-                     std::map<std::string, std::string>());
+  MOCK_CONST_METHOD1(GetUrlPath, bool(std::string *));
+  MOCK_CONST_METHOD1(GetRequestQueryParams,
+                     bool(std::map<std::string, std::string> *));
 };
 
 // The mock object for HeaderUpdate interface.