From 1f461539db6152d63352035ddfffebbac1f868ff Mon Sep 17 00:00:00 2001
From: TJ Yin <ytj@meta.com>
Date: Mon, 27 Jan 2025 19:02:28 -0800
Subject: [PATCH] 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: pranavtbhat

Differential Revision: D68464053

fbshipit-source-id: e0af57a3919a869084daee3eae08ed8ac5c487ac
---
 .../lib/cpp2/patch/test/DynamicPatchTest.cpp  | 53 ++++++++++++++++++-
 .../thrift/lib/thrift/detail/DynamicPatch.cpp | 38 ++++++++++++-
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp b/third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp
index 4cc04c148ff8d1..1a137f02fd0efd 100644
--- a/third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp
+++ b/third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp
@@ -17,9 +17,10 @@
 #include <gtest/gtest.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;
@@ -1004,4 +1005,52 @@ TEST(DynamicPatchTest, MergeMovedStructPatch) {
   testMergeMovedPatch<DynamicStructPatch>(obj);
 }
 
+TEST(DemoDiffVisitor, TerseWriteFieldMismatch1) {
+  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) {
+  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
diff --git a/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp b/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp
index f4f31508e907c6..413a848c472c35 100644
--- a/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp
+++ b/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp
@@ -841,6 +841,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,
@@ -879,7 +911,8 @@ DynamicPatch DiffVisitorBase::diffStructured(
   if (src.size() <= 1 && dst.size() <= 1) {
     // If same field is set, use normal Object diff logic.
     if (src.empty() || dst.empty() ||
-        src.begin()->first != dst.begin()->first) {
+        src.begin()->first != dst.begin()->first ||
+        maybeEmptyDeprecatedTerseField(src.begin()->second)) {
       DynamicUnknownPatch patch;
       patch.doNotConvertStringToBinary(badge);
       patch.assign(badge, dst);
@@ -1015,6 +1048,9 @@ void DiffVisitorBase::diffField(
 
   pushField(id);
   auto guard = folly::makeGuard([&] { pop(); });
+  if (maybeEmptyDeprecatedTerseField(src.at(id))) {
+    patch.ensure(badge, id, emptyValue(src.at(id).getType()));
+  }
   auto subPatch = diff(badge, src.at(id), dst.at(id));
   if (!subPatch.empty(badge)) {
     patch.patchIfSet(badge, id).merge(badge, DynamicPatch{std::move(subPatch)});