custom default values lost in DynamicPatch to TypedPatch conversion

Summary:
When converting a DynamicPatch to a TypedPatch, fields with custom default values (e.g., `bool enabled = true`) that were explicitly set to their intrinsic defaults (e.g., `false`) were incorrectly getting the custom default values instead.

Root cause: In `DiffVisitorBase::diffField()`, when a new struct field is added (exists in dst but not src), `emptyValue()` was used to create an empty Object `{}` for the ensure value. When this empty Object was later converted to a typed struct via `fromObjectStruct`, the typed struct was default-constructed with custom defaults, and the empty Object had no fields to override those defaults.

Fix: Store the actual destination field value in ensure instead of an empty Object. This ensures all field values are explicitly present when converting to typed patch, preventing custom defaults from incorrectly overriding intended values.

Reviewed By: Mizuchi

Differential Revision: D93497787

fbshipit-source-id: 246eb6efc35a900f5689ce8ce4f70ee6c0a0aab8

Author: Mohsen Akbari

third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.cpp

@@ -1787,4 +1787,68 @@ TEST(DynamicPatch, AnyPatchMergeThenExtractPreservesRootFields) {
   }
 }
 
+// Verifies that field values different from custom defaults are preserved
+// when converting DynamicPatch to TypedPatch via fromObjectStruct.
+TEST(DynamicPatchTest, CustomDefaultValuesPreservedInTypedPatchExtraction) {
+  OuterStructWithCustomDefaults src;
+  src.name() = "TestEntity";
+
+  OuterStructWithCustomDefaults dst;
+  dst.name() = "TestEntity";
+  CustomDefaultOptions opts;
+  opts.enabled() = false; // Custom default is true
+  opts.thresholdPercent() = 50; // Custom default is 90
+  opts.allowEmpty() = true; // Custom default is false
+  dst.options() = opts;
+
+  DiffVisitorBase visitor;
+  DynamicPatch dynamicPatch =
+      visitor.diff(protocol::asObject(src), protocol::asObject(dst));
+
+  auto typedPatch =
+      fromObjectStruct<type::infer_tag<OuterStructWithCustomDefaultsPatch>>(
+          dynamicPatch.toObject());
+
+  OuterStructWithCustomDefaults fresh;
+  fresh.name() = "TestEntity";
+  typedPatch.apply(fresh);
+
+  ASSERT_TRUE(fresh.options().has_value());
+  EXPECT_EQ(fresh.options()->enabled(), false);
+  EXPECT_EQ(fresh.options()->thresholdPercent(), 50);
+  EXPECT_EQ(fresh.options()->allowEmpty(), true);
+}
+
+// Verifies that intrinsic default values are preserved when they differ from
+// custom defaults during DynamicPatch to TypedPatch conversion.
+TEST(DynamicPatchTest, IntrinsicDefaultValuesPreservedWhenDifferFromCustom) {
+  OuterStructWithCustomDefaults src;
+  src.name() = "TestEntity";
+
+  OuterStructWithCustomDefaults dst;
+  dst.name() = "TestEntity";
+  CustomDefaultOptions opts;
+  opts.enabled() = false; // Intrinsic default, custom default is true
+  opts.thresholdPercent() = 0; // Intrinsic default, custom default is 90
+  opts.allowEmpty() = false;
+  dst.options() = opts;
+
+  DiffVisitorBase visitor;
+  DynamicPatch dynamicPatch =
+      visitor.diff(protocol::asObject(src), protocol::asObject(dst));
+
+  auto typedPatch =
+      fromObjectStruct<type::infer_tag<OuterStructWithCustomDefaultsPatch>>(
+          dynamicPatch.toObject());
+
+  OuterStructWithCustomDefaults fresh;
+  fresh.name() = "TestEntity";
+  typedPatch.apply(fresh);
+
+  ASSERT_TRUE(fresh.options().has_value());
+  EXPECT_EQ(fresh.options()->enabled(), false);
+  EXPECT_EQ(fresh.options()->thresholdPercent(), 0);
+  EXPECT_EQ(fresh.options()->allowEmpty(), false);
+}
+
 } // namespace apache::thrift::protocol

third-party/thrift/src/thrift/lib/cpp2/patch/test/DynamicPatchTest.thrift

@@ -41,3 +41,17 @@ struct Sets {
 struct StructWithAny {
   1: any.Any any;
 }
+
+// Structs for testing custom default value preservation in patch conversion
+@patch.GeneratePatchNew
+struct CustomDefaultOptions {
+  1: bool enabled = true;
+  2: i32 thresholdPercent = 90;
+  3: bool allowEmpty = false;
+}
+
+@patch.GeneratePatchNew
+struct OuterStructWithCustomDefaults {
+  1: string name;
+  2: optional CustomDefaultOptions options;
+}

third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.cpp

@@ -1554,13 +1554,16 @@ void DiffVisitorBase::diffField(
     auto guard = folly::makeGuard([&] { pop(); });
     auto& field = dst.at(id);
 
-    // patch after
-    // We can't ensure the field because it will be ignored for non-optional
-    // field when applied statically.
+    // For non-optional fields (like in terse structs), ensure may be ignored
+    // during typed patch application. Use ensure + patchIfSet with Assign
+    // operation to make sure the value is actually set.
     auto empty = emptyValue(field.getType());
-    auto subPatch = diff(badge, empty, field);
     patch.ensure(id, std::move(empty));
-    patch.patchIfSet(id).merge(DynamicPatch{std::move(subPatch)});
+
+    Object assignPatch;
+    assignPatch[static_cast<FieldId>(op::PatchOp::Assign)] = field;
+    patch.patchIfSet(id).merge(
+        DynamicPatch::fromObject(std::move(assignPatch)));
     return;
   }