Fix dynamic::insert

Summary: folly::dynamic::insert is broken. When we try to insert an rvalue and the key already exists in dynamic, insert will replace the value with empty dynamic.

Test Plan: unit test attached

Reviewed By: stepan@fb.com

Subscribers: trunkagent, njormrod, folly-diffs@

FB internal diff: D1704995

Signature: t1:1704995:1417053631:8e0163693df721c54b016f8c27e81e7bf60194cb

diff --git a/folly/dynamic-inl.h b/folly/dynamic-inl.h
index 81f063fb0a805bf3ae0da060db73cb933f633d67..f0a580cb7e7c266c2afb83d3df725a7cdd33e614 100644 (file)
--- a/folly/dynamic-inl.h
+++ b/folly/dynamic-inl.h
@@ -636,15 +636,8 @@ inline dynamic::const_item_iterator dynamic::find(dynamic const& key) const {
 
 template<class K, class V> inline void dynamic::insert(K&& key, V&& val) {
   auto& obj = get<ObjectImpl>();
-  auto rv = obj.insert(std::make_pair(std::forward<K>(key),
-                                      std::forward<V>(val)));
-  if (!rv.second) {
-    // note, the second use of std:forward<V>(val) is only correct
-    // if the first one did not result in a move. obj[key] = val
-    // would be preferrable but doesn't compile because dynamic
-    // is (intentionally) not default constructable
-    rv.first->second = std::forward<V>(val);
-  }
+  auto rv = obj.insert({ std::forward<K>(key), nullptr });
+  rv.first->second = std::forward<V>(val);
 }
 
 inline std::size_t dynamic::erase(dynamic const& key) {

diff --git a/folly/test/DynamicTest.cpp b/folly/test/DynamicTest.cpp
index 4701a1bf93228de65840b769f223f4f353cb642f..ff38e338b7abea1b222777346cb14bbc169c1e82 100644 (file)
--- a/folly/test/DynamicTest.cpp
+++ b/folly/test/DynamicTest.cpp
@@ -84,6 +84,15 @@ TEST(Dynamic, ObjectBasics) {
   EXPECT_EQ(d3.at("123"), 42);
   EXPECT_EQ(d3.at(123), 321);
 
+  dynamic objInsert = folly::dynamic::object();
+  dynamic objA = folly::dynamic::object("1", "2");
+  dynamic objB = folly::dynamic::object("1", "2");
+
+  objInsert.insert("1", std::move(objA));
+  objInsert.insert("1", std::move(objB));
+
+  EXPECT_EQ(objInsert.find("1")->second.size(), 1);
+
   // We don't allow objects as keys in objects.
   EXPECT_ANY_THROW(newObject[d3] = 12);
 }