From 95b0ef5606c3e62c89bc6a13eedd6a68f451fde4 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Mon, 14 Oct 2024 15:59:38 -0400
Subject: [PATCH] khepri_tree: Accumulate keep-while expirations

With this change any tree nodes removed from the tree because their
keep-while condition became unsatisfied are accumulated in the return
value of the put or delete operation which caused their deletion. These
expired tree nodes have props maps in the return map with the
`delete_reason` key set to `keep_while`. This enables callers of delete
operations to distinguish between deletes which were the target of the
delete command (`direct`) and expired nodes (`keep_while`). This also
enables callers of put operations to distinguish between creates or
updates and deletions.

This change only affects functions which return the
`khepri_adv:node_props_map()` - functions from the `khepri_adv` and
`khepri_adv_tx` modules. Previously the node props map did not include
the tree nodes removed because of expired keep-while conditions.
---
 src/khepri_adv.erl             | 66 ++++++++++++++++++++++++----------
 src/khepri_tree.erl            | 31 ++++++++++++----
 test/delete_command.erl        | 10 +++++-
 test/keep_while_conditions.erl | 32 +++++++++++++----
 4 files changed, 107 insertions(+), 32 deletions(-)

diff --git a/src/khepri_adv.erl b/src/khepri_adv.erl
index d98d9080..71065ef2 100644
--- a/src/khepri_adv.erl
+++ b/src/khepri_adv.erl
@@ -332,11 +332,18 @@ put(StoreId, PathPattern, Data) ->
 %% in the path pattern is not met, an error is returned and the tree structure
 %% is not modified.
 %%
-%% The returned `{ok, NodeProps}' tuple contains a map with the properties and
-%% payload (if any) of the targeted tree node: the payload was the one before
-%% the update, other properties like the payload version correspond to the
-%% updated node. If the targeted tree node didn't exist, `NodeProps' will be
-%% an empty map.
+%% The returned `{ok, NodePropsMap}' tuple contains a map where keys correspond
+%% to the path to a node affected by the put operation. Each key points to a
+%% map containing the properties and prior payload (if any) of a tree node
+%% created, updated or deleted by the put operation. If the put results in the
+%% creation of a tree node this props map will be empty. If the put updates an
+%% existing tree node then the props map will contain the payload of the tree
+%% node (if any) before the update while the other properties like the payload
+%% version correspond to the updated node. The `NodePropsMap' map might also
+%% contain deletions if the put operation leads to an existing tree node's
+%% keep-while condition becoming unsatisfied. The props map for any nodes
+%% deleted because of an expired keep-while condition will contain a
+%% `delete_reason' key set to `keep_while'.
 %%
 %% The payload must be one of the following form:
 %% <ul>
@@ -452,11 +459,18 @@ put_many(StoreId, PathPattern, Data) ->
 %% in the path pattern is not met, an error is returned and the tree structure
 %% is not modified.
 %%
-%% The returned `{ok, NodePropsMap}' tuple contains a map where keys
-%% correspond to the path to a tree node matching the path pattern. Each key
-%% then points to a map containing the properties and payload (if any) of the
-%% targeted tree node: the payload was the one before the update, other
-%% properties like the payload version correspond to the updated node.
+%% The returned `{ok, NodePropsMap}' tuple contains a map where keys correspond
+%% to the path to a node affected by the put operation. Each key points to a
+%% map containing the properties and prior payload (if any) of a tree node
+%% created, updated or deleted by the put operation. If the put results in the
+%% creation of a tree node this props map will be empty. If the put updates an
+%% existing tree node then the props map will contain the payload of the tree
+%% node (if any) before the update while the other properties like the payload
+%% version correspond to the updated node. The `NodePropsMap' map might also
+%% contain deletions if the put operation leads to an existing tree node's
+%% keep-while condition becoming unsatisfied. The props map for any nodes
+%% deleted because of an expired keep-while condition will contain a
+%% `delete_reason' key set to `keep_while'.
 %%
 %% The payload must be one of the following form:
 %% <ul>
@@ -831,9 +845,15 @@ delete(PathPattern, Options) when is_map(Options) ->
 %% one tree node would match at the time. If you want to delete multiple nodes
 %% at once, use {@link delete_many/3}.
 %%
-%% The returned `{ok, NodeProps}' tuple contains a map with the properties and
-%% payload (if any) of the targeted tree node as they were before the delete.
-%% If the targeted tree node didn't exist, `NodeProps' will be an empty map.
+%% The returned `{ok, NodePropsMap}' tuple contains a map where keys
+%% correspond to the path to a deleted tree node. Each key then points to a
+%% map containing the properties and payload (if any) of that deleted tree
+%% node as they were before the delete. Tree nodes in this map which were
+%% the target of the delete command will have a `delete_reason' key set to
+%% `direct' in their associated props map. Any tree nodes deleted because their
+%% keep-while condition became unsatisfied due to the deletion will have the
+%% `delete_reason' key set to `keep_while' instead. (See {@link
+%% khepri_condition:keep_while()}.)
 %%
 %% When doing an asynchronous update, the {@link handle_async_ret/1}
 %% function should be used to handle the message received from Ra.
