In DiffVisitor, if field exists in source, always ensure first

Summary: Field could be `deprecated_terse_writes`. If we don't ensure, it might not exist when calling diff.

Reviewed By: thedavekwon

Differential Revision: D68867578

fbshipit-source-id: 6ba5547d2fbbbefa0eb7391e7a0d6b2fa141bcb8

thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp

@@ -15,14 +15,18 @@
  */
 
 #include <gtest/gtest.h>
+#include <thrift/lib/cpp2/Flags.h>
 #include <thrift/lib/cpp2/patch/DynamicPatch.h>
 #include <thrift/lib/cpp2/patch/detail/PatchBadge.h>
-#include <thrift/lib/thrift/gen-cpp2/any_patch_types.h>
-
 #include <thrift/lib/cpp2/patch/test/gen-cpp2/gen_patch_DynamicPatchTest_types.h>
+#include <thrift/lib/cpp2/patch/test/gen-cpp2/gen_patch_OldTerseWrite_types.h>
+#include <thrift/lib/cpp2/protocol/Serializer.h>
+#include <thrift/lib/thrift/gen-cpp2/any_patch_types.h>
 
 namespace apache::thrift::protocol {
 using detail::badge;
+THRIFT_FLAG_DECLARE_bool(
+    thrift_patch_diff_visitor_ensure_on_potential_terse_write_field);
 
 class DemoDiffVisitor : public DiffVisitorBase {
  public:
@@ -1006,4 +1010,56 @@ TEST(DynamicPatchTest, MergeMovedStructPatch) {
   testMergeMovedPatch<DynamicStructPatch>(obj);
 }
 
+TEST(DemoDiffVisitor, TerseWriteFieldMismatch1) {
+  THRIFT_FLAG_SET_MOCK(
+      thrift_patch_diff_visitor_ensure_on_potential_terse_write_field, true);
+  using test::Foo;
+  Foo src, dst;
+  dst.bar() = "123";
+
+  // Field exists when generating the patch but not when applying the diff
+  auto srcObj = protocol::asValueStruct<type::struct_t<Foo>>(src).as_object();
+  auto dstObj = protocol::asValueStruct<type::struct_t<Foo>>(dst).as_object();
+
+  DemoDiffVisitor visitor;
+  auto patch = visitor.diff(srcObj, dstObj);
+
+  auto srcBuf = CompactSerializer::serialize<folly::IOBufQueue>(src).move();
+  auto dstBuf = CompactSerializer::serialize<folly::IOBufQueue>(dst).move();
+
+  auto srcVal = protocol::parseValue<CompactProtocolReader>(
+      *srcBuf, type::BaseType::Struct);
+  auto dstVal = protocol::parseValue<CompactProtocolReader>(
+      *dstBuf, type::BaseType::Struct);
+
+  protocol::applyPatch(patch.toObject(), srcVal);
+
+  EXPECT_EQ(srcVal, dstVal);
+}
+
+TEST(DemoDiffVisitor, TerseWriteFieldMismatch2) {
+  THRIFT_FLAG_SET_MOCK(
+      thrift_patch_diff_visitor_ensure_on_potential_terse_write_field, true);
+  using test::Foo;
+  Foo src, dst;
+  dst.bar() = "123";
+
+  // Field exists when applying the patch but not when generating the diff
+  auto srcBuf = CompactSerializer::serialize<folly::IOBufQueue>(src).move();
+  auto dstBuf = CompactSerializer::serialize<folly::IOBufQueue>(dst).move();
+
+  auto srcObj = protocol::parseObject<CompactProtocolReader>(*srcBuf);
+  auto dstObj = protocol::parseObject<CompactProtocolReader>(*dstBuf);
+
+  DemoDiffVisitor visitor;
+  auto patch = visitor.diff(srcObj, dstObj);
+
+  auto srcVal = protocol::asValueStruct<type::struct_t<Foo>>(src);
+  auto dstVal = protocol::asValueStruct<type::struct_t<Foo>>(dst);
+
+  protocol::applyPatch(patch.toObject(), srcVal);
+
+  EXPECT_EQ(srcVal, dstVal);
+}
+
 } // namespace apache::thrift::protocol

thrift/lib/thrift/detail/DynamicPatch.cpp

@@ -15,6 +15,7 @@
  */
 
 #include <folly/Overload.h>
+#include <thrift/lib/cpp2/Flags.h>
 #include <thrift/lib/cpp2/op/Clear.h>
 #include <thrift/lib/cpp2/op/Patch.h>
 #include <thrift/lib/thrift/detail/DynamicPatch.h>
@@ -23,6 +24,9 @@
 #include <thrift/lib/cpp2/patch/detail/PatchBadge.h>
 
 namespace apache::thrift::protocol {
+THRIFT_FLAG_DEFINE_bool(
+    thrift_patch_diff_visitor_ensure_on_potential_terse_write_field, false);
+
 using detail::badge;
 using detail::ValueList;
 using detail::ValueMap;
@@ -841,6 +845,38 @@ DynamicMapPatch DiffVisitorBase::diffMap(
   return patch;
 }
 
+// Check whether value should not be serialized due to deprecated_terse_writes,
+// but serialized in languages other than C++.
+static bool maybeEmptyDeprecatedTerseField(const Value& value) {
+  switch (value.getType()) {
+    case Value::Type::boolValue:
+    case Value::Type::byteValue:
+    case Value::Type::i16Value:
+    case Value::Type::i32Value:
+    case Value::Type::i64Value:
+    case Value::Type::floatValue:
+    case Value::Type::doubleValue:
+      // Numeric fields maybe empty terse field regardless the value, since it
+      // honors custom default
+      return true;
+    case Value::Type::stringValue:
+    case Value::Type::binaryValue:
+    case Value::Type::listValue:
+    case Value::Type::setValue:
+    case Value::Type::mapValue:
+      // string/binary and containers fields don't honor custom default.
+      // It can only be empty if it is intrinsic default
+      return protocol::isIntrinsicDefault(value);
+    case Value::Type::objectValue:
+      // struct/union/exception can never be empty terse field
+      return false;
+    case Value::Type::__EMPTY__:
+      folly::throw_exception<std::runtime_error>("value is empty.");
+    default:
+      notImpl();
+  }
+}
+
 void DiffVisitorBase::diffElement(
     const ValueMap& src,
     const ValueMap& dst,
@@ -889,6 +925,16 @@ DynamicPatch DiffVisitorBase::diffStructured(
     bool shouldUseAssignPatch =
         src.empty() || dst.empty() || src.begin()->first != dst.begin()->first;
 
+    if (THRIFT_FLAG(
+            thrift_patch_diff_visitor_ensure_on_potential_terse_write_field)) {
+      // If field is src looks like deprecated terse field, we need to use
+      // EnsureStruct, which is not supported by UnionPatch, thus we need to use
+      // AssignPatch.
+      shouldUseAssignPatch = shouldUseAssignPatch ||
+          (src.begin()->second != dst.begin()->second &&
+           maybeEmptyDeprecatedTerseField(src.begin()->second));
+    }
+
     if (shouldUseAssignPatch) {
       DynamicUnknownPatch patch;
       patch.doNotConvertStringToBinary(badge);
@@ -1027,6 +1073,11 @@ void DiffVisitorBase::diffField(
   auto guard = folly::makeGuard([&] { pop(); });
   auto subPatch = diff(badge, src.at(id), dst.at(id));
   if (!subPatch.empty(badge)) {
+    if (THRIFT_FLAG(
+            thrift_patch_diff_visitor_ensure_on_potential_terse_write_field) &&
+        maybeEmptyDeprecatedTerseField(src.at(id))) {
+      patch.ensure(badge, id, emptyValue(src.at(id).getType()));
+    }
     patch.patchIfSet(badge, id).merge(badge, DynamicPatch{std::move(subPatch)});
   }
 }