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 4c58f964990eb7..a7f172d23bb16c 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 @@ -15,14 +15,18 @@ */ #include +#include #include #include -#include - #include +#include +#include +#include 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(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>(src).as_object(); + auto dstObj = protocol::asValueStruct>(dst).as_object(); + + DemoDiffVisitor visitor; + auto patch = visitor.diff(srcObj, dstObj); + + auto srcBuf = CompactSerializer::serialize(src).move(); + auto dstBuf = CompactSerializer::serialize(dst).move(); + + auto srcVal = protocol::parseValue( + *srcBuf, type::BaseType::Struct); + auto dstVal = protocol::parseValue( + *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(src).move(); + auto dstBuf = CompactSerializer::serialize(dst).move(); + + auto srcObj = protocol::parseObject(*srcBuf); + auto dstObj = protocol::parseObject(*dstBuf); + + DemoDiffVisitor visitor; + auto patch = visitor.diff(srcObj, dstObj); + + auto srcVal = protocol::asValueStruct>(src); + auto dstVal = protocol::asValueStruct>(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 00cf10f8dc05fa..138b69a890654d 100644 --- a/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp +++ b/third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp @@ -15,6 +15,7 @@ */ #include +#include #include #include #include @@ -23,6 +24,9 @@ #include 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("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)}); } }