@@ -842,7 +862,8 @@ delete(PathPattern, Options) when is_map(Options) ->
 %% ```
 %% %% Delete the tree node at `/:foo/:bar'.
 %% {ok, #{data := value,
-%%        payload_version := 1}} = khepri_adv:delete(StoreId, [foo, bar]).
+%%        payload_version := 1,
+%%        delete_reason := direct}} = khepri_adv:delete(StoreId, [foo, bar]).
 %% '''
 %%
 %% @param StoreId the name of the Khepri store.
@@ -925,18 +946,25 @@ delete_many(PathPattern, Options) when is_map(Options) ->
 %% The returned `{ok, NodePropsMap}' tuple contains a map where keys
 %% correspond to the path to a deleted tree node. Each key then points to a
 %% map containing the properties and payload (if any) of that deleted tree
-%% node as they were before the delete.
+%% node as they were before the delete. Tree nodes in this map which were
+%% the target of the delete command will have a `delete_reason' key set to
+%% `direct' in their associated props map. Any tree nodes deleted because their
+%% keep-while condition became unsatisfied due to the deletion will have the
+%% `delete_reason' key set to `keep_while' instead. (See {@link
+%% khepri_condition:keep_while()}.)
 %%
 %% When doing an asynchronous update, the {@link handle_async_ret/1}
 %% function should be used to handle the message received from Ra.
 %%
 %% Example:
 %% ```
-%% %% Delete the tree node at `/:foo/:bar'.
+%% %% Delete all tree nodes matching `/*/:bar'.
 %% {ok, #{[foo, bar] := #{data := value,
-%%                        payload_version := 1},
-%%        [baz, bar] := #{payload_version := 1}}} = khepri_adv:delete_many(
-%%                                                    StoreId, [foo, bar]).
+%%                        payload_version := 1,
+%%                        delete_reason := direct},
+%%        [baz, bar] := #{payload_version := 1,
+%%                        delete_reason := direct}}} =
+%% khepri_adv:delete_many(StoreId, [?KHEPRI_WILDCARD_STAR, bar]).
 %% '''
 %%
 %% @param StoreId the name of the Khepri store.
