From 8113709f55b29db11860133a53ba02b19a61f107 Mon Sep 17 00:00:00 2001 From: Rob Hanlon Date: Tue, 22 Sep 2020 16:35:04 -0700 Subject: [PATCH] Order transitive dependencies Currently, transitive dependencies are being started in the order they were loaded, not in dependency order. This can cause issues in situations where new states which are needed for substitutes are loaded much later than the original state that is being overridden to depend on those new states. An example of this is in a test environment where you may have configuration overrides that you wouldn't define in your production overrides that are loaded from dev-only code. Without this change, the new test that was written failed because the newly added dependency was not loaded first. I had to do a bit of a hack to remove a state defined by defstate-let me know if there is a better way of doing this. --- src/mount/extensions/common_deps.clj | 31 ++++++----- test/mount/extensions/common_deps_test.clj | 57 ++++++++++++++++++++ test/mount/extensions/explicit_deps_test.clj | 28 +++++++++- 3 files changed, 101 insertions(+), 15 deletions(-) create mode 100644 test/mount/extensions/common_deps_test.clj diff --git a/src/mount/extensions/common_deps.clj b/src/mount/extensions/common_deps.clj index 65a24c2..bbc1048 100644 --- a/src/mount/extensions/common_deps.clj +++ b/src/mount/extensions/common_deps.clj @@ -7,25 +7,28 @@ (defn- transitive "Given a graph and a node, returns all the transitive nodes." [graph node] - (loop [unexpanded (graph node) - expanded #{}] - (if-let [[node & more] (seq unexpanded)] - (if (contains? expanded node) - (recur more expanded) - (recur (concat more (graph node)) - (conj expanded node))) - expanded))) + (letfn [(search [cur] + (->> (get graph cur) + (map search) + (reduce into [cur])))] + (search node))) + +(defn ^:no-doc transitives + "Filters and orders a given list of states in transitive dependency order + based upon the given dependency graphs." + [var graphs states] + (let [var-kw (utils/var->keyword var) + dependencies (reverse (transitive (:dependencies graphs) var-kw)) + dependents (transitive (:dependents graphs) var-kw) + concatted (distinct (concat dependencies dependents))] + (filter (set states) concatted))) (defn ^:no-doc with-transitives* "Calls 0-arity function `f`, while `*states*` has been bound to the transitive dependencies and dependents of the given state `var`." [var graphs f] - (let [var-kw (utils/var->keyword var) - dependencies (transitive (:dependencies graphs) var-kw) - dependents (transitive (:dependents graphs) var-kw) - concatted (set (concat dependencies dependents))] - (binding [mount/*states* (atom (filter (conj concatted var-kw) @mount/*states*))] - (f)))) + (binding [mount/*states* (atom (transitives var graphs @mount/*states*))] + (f))) (defmacro with-transitives "Wraps the `body` having the `*states*` extension point bound to the diff --git a/test/mount/extensions/common_deps_test.clj b/test/mount/extensions/common_deps_test.clj new file mode 100644 index 0000000..5295b48 --- /dev/null +++ b/test/mount/extensions/common_deps_test.clj @@ -0,0 +1,57 @@ +(ns mount.extensions.common-deps-test + (:require + [clojure.test :refer [deftest testing is]] + [mount.extensions.common-deps :as sut])) + +(def system "system") + +(def graphs + {:dependencies {::overrides #{::overrides-a ::overrides-b} + ::config-a #{} + ::overrides-a #{::config-a} + ::config-b #{} + ::registry #{} + ::config #{::overrides ::env} + ::system #{::config ::debug ::db} + ::debug #{} + ::db #{::config} + ::env #{} + ::overrides-b #{::config-b}} + :dependents {::overrides #{::config} + ::config-a #{::overrides-a} + ::overrides-a #{::overrides} + ::config-b #{::overrides-b} + ::registry #{} + ::config #{::system ::db} + ::system #{} + ::debug #{::system} + ::db #{::system} + ::env #{::config} + ::overrides-b #{::overrides}}}) + +(def states + [::overrides + ::config-a + ::overrides-a + ::config-b + ::registry + ::config + ::system + ::debug + ::db + ::env + ::overrides-b]) + +(deftest transitives-test + (testing "ordering active states in dependency order" + (is (= [::debug + ::env + ::config-b + ::overrides-b + ::config-a + ::overrides-a + ::overrides + ::config + ::db + ::system] + (sut/transitives #'system graphs states))))) diff --git a/test/mount/extensions/explicit_deps_test.clj b/test/mount/extensions/explicit_deps_test.clj index 491832f..1edb467 100644 --- a/test/mount/extensions/explicit_deps_test.clj +++ b/test/mount/extensions/explicit_deps_test.clj @@ -1,7 +1,8 @@ (ns mount.extensions.explicit-deps-test (:require [clojure.test :refer (deftest is use-fixtures testing)] [mount.extensions.explicit-deps :as deps] - [mount.lite :refer (start state status stop with-substitutes)] + [mount.lite :as mount :refer (defstate start state status stop with-substitutes)] + [mount.utils :as utils] [mount.lite-test.test-state-1 :as ts1 :refer (state-1)] [mount.lite-test.test-state-2 :as ts2 :refer (state-2)] [mount.lite-test.test-state-2-extra :as ts2e :refer (state-2-a state-2-b)] @@ -11,9 +12,25 @@ (use-fixtures :each (fn [f] (stop) (f) (stop))) +(declare someone) + +(defn def-someone + [] + (defstate someone + :start "i am someone" + :dependencies nil)) + +(defn undef-someone + [] + (ns-unmap *ns* 'someone) + (swap! mount/*states* + #(remove %2 %1) + #(= % (utils/var->keyword #'someone)))) + ;;; Tests. (def dont-need-anyone (state :start "appeltaart" :dependencies nil)) +(def i-need-someone (state :start (str @someone " yay!") :dependencies [#'someone])) (deftest build-graphs-test (testing "no substitutes used" @@ -40,6 +57,15 @@ (deps/start #'state-3)) (is (status) {#'state-1 :stopped #'state-2 :started #'state-3 :started})) +(deftest start-with-overridden-dependency + (try + (def-someone) + (with-substitutes [#'state-2-a i-need-someone] + (deps/start #'state-2-a)) + (is (status) {#'state-1 :started #'state-2 :started #'someone :started #'state-2-a :started}) + (finally + (undef-someone)))) + (deftest stop-test (with-substitutes [#'state-2 dont-need-anyone] (start))