Allow the search hook to be present at any position in list of precom…

…mits

bz1216

Also, ensure that any duplicate search hooks are stripped because the
fixup no longer blindly removes the search hook and then reinstalls it.

src/riak_search_kv_hook.erl

@@ -50,8 +50,14 @@
 fixup(Bucket, BucketProps) ->
     case proplists:get_value(search, BucketProps) of
         true ->
-            CleanPrecommit = strip_precommit(BucketProps),
-            UpdPrecommit = CleanPrecommit ++ [precommit_def()],
+            CurrentPrecommit = get_precommit(BucketProps),
+
+            UpdPrecommit = case has_search_precommit(BucketProps) of
+                false ->
+                    CurrentPrecommit ++ [precommit_def()];
+                _ ->
+                    CurrentPrecommit
+            end,
 
             %% Update the bucket properties
             {ok, lists:keystore(precommit, 1, BucketProps, 
@@ -68,13 +74,7 @@ fixup(Bucket, BucketProps) ->
             %% check if the hook is present.
             %% we don't do this on the default bucket because we don't want to
             %% inherit the search parameter.
-            Precommit = case proplists:get_value(precommit, BucketProps) of
-                undefined -> [];
-                {struct, _} = X -> [X];
-                X when is_list(X) -> X
-            end,
-
-            case lists:member(precommit_def(), Precommit) of
+            case has_search_precommit(BucketProps) of
                 true ->
                     {ok, [{search, true}|BucketProps]};
                 false ->
@@ -231,15 +231,41 @@ run_extract(RiakObject, DefaultField, {F, A}) ->
 %% existing index hooks.
 strip_precommit(BucketProps) ->
     %% Get the current precommit hook
-    case proplists:get_value(precommit, BucketProps, []) of
+    CurrentPrecommit = get_precommit(BucketProps),
+    %% Add kv/search hook - make sure there are not duplicate entries
+    CurrentPrecommit -- [precommit_def()].
+
+%% Check is the precommit is already installed
+has_search_precommit(BucketProps) ->
+    Precommit = get_precommit(BucketProps),
+    lists:member(precommit_def(), Precommit).
+
+get_precommit(BucketProps) ->
+    Precommit = case proplists:get_value(precommit, BucketProps, []) of
         X when is_list(X) ->
-            CurrentPrecommit=X;
+            X;
         {struct, _}=X ->
-            CurrentPrecommit=[X]
+            [X]
     end,
 
-    %% Add kv/search hook - make sure there are not duplicate entries
-    CurrentPrecommit -- [precommit_def()].
+    %% strip out any duplicate search hooks
+    Count = lists:foldl(fun(E, Acc) ->
+                case E == precommit_def() of
+                    true ->
+                        Acc +1;
+                    _ ->
+                        Acc
+                end
+        end, 0, Precommit),
+
+    case Count > 1 of
+        true ->
+            %% more than one precommit found, remove all but one
+            Precommit -- lists:duplicate(Count - 1, precommit_def());
+        _ ->
+            Precommit
+    end.
+
 
 -ifdef(TEST).
 

test/fixup_test.erl

@@ -32,7 +32,9 @@ fixup_test_() ->
             fun rolling_upgrade/0,
             fun other_precommit_hook/0,
             fun install_uninstall/0,
-            fun blank_bucket/0
+            fun blank_bucket/0,
+            fun custom_hook_order/0,
+            fun duplicate_hook/0
         ]
     }.
 
@@ -134,6 +136,43 @@ blank_bucket() ->
         proplists:get_value(precommit, Props3)),
     ok.
 
+custom_hook_order() ->
+    riak_core_bucket:set_bucket("testbucket", [{search, true}]),
+    riak_core_bucket:set_bucket("testbucket", [{precommit,
+                [my_precommit_def(), riak_search_kv_hook:precommit_def()]}]),
+    Props = riak_core_bucket:get_bucket("testbucket"),
+    ?assertEqual([my_precommit_def(), riak_search_kv_hook:precommit_def()],
+        proplists:get_value(precommit, Props)),
+    riak_core_bucket:set_bucket("testbucket", [{precommit,
+                [riak_search_kv_hook:precommit_def(), my_precommit_def()]}]),
+    Props2 = riak_core_bucket:get_bucket("testbucket"),
+    ?assertEqual([riak_search_kv_hook:precommit_def(), my_precommit_def()],
+        proplists:get_value(precommit, Props2)),
+    ok.
+
+duplicate_hook() ->
+    riak_core_bucket:set_bucket("testbucket", [{search, true}]),
+    riak_core_bucket:set_bucket("testbucket", [{precommit,
+                [riak_search_kv_hook:precommit_def(),
+                    my_precommit_def(), riak_search_kv_hook:precommit_def()]}]),
+    Props = riak_core_bucket:get_bucket("testbucket"),
+    ?assertEqual([my_precommit_def(), riak_search_kv_hook:precommit_def()],
+        proplists:get_value(precommit, Props)),
+    riak_core_bucket:set_bucket("testbucket", [{precommit,
+                [riak_search_kv_hook:precommit_def(),
+                    riak_search_kv_hook:precommit_def(),
+                    riak_search_kv_hook:precommit_def(),
+                    my_precommit_def(),
+                    riak_search_kv_hook:precommit_def(),
+                    riak_search_kv_hook:precommit_def(),
+                    riak_search_kv_hook:precommit_def()]
+            }]),
+    Props2 = riak_core_bucket:get_bucket("testbucket"),
+    ?assertEqual([my_precommit_def(), riak_search_kv_hook:precommit_def()],
+        proplists:get_value(precommit, Props2)),
+    ok.
+
+
 my_precommit_def() ->
     {struct, [{<<"mod">>,atom_to_binary(?MODULE, latin1)},
               {<<"fun">>,<<"precommit">>}]}.