diff --git a/src/khepri_tree.erl b/src/khepri_tree.erl
index d9f3b07b..362e6261 100644
--- a/src/khepri_tree.erl
+++ b/src/khepri_tree.erl
@@ -1341,7 +1341,9 @@ handle_keep_while_for_parent_update(
 handle_keep_while_for_parent_update(
   #walk{tree = Tree,
         node = ParentNode,
-        reversed_path = ReversedPath} = Walk,
+        reversed_path = ReversedPath,
+        tree_options = TreeOptions,
+        fun_acc = Acc} = Walk,
   AppliedChangesAcc) ->
     ParentPath = lists:reverse(ReversedPath),
     IsMet = is_keep_while_condition_met_on_self(
@@ -1354,7 +1356,11 @@ handle_keep_while_for_parent_update(
             %% This parent node must be removed because it doesn't meet its
             %% own keep_while condition. keep_while conditions for nodes
             %% depending on this one will be evaluated with the recursion.
-            Walk1 = Walk#walk{node = delete},
+            {ok, delete, Acc1} = delete_matching_nodes_cb(
+                                   ParentPath, ParentNode,
+                                   TreeOptions, keep_while, Acc),
+            Walk1 = Walk#walk{node = delete,
+                              fun_acc = Acc1},
             walk_back_up_the_tree(Walk1, AppliedChangesAcc)
     end.
 
@@ -1538,14 +1544,27 @@ remove_expired_nodes([], Walk) ->
     {ok, Walk};
 remove_expired_nodes(
   [PathToDelete | Rest],
-  #walk{tree = Tree, applied_changes = AppliedChanges} = Walk) ->
-    case delete_matching_nodes(Tree, PathToDelete, AppliedChanges, #{}) of
-        {ok, Tree1, AppliedChanges1, _Acc} ->
+  #walk{tree = Tree,
+        applied_changes = AppliedChanges,
+        tree_options = TreeOptions,
+        fun_acc = Acc} = Walk) ->
+    %% See `delete_matching_nodes/4'. This is the same except that the
+    %% accumulator is passed through and the `DeleteReason' is provided as
+    %% `keep_while'.
+    Fun = fun(Path, Node, Result) ->
+                  delete_matching_nodes_cb(
+                    Path, Node, TreeOptions, keep_while, Result)
+          end,
+    Result = walk_down_the_tree(
+               Tree, PathToDelete, TreeOptions, AppliedChanges, Fun, Acc),
+    case Result of
+        {ok, Tree1, AppliedChanges1, Acc1} ->
             AppliedChanges2 = merge_applied_changes(
                                 AppliedChanges, AppliedChanges1),
             Walk1 = Walk#walk{tree = Tree1,
                               node = Tree1#tree.root,
-                              applied_changes = AppliedChanges2},
+                              applied_changes = AppliedChanges2,
+                              fun_acc = Acc1},
             remove_expired_nodes(Rest, Walk1)
     end.
 
diff --git a/test/delete_command.erl b/test/delete_command.erl
index 2b393007..b2df6bb7 100644
--- a/test/delete_command.erl
+++ b/test/delete_command.erl
@@ -144,7 +144,15 @@ delete_a_node_deep_into_the_tree_test() ->
     ?assertEqual({ok, #{[foo, bar, baz] => #{payload_version => 1,
                                              child_list_version => 1,
                                              child_list_length => 1,
-                                             delete_reason => direct}}}, Ret),
+                                             delete_reason => direct},
+                        [foo, bar] => #{payload_version => 1,
+                                        child_list_version => 2,
+                                        child_list_length => 0,
+                                        delete_reason => keep_while},
+                        [foo] => #{payload_version => 1,
+                                   child_list_version => 2,
+                                   child_list_length => 0,
+                                   delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 delete_existing_node_with_condition_true_test() ->
diff --git a/test/keep_while_conditions.erl b/test/keep_while_conditions.erl
index dcfc671b..23f72cda 100644
--- a/test/keep_while_conditions.erl
+++ b/test/keep_while_conditions.erl
@@ -258,7 +258,8 @@ keep_while_now_false_after_command_test() ->
                  #node{props = ?INIT_NODE_PROPS,
                        payload = khepri_payload:data(bar_value)}}}}},
        Root),
-    ?assertEqual({ok, #{[foo, bar] => #{}}}, Ret),
+    ?assertEqual({ok, #{[foo, bar] => #{},
+                        [baz] => #{delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 recursive_automatic_cleanup_test() ->
@@ -293,7 +294,17 @@ recursive_automatic_cleanup_test() ->
                                              payload_version => 1,
                                              child_list_version => 1,
                                              child_list_length => 0,
-                                             delete_reason => direct}}}, Ret),
+                                             delete_reason => direct},
+                        [foo, bar] => #{data => bar_value,
+                                        payload_version => 1,
+                                        child_list_version => 3,
+                                        child_list_length => 0,
+                                        delete_reason => keep_while},
+                        [foo] => #{data => foo_value,
+                                   payload_version => 1,
+                                   child_list_version => 3,
+                                   child_list_length => 0,
+                                   delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 keep_while_now_false_after_delete_command_test() ->
@@ -324,7 +335,12 @@ keep_while_now_false_after_delete_command_test() ->
                                    payload_version => 1,
                                    child_list_version => 1,
                                    child_list_length => 0,
-                                   delete_reason => direct}}}, Ret),
+                                   delete_reason => direct},
+                        [baz] => #{data => baz_value,
+                                   payload_version => 1,
+                                   child_list_version => 1,
+                                   child_list_length => 0,
+                                   delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 automatic_reclaim_of_useless_nodes_works_test() ->
@@ -342,7 +358,9 @@ automatic_reclaim_of_useless_nodes_works_test() ->
             child_list_version => 3},
           child_nodes = #{}},
        Root),
-    ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct}}}, Ret),
+    ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct},
+                        [foo, bar] => #{delete_reason => keep_while},
+                        [foo] => #{delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 automatic_reclaim_keeps_relevant_nodes_1_test() ->
@@ -370,7 +388,8 @@ automatic_reclaim_keeps_relevant_nodes_1_test() ->
                   payload = khepri_payload:data(relevant),
                   child_nodes = #{}}}},
        Root),
-    ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct}}}, Ret),
+    ?assertEqual({ok, #{[foo, bar, baz] => #{delete_reason => direct},
+                        [foo, bar] => #{delete_reason => keep_while}}}, Ret),
     ?assertEqual([], SE).
 
 automatic_reclaim_keeps_relevant_nodes_2_test() ->
@@ -404,6 +423,7 @@ automatic_reclaim_keeps_relevant_nodes_2_test() ->
                           child_nodes = #{}}}}}},
        Root),
     ?assertEqual(
-      {ok, #{[foo, bar, baz, qux] => #{delete_reason => direct}}},
+      {ok, #{[foo, bar, baz, qux] => #{delete_reason => direct},
+             [foo, bar, baz] => #{delete_reason => keep_while}}},
       Ret),
     ?assertEqual([], SE).