incorporate qa feedback. The version.erl api is rather nice now: the version itself is entirely opaque - whilst it can be read, there's nothing provided to decompose it at all.

3 files changed, 139 insertions, 146 deletions

diff --git a/src/rabbit_mnesia.erl b/src/rabbit_mnesia.erl
index fa442c9c03..4902cfebdf 100644
--- a/src/rabbit_mnesia.erl
+++ b/src/rabbit_mnesia.erl
@@ -457,8 +457,7 @@ init_db(ClusterNodes, Force) ->
                         %% If we're just starting up a new node we won't have
                         %% a version
                         version_not_available ->
-                            ok = rabbit_version:write(
-                                   rabbit_upgrade:desired_version())
+                            ok = rabbit_version:write_desired_version()
                     end,
                     ensure_schema_integrity()
             end;
@@ -485,14 +484,14 @@ schema_ok_or_move() ->
     end.
 
 ensure_version_ok({ok, DiscVersion}) ->
-    DesiredVersion = rabbit_upgrade:desired_version(),
+    DesiredVersion = rabbit_version:desired_version(),
     case rabbit_version:'=~='(DesiredVersion, DiscVersion) of
         true  -> ok;
         false -> throw({error, {schema_mismatch,
                                 DesiredVersion, DiscVersion}})
     end;
 ensure_version_ok({error, _}) ->
-    ok = rabbit_version:write(rabbit_upgrade:desired_version()).
+    ok = rabbit_version:write_desired_version().
 
 create_schema() ->
     mnesia:stop(),
@@ -502,7 +501,7 @@ create_schema() ->
                           cannot_start_mnesia),
     ok = create_tables(),
     ok = ensure_schema_integrity(),
-    ok = rabbit_version:write(rabbit_upgrade:desired_version()).
+    ok = rabbit_version:write_desired_version().
 
 move_db() ->
     mnesia:stop(),
diff --git a/src/rabbit_upgrade.erl b/src/rabbit_upgrade.erl
index 9b2ffa28d0..9347cc5367 100644
--- a/src/rabbit_upgrade.erl
+++ b/src/rabbit_upgrade.erl
@@ -17,13 +17,11 @@
 -module(rabbit_upgrade).
 
 -export([maybe_upgrade_mnesia/0, maybe_upgrade_local/0]).
--export([desired_version/0]).
 
 -include("rabbit.hrl").
 
 -define(VERSION_FILENAME, "schema_version").
 -define(LOCK_FILENAME, "schema_upgrade_lock").
--define(SCOPES, [mnesia, local]).
 
 %% -------------------------------------------------------------------
 
@@ -31,7 +29,6 @@
 
 -spec(maybe_upgrade_mnesia/0 :: () -> 'ok').
 -spec(maybe_upgrade_local/0 :: () -> 'ok' | 'version_not_available').
--spec(desired_version/0 :: () -> rabbit_version:version()).
 
 -endif.
 
@@ -96,8 +93,8 @@
 
 maybe_upgrade_mnesia() ->
     AllNodes = rabbit_mnesia:all_clustered_nodes(),
-    case upgrades_required(mnesia) of
-        version_not_available ->
+    case rabbit_version:upgrades_required(mnesia) of
+        {error, version_not_available} ->
             rabbit:prepare(), %% Ensure we have logs for this
             case AllNodes of
                 [_] -> ok;
@@ -105,9 +102,11 @@ maybe_upgrade_mnesia() ->
                            "< 2.1.1.~nUnfortunately you will need to "
                            "rebuild the cluster.", [])
             end;
-        [] ->
+        {error, _} = Err ->
+            throw(Err);
+        {ok, []} ->
             ok;
-        Upgrades ->
+        {ok, Upgrades} ->
             rabbit:prepare(), %% Ensure we have logs for this
             case upgrade_mode(AllNodes) of
                 primary   -> primary_upgrade(Upgrades, AllNodes);
@@ -142,18 +141,19 @@ upgrade_mode(AllNodes) ->
             end;
         [Another|_] ->
             ClusterVersion =
-                case rpc:call(Another,
-                              rabbit_upgrade, desired_version, [mnesia]) of
+                case rpc:call(Another, rabbit_version, desired_scope_version,
+                              [mnesia]) of
                     {badrpc, {'EXIT', {undef, _}}} -> unknown_old_version;
                     {badrpc, Reason}               -> {unknown, Reason};
                     V                              -> V
                 end,
-            case desired_version(mnesia) of
-                ClusterVersion ->
+            MyVersion = rabbit_version:desired_scope_version(mnesia),
+            case rabbit_version:'=~='(ClusterVersion, MyVersion) of
+                true ->
                     %% The other node(s) have upgraded already, I am not the
                     %% upgrader
                     secondary;
