Skip to content

Commit

Permalink
PathVisitor: add hybrid test for access/set/get for thrift container …
Browse files Browse the repository at this point in the history 
…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/<key>, structMap/<key>/<field>, listOfStruct<key>/<field>)
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
  • Loading branch information
@facebook-github-bot
Wei-Cheng Lin authored and facebook-github-bot committed Dec 10, 2024
1 parent 5cabb73 commit 69a3bc7
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 19 deletions.
36 changes: 18 additions & 18 deletions fboss/thrift_cow/visitors/PathVisitor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,8 @@ using PathIter = typename std::vector<std::string>::const_iterator;
// instantiations
class BasePathVisitorOperator {
public:
template <typename TType>
template <typename TC, typename TType>
class SerializableWrapper : public Serializable {
using TC = apache::thrift::type_class::structure;

public:
explicit SerializableWrapper(TType& node) : node_(node) {}

Expand Down Expand Up @@ -68,7 +66,7 @@ class BasePathVisitorOperator {

virtual ~BasePathVisitorOperator() = default;

template <typename Node>
template <typename TC, typename Node>
inline void
visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end)
requires(is_cow_type_v<Node>)
Expand All @@ -82,13 +80,13 @@ class BasePathVisitorOperator {
}
}

template <typename Node>
template <typename TC, typename Node>
inline void
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 wrapper(node);
SerializableWrapper<TC, Node> wrapper(node);
if constexpr (std::is_const_v<Node>) {
cvisit(wrapper, begin, end);
cvisit(wrapper);
Expand Down Expand Up @@ -221,7 +219,7 @@ template <typename Func>
struct LambdaPathVisitorOperator {
explicit LambdaPathVisitorOperator(Func&& f) : f_(std::forward<Func>(f)) {}

template <typename Node>
template <typename TC, typename Node>
inline auto
visitTyped(Node& node, pv_detail::PathIter begin, pv_detail::PathIter end)
-> std::invoke_result_t<
Expand All @@ -232,7 +230,7 @@ struct LambdaPathVisitorOperator {
return f_(node, begin, end);
}

template <typename Node>
template <typename TC, typename Node>
inline auto visitTyped(
Node& node,
pv_detail::PathIter /* begin */,
Expand All @@ -252,7 +250,7 @@ visitNode(Node& node, const VisitImplParams<Op>& params, PathIter cursor)
{
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
try {
params.op.visitTyped(node, cursor, params.end);
params.op.template visitTyped<TC, Node>(node, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -295,7 +293,7 @@ struct PathVisitorImpl<apache::thrift::type_class::set<ValueTypeClass>> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(tObj, cursor, params.end);
params.op.template visitTyped<TC, Obj>(tObj, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -334,7 +332,7 @@ struct PathVisitorImpl<apache::thrift::type_class::set<ValueTypeClass>> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(node, cursor, params.end);
params.op.template visitTyped<TC, Node>(node, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -414,7 +412,7 @@ struct PathVisitorImpl<apache::thrift::type_class::list<ValueTypeClass>> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(tObj, cursor, params.end);
params.op.template visitTyped<TC, Obj>(tObj, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -443,7 +441,7 @@ struct PathVisitorImpl<apache::thrift::type_class::list<ValueTypeClass>> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(node, cursor, params.end);
params.op.template visitTyped<TC, Node>(node, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -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<TC, Obj>(tObj, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -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<TC, Node>(node, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -698,7 +696,7 @@ struct PathVisitorImpl<apache::thrift::type_class::structure> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(node, cursor, params.end);
params.op.template visitTyped<TC, Node>(node, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -737,7 +735,7 @@ struct PathVisitorImpl<apache::thrift::type_class::structure> {
{
try {
if (params.mode == PathVisitMode::FULL || cursor == params.end) {
params.op.visitTyped(tObj, cursor, params.end);
params.op.template visitTyped<TC, Obj>(tObj, cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
}
Expand Down Expand Up @@ -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<TC>,
std::remove_const_t<Node>>(
*const_cast<std::remove_const_t<Node>*>(&node), cursor, params.end);
if (cursor == params.end) {
return ThriftTraverseResult::OK;
Expand Down
67 changes: 67 additions & 0 deletions fboss/thrift_cow/visitors/tests/PathVisitorTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<decltype(node)>>) {
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<std::string> 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<cfg::L4PortRange>(
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<std::string, cfg::L4PortRange> newMap;
cfg::L4PortRange newRange;
newRange.min() = 3000;
newRange.max() = 4000;
newMap.emplace("200", std::move(newRange));
folly::fbstring newVal =
serialize<TC>(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<ThriftStructNode<
Expand Down
3 changes: 2 additions & 1 deletion fboss/thrift_cow/visitors/tests/VisitorTestUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ TestStruct createSimpleTestStruct() {
20,
dynamic::object("childMap", dynamic::object())("leafI32", 0)(
"listOfStruct", dynamic::array())("strMap", dynamic::object())(
"structMap", dynamic::object())))(
"structMap",
dynamic::object("30", dynamic::object("min", 100)("max", 200)))))(
"hybridMapOfMap", dynamic::object(10, dynamic::object(20, 30)))(
"hybridStruct",
dynamic::object(
Expand Down

0 comments on commit 69a3bc7

Please sign in to comment.