diff --git a/mixerclient/src/attribute_context.cc b/mixerclient/src/attribute_context.cc index a64ebef4226..cfa7312da56 100644 --- a/mixerclient/src/attribute_context.cc +++ b/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& old_map, + const std::map& new_map, + std::set* 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& string_map) { + const std::map& string_map, + const std::set& 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,7 +95,7 @@ void AttributeContext::FillProto(const Attributes& attributes, size_t old_dict_size = dict_map_.size(); context_.UpdateStart(); - std::set string_map_indexes; + std::set deleted_indexes; // Fill attributes. for (const auto& it : attributes.attributes) { @@ -81,8 +103,21 @@ void AttributeContext::FillProto(const Attributes& attributes, 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 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); } diff --git a/mixerclient/src/attribute_context.h b/mixerclient/src/attribute_context.h index 0f25286b397..12415392c8c 100644 --- a/mixerclient/src/attribute_context.h +++ b/mixerclient/src/attribute_context.h @@ -39,7 +39,8 @@ class AttributeContext { // Create a StringMap message. ::istio::mixer::v1::StringMap CreateStringMap( - const std::map& string_map); + const std::map& string_map, + const std::set& exclude_keys); // dictionary map. std::map dict_map_; diff --git a/mixerclient/src/attribute_context_test.cc b/mixerclient/src/attribute_context_test.cc index 6b2ede2b3e0..356890889b2 100644 --- a/mixerclient/src/attribute_context_test.cc +++ b/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 diff --git a/mixerclient/src/context_update.cc b/mixerclient/src/context_update.cc index dc07e23fe49..422fd2c0e15 100644 --- a/mixerclient/src/context_update.cc +++ b/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; } diff --git a/mixerclient/src/context_update.h b/mixerclient/src/context_update.h index 611a0bb2927..9d1c4d83220 100644 --- a/mixerclient/src/context_update.h +++ b/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 + 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. diff --git a/mixerclient/src/context_update_test.cc b/mixerclient/src/context_update_test.cc index 5442413d9bc..0378b8761c8 100644 --- a/mixerclient/src/context_update_test.cc +++ b/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 deleted_list = {2}; EXPECT_EQ(d.UpdateFinish(), deleted_list);