Fix bug in BinaryPatch::apply(std::string)

Summary:
facepalm

Currently applies to an empty string and replace the original value, which drops the data in the original value.

Unfortunately, we only had unit-test for assign operation, so we did not catch this issue.

Reviewed By: thedavekwon

Differential Revision: D63547473

fbshipit-source-id: 96324cc3a2533a945a6e7273f9303b4d48e8a92f

thrift/lib/cpp2/op/detail/ValuePatch.h

@@ -370,23 +370,25 @@ class BinaryPatch : public BaseStringPatch<Patch, BinaryPatch<Patch>> {
   using Base = BaseStringPatch<Patch, BinaryPatch>;
   using T = typename Base::value_type;
 
- public:
-  using Base::apply;
-  using Base::assign;
-  using Base::Base;
-
-  void assign(std::string s) {
+  static folly::IOBuf stringToIobuf(std::string s) {
     std::string* p = new std::string(std::move(s));
-    assign(folly::IOBuf(
+    return folly::IOBuf(
         folly::IOBuf::TAKE_OWNERSHIP,
         p->data(),
         p->size(),
         [](void*, void* userData) {
           delete static_cast<std::string*>(userData);
         },
-        static_cast<void*>(p)));
+        static_cast<void*>(p));
   }
 
+ public:
+  using Base::apply;
+  using Base::assign;
+  using Base::Base;
+
+  void assign(std::string s) { assign(stringToIobuf(std::move(s))); }
+
   template <typename T>
   BinaryPatch& operator=(T&& other) {
     return assign(std::forward<T>(other)), *this;
@@ -433,7 +435,7 @@ class BinaryPatch : public BaseStringPatch<Patch, BinaryPatch<Patch>> {
   }
 
   void apply(std::string& val) const {
-    folly::IOBuf buf;
+    folly::IOBuf buf = stringToIobuf(std::move(val));
     apply(buf);
     val = buf.to<std::string>();
   }