Revert D68464053: Multisect successfully blamed "D68464053: [thrift][…

…patch] In DiffVisitor, if field exists in source, always ensure first" for one test failure

Summary:
This diff reverts D68464053
D68464053: [thrift][patch] In DiffVisitor, if field exists in source, always ensure first by Mizuchi causes the following test failure:

Tests affected:
- [cogwheel:cogwheel_controller_sizing#test_create_finalize](https://www.internalfb.com/intern/test/281475113955782/)

Here's the Multisect link:
https://www.internalfb.com/multisect/19963119
Here are the tasks that are relevant to this breakage:
T191387227: Some tests unhealthy for capacity_orchestration

The backout may land if someone accepts it.

If this diff has been generated in error, you can Commandeer and Abandon it.

Reviewed By: Mizuchi

Differential Revision: D68790038

fbshipit-source-id: dc37d0cca7b4c6768d1a883bd48a17ed380f70d9

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

@@ -17,11 +17,10 @@
 #include <gtest/gtest.h>
 #include <thrift/lib/cpp2/patch/DynamicPatch.h>
 #include <thrift/lib/cpp2/patch/detail/PatchBadge.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>
 
+#include <thrift/lib/cpp2/patch/test/gen-cpp2/gen_patch_DynamicPatchTest_types.h>
+
 namespace apache::thrift::protocol {
 using detail::badge;
 
@@ -1005,52 +1004,4 @@ 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

thrift/lib/thrift/detail/DynamicPatch.cpp

@@ -841,38 +841,6 @@ 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,
@@ -911,8 +879,7 @@ 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 ||
-        maybeEmptyDeprecatedTerseField(src.begin()->second)) {
+        src.begin()->first != dst.begin()->first) {
       DynamicUnknownPatch patch;
       patch.doNotConvertStringToBinary(badge);
       patch.assign(badge, dst);
@@ -1048,9 +1015,6 @@ 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)});