From 69a3bc797ccc0f8c575ebc54ee6daca1255bb6fa Mon Sep 17 00:00:00 2001 From: Wei-Cheng Lin Date: Tue, 10 Dec 2024 15:49:31 -0800 Subject: [PATCH] PathVisitor: add hybrid test for access/set/get for thrift container under hybrid node Summary: # Overview Using new defined test struct, adding test case for accessing with path visitor with the case of thrift path terminating at hybrid node Of the overall test cases listed below, this covers the combo of 2b x 3c,3d,3e D66525279 Update test struct definitions for comprehensive coverage of hybrid node access patterns for all visitors: a. Thrift path terminating at HybridNode (annotated member hybridMapOfI32ToStruct) b. Thrift path terminating at Thrift container (list, map, set, struct field in ChildStruct) under HybridNode c. Thrift path terminating at leaf primitive under HybridNode (primitive leaves under ChildStruct: leafEnum, optionalI32, childMap/, structMap//, listOfStruct/) d. [Existing UTs already cover] paths that terminate in COW leaf, COW nodes of various TCs Path Visitor UTs to cover following operations on access patterns in #2 above: a. modifyPath b. removePath c. Set d. Get e. Access (folly::dynamic) ref: https://fburl.com/gdoc/46krq4xp # Fix Originally, the serializeWrapper assumes TC to be of type structure. This diff removes that and passes TC type down from visitTyped method. Differential Revision: D67038112 fbshipit-source-id: f06ba71e6145b7a27c2139db09fd22bf0e961e0a --- fboss/thrift_cow/visitors/PathVisitor.h | 36 +++++----- .../visitors/tests/PathVisitorTests.cpp | 67 +++++++++++++++++++ .../visitors/tests/VisitorTestUtils.cpp | 3 +- 3 files changed, 87 insertions(+), 19 deletions(-) diff --git a/fboss/thrift_cow/visitors/PathVisitor.h b/fboss/thrift_cow/visitors/PathVisitor.h index 05f6221cbf1f1..48ed8c968debd 100644 --- a/fboss/thrift_cow/visitors/PathVisitor.h +++ b/fboss/thrift_cow/visitors/PathVisitor.h @@ -31,10 +31,8 @@ using PathIter = typename std::vector::const_iterator; // instantiations class BasePathVisitorOperator { public: - template + template class SerializableWrapper : public Serializable { - using TC = apache::thrift::type_class::structure; - public: explicit SerializableWrapper(TType& node) : node_(node) {} @@ -68,7 +66,7 @@ class BasePathVisitorOperator { virtual ~BasePathVisitorOperator() = default; - template + template inline void visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end) requires(is_cow_type_v) @@ -82,13 +80,13 @@ class BasePathVisitorOperator { } } - template + template inline void visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end) requires(!is_cow_type_v) { // Node is not a Serializable, dispatch with wrapper - SerializableWrapper wrapper(node); + SerializableWrapper wrapper(node); if constexpr (std::is_const_v) { cvisit(wrapper, begin, end); cvisit(wrapper); @@ -221,7 +219,7 @@ template struct LambdaPathVisitorOperator { explicit LambdaPathVisitorOperator(Func&& f) : f_(std::forward(f)) {} - template + template inline auto visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end) -> std::invoke_result_t< @@ -232,7 +230,7 @@ struct LambdaPathVisitorOperator { return f_(node, begin, end); } - template + template inline auto visitTyped( Node& node, pv_detail::PathIter /* begin */, @@ -252,7 +250,7 @@ visitNode(Node& node, const VisitImplParams& params, PathIter cursor) { if (params.mode == PathVisitMode::FULL || cursor == params.end) { try { - params.op.visitTyped(node, cursor, params.end); + params.op.template visitTyped(node, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -295,7 +293,7 @@ struct PathVisitorImpl> { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(tObj, cursor, params.end); + params.op.template visitTyped(tObj, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -334,7 +332,7 @@ struct PathVisitorImpl> { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(node, cursor, params.end); + params.op.template visitTyped(node, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -414,7 +412,7 @@ struct PathVisitorImpl> { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(tObj, cursor, params.end); + params.op.template visitTyped(tObj, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -443,7 +441,7 @@ struct PathVisitorImpl> { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(node, cursor, params.end); + params.op.template visitTyped(node, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -515,7 +513,7 @@ struct PathVisitorImpl< { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(tObj, cursor, params.end); + params.op.template visitTyped(tObj, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -547,7 +545,7 @@ struct PathVisitorImpl< { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(node, cursor, params.end); + params.op.template visitTyped(node, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -698,7 +696,7 @@ struct PathVisitorImpl { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(node, cursor, params.end); + params.op.template visitTyped(node, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -737,7 +735,7 @@ struct PathVisitorImpl { { try { if (params.mode == PathVisitMode::FULL || cursor == params.end) { - params.op.visitTyped(tObj, cursor, params.end); + params.op.template visitTyped(tObj, cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; } @@ -835,7 +833,9 @@ struct PathVisitorImpl { // take a const param. Here we cast away the const and rely on // primitive node's functions throwing an exception if the node is // immutable. - params.op.visitTyped( + params.op.template visitTyped< + std::remove_const_t, + std::remove_const_t>( *const_cast*>(&node), cursor, params.end); if (cursor == params.end) { return ThriftTraverseResult::OK; diff --git a/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp b/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp index ec90d0b549e76..c7f3af8de70de 100644 --- a/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp +++ b/fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp @@ -199,6 +199,73 @@ TYPED_TEST(PathVisitorTests, AccessAtHybridNodeTest) { EXPECT_EQ(getOp.val, newVal); } +TYPED_TEST(PathVisitorTests, AccessAtHybridThriftContainerTest) { + RootTestStruct root; + ParentTestStruct parent; + auto testStruct = createSimpleTestStruct(); + parent.mapOfI32ToMapOfStruct() = {{3, {{"4", std::move(testStruct)}}}}; + root.mapOfI32ToMapOfStruct() = {{1, {{"2", std::move(parent)}}}}; + + auto nodeA = this->initNode(root); + 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>) { + dyn = node.toFollyDynamic(); + } else { + facebook::thrift::to_dynamic( + dyn, node, facebook::thrift::dynamic_format::JSON_1); + } + }); + + // Thrift path at thrift container under HybridNode - Access + std::vector path{ + "mapOfI32ToMapOfStruct", + "1", + "2", + "mapOfI32ToMapOfStruct", + "3", + "4", + "hybridMapOfI32ToStruct", + "20", + "structMap"}; + auto result = RootPathVisitor::visit( + *nodeA, path.begin(), path.end(), PathVisitMode::LEAF, processPath); + EXPECT_EQ(result, ThriftTraverseResult::OK); + EXPECT_NE(dyn.find("30"), dyn.items().end()); + cfg::L4PortRange got = facebook::thrift::from_dynamic( + dyn["30"], facebook::thrift::dynamic_format::JSON_1); + EXPECT_EQ(*got.min(), 100); + EXPECT_EQ(*got.max(), 200); + + // Thrift path at thrift container under HybridNode - Set + using TC = apache::thrift::type_class::map< + apache::thrift::type_class::string, + apache::thrift::type_class::structure>; + + std::map newMap; + cfg::L4PortRange newRange; + newRange.min() = 3000; + newRange.max() = 4000; + newMap.emplace("200", std::move(newRange)); + folly::fbstring newVal = + serialize(fsdb::OperProtocol::SIMPLE_JSON, newMap); + SetEncodedPathVisitorOperator setOp(fsdb::OperProtocol::SIMPLE_JSON, newVal); + + result = RootPathVisitor::visit( + *nodeA, path.begin(), path.end(), PathVisitMode::LEAF, setOp); + EXPECT_EQ(result, ThriftTraverseResult::OK); + + // Thrift path at thrift container under HybridNode - Get + GetEncodedPathVisitorOperator getOp(fsdb::OperProtocol::SIMPLE_JSON); + result = RootPathVisitor::visit( + *nodeA, path.begin(), path.end(), PathVisitMode::LEAF, getOp); + EXPECT_EQ(result, ThriftTraverseResult::OK); + EXPECT_EQ(getOp.val, newVal); +} + TYPED_TEST(PathVisitorTests, AccessFieldInContainer) { auto structA = createSimpleTestStruct(); auto nodeA = std::make_shared