From 469296d2acc3bc34dba737cdbf2e1064d885551f Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Mon, 14 Oct 2024 15:06:44 -0400 Subject: [PATCH] Add `delete_reason` prop to deleted nodes' props maps This change adds a new `delete_reason` key to the `node_props()` map which is either `direct` or `keep_while`. This commit changes `khepri_tree` to add the `direct` delete reason for tree nodes which were deleted because they were the target of a delete command. The `keep_while` reason will be added in the child commit. The motivation to include a delete reason is that in the child commit we will include tree nodes deleted because their keep-while conditions became unsatisfied. `delete_reason` enables the caller to distinguish between tree nodes deleted "directly" (the target of the delete command) and the others which were expired. It also enables callers of put operations to distinguish between puts (tree node creations and updates) and deletions, since a put operation could also trigger a keep-while expiration. --- src/khepri.erl | 20 ++++++++++++++++++- src/khepri_node.hrl | 3 ++- src/khepri_tree.erl | 35 ++++++++++++++++++++++++++-------- test/advanced_delete.erl | 6 ++++-- test/advanced_tx_delete.erl | 6 ++++-- test/async_option.erl | 5 +++-- test/cluster_SUITE.erl | 3 ++- test/delete_command.erl | 24 +++++++++++++++-------- test/keep_while_conditions.erl | 14 +++++++++----- test/prop_state_machine.erl | 14 +++++++++++++- test/root_node.erl | 24 +++++++++++++++-------- 11 files changed, 115 insertions(+), 39 deletions(-) diff --git a/src/khepri.erl b/src/khepri.erl index 001acf33..d5a2a78f 100644 --- a/src/khepri.erl +++ b/src/khepri.erl @@ -203,6 +203,22 @@ -type child_list_length() :: non_neg_integer(). %% Number of direct child nodes under a tree node. +-type delete_reason() :: explicit | keep_while. +%% The reason why a tree node was removed from the store. +%% +%% + -type node_props() :: #{data => khepri:data(), has_data => boolean(), @@ -211,7 +227,8 @@ payload_version => khepri:payload_version(), child_list_version => khepri:child_list_version(), child_list_length => khepri:child_list_length(), - child_names => [khepri_path:node_id()]}. + child_names => [khepri_path:node_id()], + delete_reason => khepri:delete_reason()}. %% Structure used to return properties, payload and child nodes for a specific %% tree node. %% @@ -460,6 +477,7 @@ payload_version/0, child_list_version/0, child_list_length/0, + delete_reason/0, node_props/0, trigger_id/0, diff --git a/src/khepri_node.hrl b/src/khepri_node.hrl index df88ecb7..2c5ed6a1 100644 --- a/src/khepri_node.hrl +++ b/src/khepri_node.hrl @@ -16,7 +16,8 @@ child_list_version => ?INIT_CHILD_LIST_VERSION}). -define(DEFAULT_PROPS_TO_RETURN, [payload, - payload_version]). + payload_version, + delete_reason]). -record(node, {props = ?INIT_NODE_PROPS :: khepri_machine:props(), payload = ?NO_PAYLOAD :: khepri_payload:payload(), diff --git a/src/khepri_tree.erl b/src/khepri_tree.erl index 48f66c68..d9f3b07b 100644 --- a/src/khepri_tree.erl +++ b/src/khepri_tree.erl @@ -528,22 +528,41 @@ find_matching_nodes_cb(_, {interrupted, _, _}, _, Acc, _) -> delete_matching_nodes(Tree, PathPattern, AppliedChanges, TreeOptions) -> Fun = fun(Path, Node, Result) -> - delete_matching_nodes_cb(Path, Node, TreeOptions, Result) + delete_matching_nodes_cb( + Path, Node, TreeOptions, direct, Result) end, walk_down_the_tree( Tree, PathPattern, TreeOptions, AppliedChanges, Fun, #{}). -delete_matching_nodes_cb([] = Path, #node{} = Node, TreeOptions, Result) -> +delete_matching_nodes_cb( + [] = Path, #node{} = Node, TreeOptions, DeleteReason, Result) -> Node1 = remove_node_payload(Node), Node2 = remove_node_child_nodes(Node1), - NodeProps = gather_node_props(Node, TreeOptions), - {ok, Node2, Result#{Path => NodeProps}}; -delete_matching_nodes_cb(Path, #node{} = Node, TreeOptions, Result) -> - NodeProps = gather_node_props(Node, TreeOptions), - {ok, delete, Result#{Path => NodeProps}}; -delete_matching_nodes_cb(_, {interrupted, _, _}, _Options, Result) -> + NodeProps1 = gather_node_props(Node, TreeOptions), + NodeProps2 = maybe_add_delete_reason_prop( + NodeProps1, TreeOptions, DeleteReason), + {ok, Node2, Result#{Path => NodeProps2}}; +delete_matching_nodes_cb( + Path, #node{} = Node, TreeOptions, DeleteReason, Result) -> + NodeProps1 = gather_node_props(Node, TreeOptions), + NodeProps2 = maybe_add_delete_reason_prop( + NodeProps1, TreeOptions, DeleteReason), + {ok, delete, Result#{Path => NodeProps2}}; +delete_matching_nodes_cb( + _, {interrupted, _, _}, _Options, _DeleteReason, Result) -> {ok, keep, Result}. +maybe_add_delete_reason_prop( + NodeProps, #{props_to_return := WantedProps}, DeleteReason) -> + case lists:member(delete_reason, WantedProps) of + true -> + NodeProps#{delete_reason => DeleteReason}; + false -> + NodeProps + end; +maybe_add_delete_reason_prop(NodeProps, _TreeOptions, _DeleteReason) -> + NodeProps. + %% ------------------------------------------------------------------- %% Insert or update a tree node. %% ------------------------------------------------------------------- diff --git a/test/advanced_delete.erl b/test/advanced_delete.erl index 76d187e2..20ceaec0 100644 --- a/test/advanced_delete.erl +++ b/test/advanced_delete.erl @@ -36,7 +36,8 @@ delete_existing_node_test_() -> khepri_adv:create(?FUNCTION_NAME, [foo], foo_value)), ?_assertEqual( {ok, #{[foo] => #{data => foo_value, - payload_version => 1}}}, + payload_version => 1, + delete_reason => direct}}}, khepri_adv:delete(?FUNCTION_NAME, [foo])), ?_assertEqual( {error, ?khepri_error(node_not_found, #{node_name => foo, @@ -77,7 +78,8 @@ delete_many_on_existing_node_with_condition_true_test_() -> khepri_adv:create(?FUNCTION_NAME, [foo], foo_value)), ?_assertEqual( {ok, #{[foo] => #{data => foo_value, - payload_version => 1}}}, + payload_version => 1, + delete_reason => direct}}}, khepri_adv:delete_many( ?FUNCTION_NAME, [#if_name_matches{regex = "foo"}])), ?_assertEqual( diff --git a/test/advanced_tx_delete.erl b/test/advanced_tx_delete.erl index 483c4bbe..e4949a62 100644 --- a/test/advanced_tx_delete.erl +++ b/test/advanced_tx_delete.erl @@ -50,7 +50,8 @@ delete_existing_node_test_() -> ?_assertEqual( {ok, {ok, #{[foo] => #{data => foo_value, - payload_version => 1}}}}, + payload_version => 1, + delete_reason => direct}}}}, begin Fun = fun() -> khepri_tx_adv:delete([foo]) @@ -116,7 +117,8 @@ delete_many_on_existing_node_with_condition_true_test_() -> ?_assertEqual( {ok, {ok, #{[foo] => #{data => foo_value, - payload_version => 1}}}}, + payload_version => 1, + delete_reason => direct}}}}, begin Fun = fun() -> khepri_tx_adv:delete_many( diff --git a/test/async_option.erl b/test/async_option.erl index 3264fc96..92b410f9 100644 --- a/test/async_option.erl +++ b/test/async_option.erl @@ -230,7 +230,7 @@ async_with_correlation_in_delete_test_() -> ?FUNCTION_NAME, [foo], #{async => Correlation})), RaEvent = receive {ra_event, _, _} = Event -> Event end, ?assertEqual( - [{Correlation, {ok, #{[foo] => #{}}}}], + [{Correlation, {ok, #{[foo] => #{delete_reason => direct}}}}], khepri:handle_async_ret(?FUNCTION_NAME, RaEvent)), ?assertEqual( {error, @@ -289,7 +289,8 @@ async_with_correlation_and_priority_in_delete_test_() -> RaEvent = receive {ra_event, _, _} = Event -> Event end, ?assertEqual( [{Correlation, {ok, #{[foo] => - #{payload_version => 1}}}}], + #{payload_version => 1, + delete_reason => direct}}}}], khepri:handle_async_ret(?FUNCTION_NAME, RaEvent)), ?assertEqual( {error, diff --git a/test/cluster_SUITE.erl b/test/cluster_SUITE.erl index 0a413569..a596717e 100644 --- a/test/cluster_SUITE.erl +++ b/test/cluster_SUITE.erl @@ -1693,7 +1693,8 @@ can_use_default_store_on_single_node(_Config) -> {ok, #{[bar] => #{payload_version => 2}}}, khepri_adv:clear_many_payloads([bar])), ?assertEqual( - {ok, #{[bar] => #{payload_version => 2}}}, + {ok, #{[bar] => #{payload_version => 2, + delete_reason => direct}}}, khepri_adv:delete([bar])), ?assertMatch( {ok, #{}}, diff --git a/test/delete_command.erl b/test/delete_command.erl index 4bb9f0e3..2b393007 100644 --- a/test/delete_command.erl +++ b/test/delete_command.erl @@ -67,7 +67,8 @@ delete_existing_node_with_data_test() -> ?assertEqual({ok, #{[foo] => #{data => foo_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_existing_node_with_data_using_dot_test() -> @@ -92,7 +93,8 @@ delete_existing_node_with_data_using_dot_test() -> ?assertEqual({ok, #{[foo] => #{data => foo_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_existing_node_with_child_nodes_test() -> @@ -116,7 +118,8 @@ delete_existing_node_with_child_nodes_test() -> Root), ?assertEqual({ok, #{[foo] => #{payload_version => 1, child_list_version => 1, - child_list_length => 1}}}, Ret), + child_list_length => 1, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_a_node_deep_into_the_tree_test() -> @@ -140,7 +143,8 @@ delete_a_node_deep_into_the_tree_test() -> Root), ?assertEqual({ok, #{[foo, bar, baz] => #{payload_version => 1, child_list_version => 1, - child_list_length => 1}}}, Ret), + child_list_length => 1, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_existing_node_with_condition_true_test() -> @@ -170,7 +174,8 @@ delete_existing_node_with_condition_true_test() -> ?assertEqual({ok, #{[bar] => #{data => bar_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_existing_node_with_condition_false_test() -> @@ -230,7 +235,8 @@ delete_existing_node_with_condition_true_using_dot_test() -> ?assertEqual({ok, #{[bar] => #{data => bar_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_existing_node_with_condition_false_using_dot_test() -> @@ -292,11 +298,13 @@ delete_many_nodes_at_once_test() -> ?assertEqual({ok, #{[bar] => #{data => bar_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}, + child_list_length => 0, + delete_reason => direct}, [baz] => #{data => baz_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_command_bumps_applied_command_count_test() -> diff --git a/test/keep_while_conditions.erl b/test/keep_while_conditions.erl index 7dec366d..dcfc671b 100644 --- a/test/keep_while_conditions.erl +++ b/test/keep_while_conditions.erl @@ -292,7 +292,8 @@ recursive_automatic_cleanup_test() -> ?assertEqual({ok, #{[foo, bar, baz] => #{data => baz_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). keep_while_now_false_after_delete_command_test() -> @@ -322,7 +323,8 @@ keep_while_now_false_after_delete_command_test() -> ?assertEqual({ok, #{[foo] => #{data => foo_value, payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). automatic_reclaim_of_useless_nodes_works_test() -> @@ -340,7 +342,7 @@ automatic_reclaim_of_useless_nodes_works_test() -> child_list_version => 3}, child_nodes = #{}}, Root), - ?assertEqual({ok, #{[foo, bar, baz] => #{}}}, Ret), + ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct}}}, Ret), ?assertEqual([], SE). automatic_reclaim_keeps_relevant_nodes_1_test() -> @@ -368,7 +370,7 @@ automatic_reclaim_keeps_relevant_nodes_1_test() -> payload = khepri_payload:data(relevant), child_nodes = #{}}}}, Root), - ?assertEqual({ok, #{[foo, bar, baz] => #{}}}, Ret), + ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct}}}, Ret), ?assertEqual([], SE). automatic_reclaim_keeps_relevant_nodes_2_test() -> @@ -401,5 +403,7 @@ automatic_reclaim_keeps_relevant_nodes_2_test() -> payload = khepri_payload:data(bar_value), child_nodes = #{}}}}}}, Root), - ?assertEqual({ok, #{[foo, bar, baz, qux] => #{}}}, Ret), + ?assertEqual( + {ok, #{[foo, bar, baz, qux] => #{delete_reason => direct}}}, + Ret), ?assertEqual([], SE). diff --git a/test/prop_state_machine.erl b/test/prop_state_machine.erl index 965d5f35..8e575b14 100644 --- a/test/prop_state_machine.erl +++ b/test/prop_state_machine.erl @@ -102,7 +102,7 @@ postcondition( #state{entries = Entries}, {call, khepri_adv, delete_many, [_StoreId, Path, _Options]}, Result) -> - result_is_ok(Result, Entries, Path, {ok, #{}}). + result_is_ok_after_delete(Result, Entries, Path). add_entry(Entries, Path, Payload) -> {Entry1, New} = case Entries of @@ -183,6 +183,18 @@ result_is_ok(Result, Entries, Path, Default) -> Default =:= Result end. +result_is_ok_after_delete({ok, NodePropsMap}, Entries, Path) -> + case Entries of + #{Path := NodeProps} -> + DeletedProps = maps:get(Path, NodePropsMap, #{}), + DeletedProps1 = maps:remove(delete_reason, DeletedProps), + DeletedProps1 =:= NodeProps; + _ -> + NodePropsMap =:= #{} + end; +result_is_ok_after_delete(_, _Entries, _Path) -> + false. + result_is_ok_after_put(Result, Entries, Path, Payload, Default) -> case Entries of #{Path := #{data := Payload} = NodeProps} -> diff --git a/test/root_node.erl b/test/root_node.erl index 46fc290e..c510e7ef 100644 --- a/test/root_node.erl +++ b/test/root_node.erl @@ -325,7 +325,8 @@ delete_empty_root_node_test() -> Root), ?assertEqual({ok, #{[] => #{payload_version => 1, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_using_empty_path_test() -> @@ -349,7 +350,8 @@ delete_root_node_using_empty_path_test() -> ?assertEqual({ok, #{[] => #{data => value, payload_version => 2, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_using_root_test() -> @@ -373,7 +375,8 @@ delete_root_node_using_root_test() -> ?assertEqual({ok, #{[] => #{data => value, payload_version => 2, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_using_dot_test() -> @@ -397,7 +400,8 @@ delete_root_node_using_dot_test() -> ?assertEqual({ok, #{[] => #{data => value, payload_version => 2, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_using_dot_dot_test() -> @@ -421,7 +425,8 @@ delete_root_node_using_dot_dot_test() -> ?assertEqual({ok, #{[] => #{data => value, payload_version => 2, child_list_version => 1, - child_list_length => 0}}}, Ret), + child_list_length => 0, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_with_child_nodes_test() -> @@ -446,7 +451,8 @@ delete_root_node_with_child_nodes_test() -> Root), ?assertEqual({ok, #{[] => #{payload_version => 1, child_list_version => 3, - child_list_length => 2}}}, Ret), + child_list_length => 2, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_with_condition_true_test() -> @@ -471,7 +477,8 @@ delete_root_node_with_condition_true_test() -> Root), ?assertEqual({ok, #{[] => #{payload_version => 1, child_list_version => 2, - child_list_length => 1}}}, Ret), + child_list_length => 1, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_with_condition_true_using_dot_test() -> @@ -496,7 +503,8 @@ delete_root_node_with_condition_true_using_dot_test() -> Root), ?assertEqual({ok, #{[] => #{payload_version => 1, child_list_version => 2, - child_list_length => 1}}}, Ret), + child_list_length => 1, + delete_reason => direct}}}, Ret), ?assertEqual([], SE). delete_root_node_with_condition_false_test() ->