Refactory CacheKeySet class. (#57)

istio · Apr 21, 2017 · 7b78421 · 7b78421
1 parent 68964bd
commit 7b78421
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 87 deletions.
diff --git a/mixerclient/src/cache_key_set.cc b/mixerclient/src/cache_key_set.cc
@@ -13,36 +13,112 @@
  * limitations under the License.
  */
 
-#include "src/cache_key_set.h"
+#include "cache_key_set.h"
+#include <map>
+#include <set>
 
 namespace istio {
 namespace mixer_client {
+namespace {
 
-CacheKeySet::CacheKeySet(const std::vector<std::string>& cache_keys) {
-  for (const std::string& key : cache_keys) {
-    size_t pos = key.find_first_of('/');
-    if (pos == std::string::npos) {
-      // Always override with an empty one.
-      // It handles case where
-      // "key/sub_key" comes first, then "key" come.
-      // In this case, "key" wins.
-      key_map_[key] = SubKeySet();
+// A class to store sub key set
+class SubKeySetImpl : public SubKeySet {
+ public:
+  // Check if the sub key is in the set.
+  bool Found(const std::string& sub_key) const override {
+    if (sub_keys_.empty()) {
+      return true;
     } else {
-      std::string split_key = key.substr(0, pos);
-      std::string split_sub_key = key.substr(pos + 1);
-      auto it = key_map_.find(split_key);
-      if (it == key_map_.end()) {
-        key_map_[split_key] = SubKeySet();
-        key_map_[split_key].sub_keys_.insert(split_sub_key);
+      return sub_keys_.find(sub_key) != sub_keys_.end();
+    }
+  }
+
+ private:
+  friend class InclusiveCacheKeySet;
+  std::set<std::string> sub_keys_;
+};
+
+class InclusiveCacheKeySet : public CacheKeySet {
+ public:
+  InclusiveCacheKeySet(const std::vector<std::string>& cache_keys) {
+    for (const std::string& key : cache_keys) {
+      size_t pos = key.find_first_of('/');
+      if (pos == std::string::npos) {
+        // Always override with an empty one.
+        // It handles case where
+        // "key/sub_key" comes first, then "key" come.
+        // In this case, "key" wins.
+        key_map_[key] = SubKeySetImpl();
       } else {
-        // If map to empty set, it was created by "key"
-        // without sub_key, keep it as empty set.
-        if (!it->second.sub_keys_.empty()) {
-          it->second.sub_keys_.insert(split_sub_key);
+        std::string split_key = key.substr(0, pos);
+        std::string split_sub_key = key.substr(pos + 1);
+        auto it = key_map_.find(split_key);
+        if (it == key_map_.end()) {
+          key_map_[split_key] = SubKeySetImpl();
+          key_map_[split_key].sub_keys_.insert(split_sub_key);
+        } else {
+          // If map to empty set, it was created by "key"
+          // without sub_key, keep it as empty set.
+          if (!it->second.sub_keys_.empty()) {
+            it->second.sub_keys_.insert(split_sub_key);
+          }
         }
       }
     }
   }
+
+  // Return nullptr if the key is not in the set.
+  const SubKeySet* Find(const std::string& key) const override {
+    const auto it = key_map_.find(key);
+    return it == key_map_.end() ? nullptr : &it->second;
+  }
+
+ private:
+  std::map<std::string, SubKeySetImpl> key_map_;
+};
+
+// Exclusive key set
+class ExclusiveCacheKeySet : public CacheKeySet {
+ public:
+  ExclusiveCacheKeySet(const std::vector<std::string>& exclusive_keys) {
+    keys_.insert(exclusive_keys.begin(), exclusive_keys.end());
+  }
+
+  const SubKeySet* Find(const std::string& key) const override {
+    const auto it = keys_.find(key);
+    return it == keys_.end() ? &subkey_ : nullptr;
+  }
+
+ private:
+  SubKeySetImpl subkey_;
+  std::set<std::string> keys_;
+};
+
+// all key set
+class AllCacheKeySet : public CacheKeySet {
+ public:
+  const SubKeySet* Find(const std::string& key) const override {
+    return &subkey_;
+  }
+
+ private:
+  SubKeySetImpl subkey_;
+};
+
+}  // namespace
+
+std::unique_ptr<CacheKeySet> CacheKeySet::CreateInclusive(
+    const std::vector<std::string>& inclusive_keys) {
+  return std::unique_ptr<CacheKeySet>(new InclusiveCacheKeySet(inclusive_keys));
+}
+
+std::unique_ptr<CacheKeySet> CacheKeySet::CreateExclusive(
+    const std::vector<std::string>& exclusive_keys) {
+  return std::unique_ptr<CacheKeySet>(new ExclusiveCacheKeySet(exclusive_keys));
+}
+
+std::unique_ptr<CacheKeySet> CacheKeySet::CreateAll() {
+  return std::unique_ptr<CacheKeySet>(new AllCacheKeySet());
 }
 
 }  // namespace mixer_client

diff --git a/mixerclient/src/cache_key_set.h b/mixerclient/src/cache_key_set.h
@@ -16,51 +16,44 @@
 #ifndef MIXER_CLIENT_CACHE_KEY_SET_H_
 #define MIXER_CLIENT_CACHE_KEY_SET_H_
 
-#include <map>
-#include <set>
+#include <memory>
 #include <string>
 #include <vector>
 
 namespace istio {
 namespace mixer_client {
 
-// A class to store sub key set
 class SubKeySet {
  public:
-  // Check if the sub key is in the set.
-  bool Found(const std::string& sub_key) const {
-    if (sub_keys_.empty()) {
-      return true;
-    } else {
-      return sub_keys_.find(sub_key) != sub_keys_.end();
-    }
-  }
+  virtual ~SubKeySet() {}
 
- private:
-  friend class CacheKeySet;
-  std::set<std::string> sub_keys_;
+  // Check if subkey should be used.
+  virtual bool Found(const std::string& sub_key) const = 0;
 };
 
-// A class to handle cache key set
-// A std::set will not work for string map sub keys.
-// For a StringMap attruibute,
-// If cache key is "attribute.key", all values will be used.
-// If cache key is "attribute.key/map.key", only the map key is used.
-// If both format exists, the whole map will be used.
 class CacheKeySet {
  public:
-  CacheKeySet(const std::vector<std::string>& cache_keys);
-
-  // Return nullptr if the key is not in the set.
-  const SubKeySet* Find(const std::string& key) const {
-    const auto it = key_map_.find(key);
-    return it == key_map_.end() ? nullptr : &it->second;
-  }
-
-  bool empty() const { return key_map_.empty(); }
-
- private:
-  std::map<std::string, SubKeySet> key_map_;
+  virtual ~CacheKeySet() {}
+
+  // Check if a key should be used, return nullptr if no.
+  virtual const SubKeySet* Find(const std::string& key) const = 0;
+
+  // Only the keys in the inclusive_keys are used.
+  // A class to handle cache key set
+  // A std::set will not work for string map sub keys.
+  // For a StringMap attruibute,
+  // If cache key is "attribute.key", all values will be used.
+  // If cache key is "attribute.key/map.key", only the map key is used.
+  // If both format exists, the whole map will be used.
+  static std::unique_ptr<CacheKeySet> CreateInclusive(
+      const std::vector<std::string>& inclusive_keys);
+
+  // Only the keys NOT in the exclusive_keys are used.
+  static std::unique_ptr<CacheKeySet> CreateExclusive(
+      const std::vector<std::string>& exclusive_keys);
+
+  // All keys are used.
+  static std::unique_ptr<CacheKeySet> CreateAll();
 };
 
 }  // namespace mixer_client

diff --git a/mixerclient/src/cache_key_set_test.cc b/mixerclient/src/cache_key_set_test.cc
@@ -21,31 +21,31 @@ namespace mixer_client {
 namespace {
 
 TEST(CacheKeySetTest, TestNonSubkey) {
-  CacheKeySet s({"key1", "key2"});
-  EXPECT_TRUE(s.Find("key1") != nullptr);
-  EXPECT_TRUE(s.Find("key2") != nullptr);
-  EXPECT_TRUE(s.Find("xyz") == nullptr);
+  auto s = CacheKeySet::CreateInclusive({"key1", "key2"});
+  EXPECT_TRUE(s->Find("key1") != nullptr);
+  EXPECT_TRUE(s->Find("key2") != nullptr);
+  EXPECT_TRUE(s->Find("xyz") == nullptr);
 }
 
 TEST(CacheKeySetTest, TestSubkeyLater) {
   // Whole key comes first, then sub key comes.
-  CacheKeySet s({"key1", "key2", "key2/sub"});
-  EXPECT_TRUE(s.Find("key2")->Found("sub"));
-  EXPECT_TRUE(s.Find("key2")->Found("xyz"));
+  auto s = CacheKeySet::CreateInclusive({"key1", "key2", "key2/sub"});
+  EXPECT_TRUE(s->Find("key2")->Found("sub"));
+  EXPECT_TRUE(s->Find("key2")->Found("xyz"));
 }
 
 TEST(CacheKeySetTest, TestSubkeyFirst) {
   // The sub key comes first, then the whole key comes
-  CacheKeySet s({"key1", "key2/sub", "key2"});
-  EXPECT_TRUE(s.Find("key2")->Found("sub"));
-  EXPECT_TRUE(s.Find("key2")->Found("xyz"));
+  auto s = CacheKeySet::CreateInclusive({"key1", "key2/sub", "key2"});
+  EXPECT_TRUE(s->Find("key2")->Found("sub"));
+  EXPECT_TRUE(s->Find("key2")->Found("xyz"));
 }
 
 TEST(CacheKeySetTest, TestSubkey) {
-  CacheKeySet s({"key1", "key2/sub1", "key2/sub2"});
-  EXPECT_TRUE(s.Find("key2")->Found("sub1"));
-  EXPECT_TRUE(s.Find("key2")->Found("sub2"));
-  EXPECT_FALSE(s.Find("key2")->Found("xyz"));
+  auto s = CacheKeySet::CreateInclusive({"key1", "key2/sub1", "key2/sub2"});
+  EXPECT_TRUE(s->Find("key2")->Found("sub1"));
+  EXPECT_TRUE(s->Find("key2")->Found("sub2"));
+  EXPECT_FALSE(s->Find("key2")->Found("xyz"));
 }
 
 }  // namespace

diff --git a/mixerclient/src/check_cache.cc b/mixerclient/src/check_cache.cc
@@ -27,17 +27,17 @@ using ::google::protobuf::util::error::Code;
 namespace istio {
 namespace mixer_client {
 
-CheckCache::CheckCache(const CheckOptions& options)
-    : options_(options), cache_keys_(options.cache_keys) {
+CheckCache::CheckCache(const CheckOptions& options) : options_(options) {
   // Converts flush_interval_ms to Cycle used by SimpleCycleTimer.
   flush_interval_in_cycle_ =
       options_.flush_interval_ms * SimpleCycleTimer::Frequency() / 1000;
 
-  if (options.num_entries >= 0 && !cache_keys_.empty()) {
+  if (options.num_entries > 0 && !options_.cache_keys.empty()) {
     cache_.reset(new CheckLRUCache(
         options.num_entries, std::bind(&CheckCache::OnCacheEntryDelete, this,
                                        std::placeholders::_1)));
     cache_->SetMaxIdleSeconds(options.expiration_ms / 1000.0);
+    cache_keys_ = CacheKeySet::CreateInclusive(options_.cache_keys);
   }
 }
 
@@ -58,7 +58,7 @@ Status CheckCache::Check(const Attributes& attributes, CheckResponse* response,
     return Status(Code::NOT_FOUND, "");
   }
 
-  std::string request_signature = GenerateSignature(attributes, cache_keys_);
+  std::string request_signature = GenerateSignature(attributes, *cache_keys_);
   if (signature) {
     *signature = request_signature;
   }

diff --git a/mixerclient/src/check_cache.h b/mixerclient/src/check_cache.h
@@ -118,7 +118,7 @@ class CheckCache {
   CheckOptions options_;
 
   // The cache keys.
-  CacheKeySet cache_keys_;
+  std::unique_ptr<CacheKeySet> cache_keys_;
 
   // Mutex guarding the access of cache_;
   std::mutex cache_mutex_;

diff --git a/mixerclient/src/signature_test.cc b/mixerclient/src/signature_test.cc
@@ -64,55 +64,48 @@ class SignatureUtilTest : public ::testing::Test {
     attributes_.attributes[key] = Attributes::StringMapValue(std::move(value));
   }
 
-  CacheKeySet GetKeys() const {
-    std::vector<std::string> keys;
-    for (const auto& it : attributes_.attributes) {
-      keys.push_back(it.first);
-    }
-    return CacheKeySet(keys);
-  }
-
   Attributes attributes_;
 };
 
 TEST_F(SignatureUtilTest, Attributes) {
+  auto key_set = CacheKeySet::CreateAll();
   AddString("string-key", "this is a string value");
   EXPECT_EQ("26f8f724383c46e7f5803380ab9c17ba",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   AddBytes("bytes-key", "this is a bytes value");
   EXPECT_EQ("1f409524b79b9b5760032dab7ecaf960",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   AddDoublePair("double-key", 99.9);
   EXPECT_EQ("6183342ff222018f6300de51cdcd4501",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   AddInt64Pair("int-key", 35);
   EXPECT_EQ("d681b9c72d648f9c831d95b4748fe1c2",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   AddBoolPair("bool-key", true);
   EXPECT_EQ("958930b41f0d8b43f5c61c31b0b092e2",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   // default to Clock's epoch.
   std::chrono::time_point<std::chrono::system_clock> time_point;
   AddTime("time-key", time_point);
   EXPECT_EQ("f7dd61e1a5881e2492d93ad023ab49a2",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   std::chrono::seconds secs(5);
   AddDuration("duration-key",
               std::chrono::duration_cast<std::chrono::nanoseconds>(secs));
   EXPECT_EQ("13ae11ea2bea216da46688cd9698645e",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 
   std::map<std::string, std::string> string_map = {{"key1", "value1"},
                                                    {"key2", "value2"}};
   AddStringMap("string-map-key", std::move(string_map));
   EXPECT_EQ("c861f02e251c896513eb0f7c97aa2ce7",
-            MD5::DebugString(GenerateSignature(attributes_, GetKeys())));
+            MD5::DebugString(GenerateSignature(attributes_, *key_set)));
 }
 
 }  // namespace