Support string map delta update. (#34)

mixerclient/src/attribute_context.cc

@@ -42,6 +42,25 @@ Duration CreateDuration(std::chrono::nanoseconds value) {
   return duration;
 }
 
+// Compare two string maps to check
+// 1) any removed keys: ones in the old, but not in the new.
+// 2) Get all key/value pairs which are the same in the two maps.
+// Return true if there are some keys removed.
+bool CompareStringMaps(const std::map<std::string, std::string>& old_map,
+                       const std::map<std::string, std::string>& new_map,
+                       std::set<std::string>* same_keys) {
+  for (const auto& old_it : old_map) {
+    const auto& new_it = new_map.find(old_it.first);
+    if (new_it == new_map.end()) {
+      return true;
+    }
+    if (old_it.second == new_it->second) {
+      same_keys->insert(old_it.first);
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 int AttributeContext::GetNameIndex(const std::string& name) {
@@ -59,11 +78,14 @@ int AttributeContext::GetNameIndex(const std::string& name) {
 }
 
 ::istio::mixer::v1::StringMap AttributeContext::CreateStringMap(
-    const std::map<std::string, std::string>& string_map) {
+    const std::map<std::string, std::string>& string_map,
+    const std::set<std::string>& exclude_keys) {
   ::istio::mixer::v1::StringMap map_msg;
   auto* map_pb = map_msg.mutable_map();
   for (const auto& it : string_map) {
-    (*map_pb)[GetNameIndex(it.first)] = it.second;
+    if (exclude_keys.find(it.first) == exclude_keys.end()) {
+      (*map_pb)[GetNameIndex(it.first)] = it.second;
+    }
   }
   return map_msg;
 }
@@ -73,16 +95,29 @@ void AttributeContext::FillProto(const Attributes& attributes,
   size_t old_dict_size = dict_map_.size();
 
   context_.UpdateStart();
-  std::set<int> string_map_indexes;
+  std::set<int> deleted_indexes;
 
   // Fill attributes.
   for (const auto& it : attributes.attributes) {
     const std::string& name = it.first;
 
     int index = GetNameIndex(name);
 
-    // Check the context, if same, no need to send it.
-    if (context_.Update(index, it.second)) {
+    // Handle StringMap differently to support its delta update.
+    std::set<std::string> same_keys;
+    bool has_removed_keys = false;
+    ContextUpdate::CompValueFunc cmp_func;
+    if (it.second.type == Attributes::Value::ValueType::STRING_MAP) {
+      cmp_func = [&](const Attributes::Value& old_value,
+                     const Attributes::Value& new_value) {
+        has_removed_keys = CompareStringMaps(
+            old_value.string_map_v, new_value.string_map_v, &same_keys);
+      };
+    }
+
+    // The function returns true if the attribute is same as one
+    // in the context.
+    if (context_.Update(index, it.second, cmp_func)) {
       continue;
     }
 
@@ -112,19 +147,20 @@ void AttributeContext::FillProto(const Attributes& attributes,
             CreateDuration(it.second.duration_nanos_v);
         break;
       case Attributes::Value::ValueType::STRING_MAP:
+        // If there are some keys removed, do a whole map replacement
+        if (has_removed_keys) {
+          deleted_indexes.insert(index);
+          same_keys.clear();
+        }
         (*pb->mutable_stringmap_attributes())[index] =
-            CreateStringMap(it.second.string_map_v);
-        string_map_indexes.insert(index);
+            CreateStringMap(it.second.string_map_v, same_keys);
         break;
     }
   }
 
-  auto deleted_attrs = context_.UpdateFinish();
-  // TODO: support string map update.  Before that,
-  // all string_map fields should be marked as "deleted"
-  // in order to replace the whole map.
-  deleted_attrs.insert(string_map_indexes.begin(), string_map_indexes.end());
-  for (const auto& it : deleted_attrs) {
+  auto deleted = context_.UpdateFinish();
+  deleted.insert(deleted_indexes.begin(), deleted_indexes.end());
+  for (const auto& it : deleted) {
     pb->add_deleted_attributes(it);
   }
 

mixerclient/src/attribute_context.h

@@ -39,7 +39,8 @@ class AttributeContext {
 
   // Create a StringMap message.
   ::istio::mixer::v1::StringMap CreateStringMap(
-      const std::map<std::string, std::string>& string_map);
+      const std::map<std::string, std::string>& string_map,
+      const std::set<std::string>& exclude_keys);
 
   // dictionary map.
   std::map<std::string, int> dict_map_;

mixerclient/src/attribute_context_test.cc

@@ -176,8 +176,6 @@ stringMap_attributes {
     }
   }
 }
-deleted_attributes: 15
-deleted_attributes: 16
 )";
 
 // A expected delta attribute protobuf in text.
@@ -209,6 +207,47 @@ string_attributes {
 deleted_attributes: 3
 )";
 
+// A expected delta attribute protobuf in text
+// for string map update.
+const char kAttributeStringMapDelta[] = R"(
+stringMap_attributes {
+  key: 1
+  value {
+    map {
+      key: 1
+      value: "value11"
+    }
+  }
+}
+stringMap_attributes {
+  key: 2
+  value {
+    map {
+      key: 2
+      value: "value22"
+    }
+    map {
+      key: 3
+      value: "value3"
+    }
+  }
+}
+stringMap_attributes {
+  key: 3
+  value {
+    map {
+      key: 1
+      value: "value1"
+    }
+    map {
+      key: 2
+      value: "value2"
+    }
+  }
+}
+deleted_attributes: 3
+)";
+
 TEST(AttributeContextTest, TestFillProto) {
   AttributeContext c;
 
@@ -291,5 +330,47 @@ TEST(AttributeContextTest, TestDeltaUpdate) {
   EXPECT_TRUE(MessageDifferencer::Equals(pb_attrs, expected_attr));
 }
 
+TEST(AttributeContextTest, TestStingMapUpdate) {
+  AttributeContext c;
+
+  Attributes a;
+  a.attributes["key1"] = Attributes::StringMapValue({{"key1", "value1"}});
+  a.attributes["key2"] =
+      Attributes::StringMapValue({{"key1", "value1"}, {"key2", "value2"}});
+  a.attributes["key3"] = Attributes::StringMapValue(
+      {{"key1", "value1"}, {"key2", "value2"}, {"key3", "value3"}});
+
+  ::istio::mixer::v1::Attributes pb_attrs;
+  c.FillProto(a, &pb_attrs);
+
+  // Update same attribute again, should generate an empty proto
+  pb_attrs.Clear();
+  c.FillProto(a, &pb_attrs);
+  ::istio::mixer::v1::Attributes empty_attr;
+  EXPECT_TRUE(MessageDifferencer::Equals(pb_attrs, empty_attr));
+
+  Attributes b;
+  // Value changed
+  b.attributes["key1"] = Attributes::StringMapValue({{"key1", "value11"}});
+  // an new key added
+  b.attributes["key2"] = Attributes::StringMapValue(
+      {{"key1", "value1"}, {"key2", "value22"}, {"key3", "value3"}});
+  // a key removed.
+  b.attributes["key3"] =
+      Attributes::StringMapValue({{"key1", "value1"}, {"key2", "value2"}});
+
+  pb_attrs.Clear();
+  c.FillProto(b, &pb_attrs);
+
+  std::string out_str;
+  TextFormat::PrintToString(pb_attrs, &out_str);
+  GOOGLE_LOG(INFO) << "===" << out_str << "===";
+
+  ::istio::mixer::v1::Attributes expected_attr;
+  ASSERT_TRUE(
+      TextFormat::ParseFromString(kAttributeStringMapDelta, &expected_attr));
+  EXPECT_TRUE(MessageDifferencer::Equals(pb_attrs, expected_attr));
+}
+
 }  // namespace mixer_client
 }  // namespace istio

mixerclient/src/context_update.cc

@@ -24,9 +24,19 @@ void ContextUpdate::UpdateStart() {
   }
 }
 
-bool ContextUpdate::Update(int index, Attributes::Value value) {
+bool ContextUpdate::Update(int index, Attributes::Value value,
+                           CompValueFunc cmp_func) {
   auto it = map_.find(index);
-  bool same = (it != map_.end() && it->second == value);
+  bool same = false;
+  if (it != map_.end()) {
+    if (it->second == value) {
+      same = true;
+    } else {
+      if (cmp_func) {
+        cmp_func(it->second, value);
+      }
+    }
+  }
   if (!same) {
     map_[index] = value;
   }

mixerclient/src/context_update.h

@@ -29,10 +29,16 @@ class ContextUpdate {
   // Start a update for a request.
   void UpdateStart();
 
+  // A function to compare two values.
+  typedef std::function<void(const Attributes::Value& old_value,
+                             const Attributes::Value& new_value)>
+      CompValueFunc;
+
   // Check an attribute, return true if the attribute
   // is in the context with same value, not need to send it.
   // Otherwise, update the context.
-  bool Update(int index, Attributes::Value value);
+  // If the attribute is in the context, call cmp_func.
+  bool Update(int index, Attributes::Value value, CompValueFunc cmp_func);
 
   // Finish a update for a request, remove these not in
   // the current request, and return the deleted set.

mixerclient/src/context_update_test.cc

@@ -21,19 +21,20 @@ namespace mixer_client {
 
 TEST(ContextUpdateTest, Test1) {
   ContextUpdate d;
+  ContextUpdate::CompValueFunc cmp_func;
 
   // Build a context with 1, and 2
   d.UpdateStart();
-  EXPECT_FALSE(d.Update(1, Attributes::Int64Value(1)));
-  EXPECT_FALSE(d.Update(2, Attributes::Int64Value(2)));
-  EXPECT_FALSE(d.Update(3, Attributes::Int64Value(3)));
+  EXPECT_FALSE(d.Update(1, Attributes::Int64Value(1), cmp_func));
+  EXPECT_FALSE(d.Update(2, Attributes::Int64Value(2), cmp_func));
+  EXPECT_FALSE(d.Update(3, Attributes::Int64Value(3), cmp_func));
   EXPECT_TRUE(d.UpdateFinish().empty());
 
   d.UpdateStart();
   // 1 is the same.
-  EXPECT_TRUE(d.Update(1, Attributes::Int64Value(1)));
+  EXPECT_TRUE(d.Update(1, Attributes::Int64Value(1), cmp_func));
   // 3 is with different value.
-  EXPECT_FALSE(d.Update(3, Attributes::Int64Value(33)));
+  EXPECT_FALSE(d.Update(3, Attributes::Int64Value(33), cmp_func));
   // 2 is in the deleted list.
   std::set<int> deleted_list = {2};
   EXPECT_EQ(d.UpdateFinish(), deleted_list);