-                MyVersion ->
+                false ->
                     %% The other node(s) are running an unexpected version.
                     die("Cluster upgrade needed but other nodes are "
                         "running ~p~nand I want ~p",
@@ -208,7 +208,7 @@ secondary_upgrade(AllNodes) ->
                    end,
     rabbit_misc:ensure_ok(mnesia:start(), cannot_start_mnesia),
     ok = rabbit_mnesia:init_db(ClusterNodes, true),
-    ok = write_desired_scope_version(mnesia),
+    ok = rabbit_version:write_desired_scope_version(mnesia),
     ok.
 
 nodes_running(Nodes) ->
@@ -223,90 +223,14 @@ node_running(Node) ->
 %% -------------------------------------------------------------------
 
 maybe_upgrade_local() ->
-    case upgrades_required(local) of
-        version_not_available -> version_not_available;
-        []                    -> ok;
-        Upgrades              -> apply_upgrades(local, Upgrades,
-                                                fun() -> ok end)
+    case rabbit_version:upgrades_required(local) of
+        {error, version_not_available} -> version_not_available;
+        {error, _} = Err               -> throw(Err);
+        {ok, []}                       -> ok;
+        {ok, Upgrades}                 -> apply_upgrades(local, Upgrades,
+                                                         fun () -> ok end)
     end.
 
-desired_version() -> [{Scope, desired_version(Scope)} || Scope <- ?SCOPES].
-
-desired_version(Scope) -> with_upgrade_graph(fun (G) -> heads(G) end, Scope).
-
-write_desired_scope_version(Scope) ->
-    ok = rabbit_version:with_scope_version(
-           Scope,
-           fun ({error, Error}) ->
-                   throw({error, {cannot_read_version_to_write_it, Error}})
-           end,
-           fun (_SV) -> {desired_version(Scope), ok} end).
-
-%% -------------------------------------------------------------------
-
-upgrades_required(Scope) ->
-    rabbit_version:with_scope_version(
-      Scope,
-      fun ({error, enoent}) -> version_not_available end,
-      fun (CurrentHeads) ->
-              {CurrentHeads,
-               with_upgrade_graph(
-                 fun (G) ->
-                         case unknown_heads(CurrentHeads, G) of
-                             [] ->
-                                 upgrades_to_apply(CurrentHeads, G);
-                             Unknown ->
-                                 throw({error,
-                                        {future_upgrades_found, Unknown}})
-                         end
-                 end, Scope)}
-      end).
-
-with_upgrade_graph(Fun, Scope) ->
-    case rabbit_misc:build_acyclic_graph(
-           fun (Module, Steps) -> vertices(Module, Steps, Scope) end,
-           fun (Module, Steps) -> edges(Module, Steps, Scope) end,
-           rabbit_misc:all_module_attributes(rabbit_upgrade)) of
-        {ok, G} -> try
-                       Fun(G)
-                   after
-                       true = digraph:delete(G)
-                   end;
-        {error, {vertex, duplicate, StepName}} ->
-            throw({error, {duplicate_upgrade_step, StepName}});
-        {error, {edge, {bad_vertex, StepName}, _From, _To}} ->
-            throw({error, {dependency_on_unknown_upgrade_step, StepName}});
-        {error, {edge, {bad_edge, StepNames}, _From, _To}} ->
-            throw({error, {cycle_in_upgrade_steps, StepNames}})
-    end.
-
-vertices(Module, Steps, Scope0) ->
-    [{StepName, {Module, StepName}} || {StepName, Scope1, _Reqs} <- Steps,
-                                       Scope0 == Scope1].
-
-edges(_Module, Steps, Scope0) ->
-    [{Require, StepName} || {StepName, Scope1, Requires} <- Steps,
-                            Require <- Requires,
-                            Scope0 == Scope1].
-unknown_heads(Heads, G) ->
-    [H || H <- Heads, digraph:vertex(G, H) =:= false].
-
-upgrades_to_apply(Heads, G) ->
-    %% Take all the vertices which can reach the known heads. That's
-    %% everything we've already applied. Subtract that from all
-    %% vertices: that's what we have to apply.
-    Unsorted = sets:to_list(
-                 sets:subtract(
-                   sets:from_list(digraph:vertices(G)),
-                   sets:from_list(digraph_utils:reaching(Heads, G)))),
-    %% Form a subgraph from that list and find a topological ordering
-    %% so we can invoke them in order.
-    [element(2, digraph:vertex(G, StepName)) ||
-        StepName <- digraph_utils:topsort(digraph_utils:subgraph(G, Unsorted))].
-
-heads(G) ->
-    lists:sort([V || V <- digraph:vertices(G), digraph:out_degree(G, V) =:= 0]).
-
 %% -------------------------------------------------------------------
 
 apply_upgrades(Scope, Upgrades, Fun) ->
@@ -329,7 +253,7 @@ apply_upgrades(Scope, Upgrades, Fun) ->
                     [apply_upgrade(Scope, Upgrade) || Upgrade <- Upgrades],
                     info("~s upgrades: All upgrades applied successfully~n",
                          [Scope]),
-                    ok = write_desired_scope_version(Scope),
+                    ok = rabbit_version:write_desired_scope_version(Scope),
                     ok = rabbit_misc:recursive_delete([BackupDir]),
                     info("~s upgrades: Mnesia backup removed~n", [Scope]),
                     ok = file:delete(LockFile);
diff --git a/src/rabbit_version.erl b/src/rabbit_version.erl
index 8c577f9c37..2d7ba8e431 100644
--- a/src/rabbit_version.erl
+++ b/src/rabbit_version.erl
@@ -16,86 +16,156 @@
 
 -module(rabbit_version).
 
--export([read/0, write/1, with_scope_version/3, '=~='/2]).
+-export([read/0, '=~='/2, desired_version/0, desired_scope_version/1,
+         write_desired_version/0, write_desired_scope_version/1,
+         upgrades_required/1]).
 
 %% -------------------------------------------------------------------
 -ifdef(use_specs).
 
--export_type([step/0, version/0, scope/0]).
+-export_type([scope/0, step/0, scope_version/0]).
 
 -type(scope() :: atom()).
--type(step() :: atom()).
--type(version() :: [{scope(), [step()]}]).
+-type(scope_version() :: [atom()]).
+-type(step() :: {atom(), atom()}).
+
+-type(version() :: [atom()]).
 
 -spec(read/0 :: () -> rabbit_types:ok_or_error2(version(), any())).
--spec(write/1 :: (version()) -> 'ok').
--spec(with_scope_version/3 ::
-        (scope(),
-         fun (({'error', any()}) -> E),
-         fun (([step()]) -> {[step()], A})) -> E | A).
 -spec('=~='/2 :: (version(), version()) -> boolean()).
+-spec(desired_version/0 :: () -> version()).
+-spec(desired_scope_version/1 :: (scope()) -> scope_version()).
+-spec(write_desired_version/0 :: () -> 'ok').
+-spec(write_desired_scope_version/1 ::
+        (scope()) -> rabbit_types:ok_or_error(any())).
+-spec(upgrades_required/1 ::
+        (scope()) -> rabbit_types:ok_or_error2([step()], any())).
 
 -endif.
 %% -------------------------------------------------------------------
 
 -define(VERSION_FILENAME, "schema_version").
+-define(SCOPES, [mnesia, local]).
 
 %% -------------------------------------------------------------------
 
-read() ->
-    case rabbit_misc:read_term_file(schema_filename()) of
-        {ok, [V]}        -> {ok, categorise_by_scope(V)};
-        {error, _} = Err -> Err
-    end.
+read() -> case rabbit_misc:read_term_file(schema_filename()) of
+              {ok, [V]}        -> {ok, V};
+              {error, _} = Err -> Err
+          end.
 
-write(Version) ->
-    V = [Name || {_Scope, Names} <- Version, Name <- Names],
-    ok = rabbit_misc:write_term_file(schema_filename(), [V]).
+write(V) -> ok = rabbit_misc:write_term_file(schema_filename(), [V]).
 
-with_scope_version(Scope, ErrorHandler, Fun) ->
+read_scope_version(Scope) ->
     case read() of
         {error, _} = Err ->
-            ErrorHandler(Err);
+            Err;
         {ok, Version} ->
-            SV = case lists:keysearch(Scope, 1, Version) of
+            {ok, case lists:keysearch(Scope, 1, categorise_by_scope(Version)) of
                      false                 -> [];
                      {value, {Scope, SV1}} -> SV1
-                 end,
-            {SV2, Result} = Fun(SV),
-            ok = case SV =:= SV2 of
-                     true  -> ok;
-                     false -> write(lists:keystore(Scope, 1, Version,
-                                                   {Scope, SV2}))
-                 end,
-            Result
+                 end}
     end.
 
+write_scope_version(Scope, ScopeVersion) ->
+    case read() of
+        {error, _} = Err ->
+            Err;
+        {ok, Version} ->
+            Version1 = lists:keystore(Scope, 1, categorise_by_scope(Version),
+                                      {Scope, ScopeVersion}),
+            ok = write([Name || {_Scope, Names} <- Version1, Name <- Names])
+    end.
+
+%% -------------------------------------------------------------------
+
 '=~='(VerA, VerB) ->
-    matches(lists:usort(VerA), lists:usort(VerB)).
+    lists:usort(VerA) =:= lists:usort(VerB).
+
+%% -------------------------------------------------------------------
+
+desired_version() ->
+    [Name || Scope <- ?SCOPES, Name <- desired_scope_version(Scope)].
+
+desired_scope_version(Scope) -> with_upgrade_graph(fun heads/1, Scope).
+
+write_desired_version() -> write(desired_version()).
+
+write_desired_scope_version(Scope) ->
+    write_scope_version(Scope, desired_scope_version(Scope)).
+
+upgrades_required(Scope) ->
+    case read_scope_version(Scope) of
+        {error, enoent} ->
+            {error, version_not_available};
+        {ok, CurrentHeads} ->
+            with_upgrade_graph(
+              fun (G) ->
+                      case unknown_heads(CurrentHeads, G) of
+                          []      -> {ok, upgrades_to_apply(CurrentHeads, G)};
+                          Unknown -> {error, {future_upgrades_found, Unknown}}
+                      end
+              end, Scope)
+    end.
+
+%% -------------------------------------------------------------------
+
+with_upgrade_graph(Fun, Scope) ->
+    case rabbit_misc:build_acyclic_graph(
+           fun (Module, Steps) -> vertices(Module, Steps, Scope) end,
+           fun (Module, Steps) -> edges(Module, Steps, Scope) end,
+           rabbit_misc:all_module_attributes(rabbit_upgrade)) of
+        {ok, G} -> try
+                       Fun(G)
+                   after
+                       true = digraph:delete(G)
+                   end;
+        {error, {vertex, duplicate, StepName}} ->
+            throw({error, {duplicate_upgrade_step, StepName}});
+        {error, {edge, {bad_vertex, StepName}, _From, _To}} ->
+            throw({error, {dependency_on_unknown_upgrade_step, StepName}});
+        {error, {edge, {bad_edge, StepNames}, _From, _To}} ->
+            throw({error, {cycle_in_upgrade_steps, StepNames}})
+    end.
+
+vertices(Module, Steps, Scope0) ->
+    [{StepName, {Module, StepName}} || {StepName, Scope1, _Reqs} <- Steps,
+                                       Scope0 == Scope1].
+
+edges(_Module, Steps, Scope0) ->
+    [{Require, StepName} || {StepName, Scope1, Requires} <- Steps,
+                            Require <- Requires,
+                            Scope0 == Scope1].
+unknown_heads(Heads, G) ->
+    [H || H <- Heads, digraph:vertex(G, H) =:= false].
+
+upgrades_to_apply(Heads, G) ->
+    %% Take all the vertices which can reach the known heads. That's
+    %% everything we've already applied. Subtract that from all
+    %% vertices: that's what we have to apply.
+    Unsorted = sets:to_list(
+                 sets:subtract(
+                   sets:from_list(digraph:vertices(G)),
+                   sets:from_list(digraph_utils:reaching(Heads, G)))),
+    %% Form a subgraph from that list and find a topological ordering
+    %% so we can invoke them in order.
+    [element(2, digraph:vertex(G, StepName)) ||
+        StepName <- digraph_utils:topsort(digraph_utils:subgraph(G, Unsorted))].
+
+heads(G) ->
+    lists:sort([V || V <- digraph:vertices(G), digraph:out_degree(G, V) =:= 0]).
 
 %% -------------------------------------------------------------------
 
-matches([], []) ->
-    true;
-matches([{Scope, SV}|VerA], [{Scope, SV}|VerB]) ->
-    matches(VerA, VerB);
-matches([{Scope, SVA}|VerA], [{Scope, SVB}|VerB]) ->
-    case {lists:usort(SVA), lists:usort(SVB)} of
-        {SV, SV} -> matches(VerA, VerB);
-        _        -> false
-    end;
-matches(_VerA, _VerB) ->
-    false.
-
-categorise_by_scope(Heads) when is_list(Heads) ->
+categorise_by_scope(Version) when is_list(Version) ->
     Categorised =
         [{Scope, Name} || {_Module, Attributes} <-
                               rabbit_misc:all_module_attributes(rabbit_upgrade),
                           {Name, Scope, _Requires} <- Attributes,
-                          lists:member(Name, Heads)],
+                          lists:member(Name, Version)],
     orddict:to_list(
-      lists:foldl(fun ({Scope, Name}, Version) ->
-                          rabbit_misc:orddict_cons(Scope, Name, Version)
+      lists:foldl(fun ({Scope, Name}, CatVersion) ->
+                          rabbit_misc:orddict_cons(Scope, Name, CatVersion)
                   end, orddict:new(), Categorised)).
 
 dir() -> rabbit_mnesia:dir().