make pvlambda with node to be Serializable

Summary: now we could assume all nodes pass to pvlambda is a Serializable and they have the interface available Differential Revision: D67171982 fbshipit-source-id: 129256bf66c33d7c874effdad166e25bf9ff5d8a
facebook · Dec 20, 2024 · 6597dd8 · 6597dd8
1 parent 8a845b0
commit 6597dd8
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 65 deletions.
diff --git a/fboss/thrift_cow/nodes/Serializer.h b/fboss/thrift_cow/nodes/Serializer.h
@@ -236,9 +236,12 @@ struct Serializable {
   virtual folly::dynamic toFollyDynamic() const = 0;
 };
 
+struct ThriftObject {};
+
 template <typename TC, typename TType>
 class SerializableWrapper : public Serializable {
  public:
+  using CowType = ThriftObject;
   explicit SerializableWrapper(TType& node) : node_(node) {}
 
   folly::IOBuf encodeBuf(fsdb::OperProtocol proto) const override {

diff --git a/fboss/thrift_cow/nodes/ThriftStructNode-inl.h b/fboss/thrift_cow/nodes/ThriftStructNode-inl.h
@@ -600,13 +600,11 @@ class ThriftStructNode : public NodeBaseT<
           return;
         }
         auto tok = *begin;
-        if constexpr (thrift_cow::is_cow_type_v<decltype(node)>) {
-          if constexpr (std::is_same_v<
-                            typename folly::remove_cvref_t<
-                                decltype(node)>::CowType,
-                            NodeType>) {
-            node.modify(tok);
-          }
+        if constexpr (std::is_same_v<
+                          typename folly::remove_cvref_t<
+                              decltype(node)>::CowType,
+                          NodeType>) {
+          node.modify(tok);
         }
       });
 

diff --git a/fboss/thrift_cow/nodes/tests/ThriftStructNodeTests.cpp b/fboss/thrift_cow/nodes/tests/ThriftStructNodeTests.cpp
@@ -510,14 +510,7 @@ TYPED_TEST(ThriftStructNodeTestSuite, ThriftStructNodeModifyPathTest) {
   folly::dynamic dyn;
   auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
   // non-existent node
   auto visitResult = RootPathVisitor::visit(

diff --git a/fboss/thrift_cow/visitors/PathVisitor.h b/fboss/thrift_cow/visitors/PathVisitor.h
@@ -188,10 +188,22 @@ struct LambdaPathVisitorOperator {
 
   template <typename TC, typename Node>
   inline auto
-  visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end) {
+  visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end)
+    requires(is_cow_type_v<Node>)
+  {
     return f_(node, begin, end);
   }
 
+  template <typename TC, typename Node>
+  inline auto
+  visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end)
+    requires(!is_cow_type_v<Node>)
+  {
+    // Node is not a Serializable, dispatch with wrapper
+    SerializableWrapper<TC, Node> wrapper(node);
+    return f_(wrapper, begin, end);
+  }
+
  private:
   Func f_;
 };

diff --git a/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp b/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp
@@ -70,16 +70,9 @@ TYPED_TEST(PathVisitorTests, AccessField) {
 
   auto nodeA = this->initNode(structA);
   folly::dynamic dyn;
-  auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
+  auto processPath = pvlambda([&dyn](const auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
   std::vector<std::string> path{"inlineInt"};
   auto result = RootPathVisitor::visit(
@@ -145,16 +138,9 @@ TYPED_TEST(PathVisitorTests, AccessAtHybridNodeTest) {
 
   auto nodeA = this->initNode(root);
   folly::dynamic dyn;
-  auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
+  auto processPath = pvlambda([&dyn](Serializable& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
 
   // Thrift path terminating at HybridNode - Access
@@ -210,14 +196,7 @@ TYPED_TEST(PathVisitorTests, AccessAtHybridThriftContainerTest) {
   folly::dynamic dyn;
   auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
 
   // Thrift path at thrift container under HybridNode - Access
@@ -277,14 +256,7 @@ TYPED_TEST(PathVisitorTests, AccessAtHybridThriftContainerKeyTest) {
   folly::dynamic dyn;
   auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
 
   // Thrift path at thrift container key under HybridNode - Access
@@ -339,14 +311,7 @@ TYPED_TEST(PathVisitorTests, AccessFieldInContainer) {
   folly::dynamic dyn;
   auto processPath = pvlambda([&dyn](auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      dyn = node.toFollyDynamic();
-    } else {
-      facebook::thrift::to_dynamic(
-          dyn, node, facebook::thrift::dynamic_format::JSON_1);
-    }
+    dyn = node.toFollyDynamic();
   });
   std::vector<std::string> path{"mapOfEnumToStruct", "3"};
   auto result = RootPathVisitor::visit(
@@ -406,13 +371,7 @@ TEST(PathVisitorTests, AccessOptional) {
   std::string got;
   auto processPath = pvlambda([&got](auto& node, auto begin, auto end) {
     EXPECT_EQ(begin, end);
-    if constexpr (std::is_base_of_v<
-                      Serializable,
-                      std::remove_cvref_t<decltype(node)>>) {
-      got = node.toFollyDynamic().asString();
-    } else {
-      FAIL() << "unexpected non-cow visit";
-    }
+    got = node.toFollyDynamic().asString();
   });
   std::vector<std::string> path{"optionalString"};
   auto result = RootPathVisitor::visit(