Merge pull request #7546 from metabase/xray-keep-rules-in-memory

xrays: keep rules in memory

project.clj

@@ -10,8 +10,7 @@
             "test" ["with-profile" "+expectations" "expectations"]
             "generate-sample-dataset" ["with-profile" "+generate-sample-dataset" "run"]
             "profile" ["with-profile" "+profile" "run" "profile"]
-            "h2" ["with-profile" "+h2-shell" "run" "-url" "jdbc:h2:./metabase.db" "-user" "" "-password" "" "-driver" "org.h2.Driver"]
-            "validate-automagic-dashboards" ["with-profile" "+validate-automagic-dashboards" "run"]}
+            "h2" ["with-profile" "+h2-shell" "run" "-url" "jdbc:h2:./metabase.db" "-user" "" "-password" "" "-driver" "org.h2.Driver"]}
   :dependencies [[org.clojure/clojure "1.8.0"]
                  [org.clojure/core.async "0.3.442"]
                  [org.clojure/core.match "0.3.0-alpha4"]              ; optimized pattern matching library for Clojure
@@ -195,5 +194,4 @@
              :profile {:jvm-opts ["-XX:+CITime"                       ; print time spent in JIT compiler
                                   "-XX:+PrintGC"]}                    ; print a message when garbage collection takes place
              ;; get the H2 shell with 'lein h2'
-             :h2-shell {:main org.h2.tools.Shell}
-             :validate-automagic-dashboards {:main metabase.automagic-dashboards.rules}})
+             :h2-shell {:main org.h2.tools.Shell}})

src/metabase/api/automagic_dashboards.clj

@@ -30,20 +30,20 @@
 
 (def ^:private Prefix
   (su/with-api-error-message
-      (->> ["table" "metric" "field"]
-           (mapcat rules/load-rules)
-           (filter :indepth)
-           (map :rule)
-           (apply s/enum))
+      (s/pred (fn [prefix]
+                (some #(not-empty (rules/get-rules [% prefix])) ["table" "metric" "field"])))
     (tru "invalid value for prefix")))
 
 (def ^:private Rule
   (su/with-api-error-message
-      (->> ["table" "metric" "field"]
-           (mapcat rules/load-rules)
-           (mapcat :indepth)
-           (map :rule)
-           (apply s/enum))
+      (s/pred (fn [rule]
+                (some (fn [toplevel]
+                        (some (comp rules/get-rule
+                                    (fn [prefix]
+                                      [toplevel prefix rule])
+                                    :rule)
+                              (rules/get-rules [toplevel])))
+                      ["table" "metric" "field"])))
     (tru "invalid value for rule name")))
 
 (def ^:private ^{:arglists '([s])} decode-base64-json
@@ -51,13 +51,9 @@
 
 (def ^:private Base64EncodedJSON
   (su/with-api-error-message
-      (s/pred decode-base64-json "valid base64 encoded json")
+      (s/pred decode-base64-json)
     (tru "value couldn''t be parsed as base64 encoded JSON")))
 
-(defn- load-rule
-  [entity prefix rule]
-  (rules/load-rule (format "%s/%s/%s.yaml" entity prefix rule)))
-
 (api/defendpoint GET "/database/:id/candidates"
   "Return a list of candidates for automagic dashboards orderd by interestingness."
   [id]
@@ -84,7 +80,7 @@
       Table
       api/check-404
       (magic/automagic-analysis
-       {:rule (load-rule "table" prefix rule)
+       {:rule ["table" prefix rule]
         :show (keyword show)})))
 
 (api/defendpoint GET "/segment/:id"
@@ -103,7 +99,7 @@
       Segment
       api/check-404
       (magic/automagic-analysis
-       {:rule (load-rule "table" prefix rule)
+       {:rule ["table" prefix rule]
         :show (keyword show)})))
 
 (api/defendpoint GET "/question/:id/cell/:cell-query"
@@ -130,7 +126,7 @@
       Card
       api/check-404
       (magic/automagic-analysis {:show       (keyword show)
-                                 :rule       (load-rule "table" prefix rule)
+                                 :rule       ["table" prefix rule]
                                  :cell-query (decode-base64-json cell-query)})))
 
 (api/defendpoint GET "/metric/:id"
@@ -158,7 +154,7 @@
    prefix Prefix
    rule   Rule}
   (-> id Card api/check-404 (magic/automagic-analysis {:show (keyword show)
-                                                       :rule (load-rule "table" prefix rule)})))
+                                                       :rule ["table" prefix rule]})))
 
 (api/defendpoint GET "/adhoc/:query"
   "Return an automagic dashboard analyzing ad hoc query."
@@ -181,7 +177,7 @@
       decode-base64-json
       query/adhoc-query
       (magic/automagic-analysis {:show (keyword show)
-                                 :rule (load-rule "table" prefix rule)})))
+                                 :rule ["table" prefix rule]})))
 
 (api/defendpoint GET "/adhoc/:query/cell/:cell-query"
   "Return an automagic dashboard analyzing ad hoc query."
@@ -211,7 +207,7 @@
         query/adhoc-query
         (magic/automagic-analysis {:show       (keyword show)
                                    :cell-query cell-query
-                                   :rule       (load-rule "table" prefix rule)}))))
+                                   :rule       ["table" prefix rule]}))))
 
 (def ^:private valid-comparison-pair?
   #{["segment" "segment"]

src/metabase/automagic_dashboards/core.clj

@@ -568,7 +568,7 @@
   [entity]
   (let [root      (->root entity)
         rule      (->> root
-                       (matching-rules (rules/load-rules (:rules-prefix root)))
+                       (matching-rules (rules/get-rules [(:rules-prefix root)]))
                        first)
         dashboard (make-dashboard root rule)]
     {:url         (:url root)
@@ -589,8 +589,7 @@
 
 (defn- indepth
   [root rule]
-  (->> rule
-       :indepth
+  (->> (rules/get-rules [(:rules-prefix root) (:rule rule)])
        (keep (fn [indepth]
                (when-let [[dashboard _] (apply-rule root indepth)]
                  {:title       ((some-fn :short-title :title) dashboard)
@@ -609,9 +608,9 @@
   "Create dashboards for table `root` using the best matching heuristics."
   [{:keys [rule show rules-prefix query-filter cell-query full-name] :as root}]
   (when-let [[dashboard rule] (if rule
-                                (apply-rule root rule)
+                                (apply-rule root (rules/get-rule rule))
                                 (->> root
-                                     (matching-rules (rules/load-rules rules-prefix))
+                                     (matching-rules (rules/get-rules [rules-prefix]))
                                      (keep (partial apply-rule root))
                                      ;; `matching-rules` returns an `ArraySeq` (via `sort-by`) so
                                      ;; `first` realises one element at a time (no chunking).
@@ -765,7 +764,7 @@
    interestingness of tables they contain (see above)."
   ([database] (candidate-tables database nil))
   ([database schema]
-   (let [rules (rules/load-rules "table")]
+   (let [rules (rules/get-rules ["table"])]
      (->> (apply db/select Table
                  (cond-> [:db_id           (u/get-id database)
                           :visibility_type nil]

src/metabase/automagic_dashboards/rules.clj

@@ -276,79 +276,73 @@
 
 (def ^:private rules-dir "automagic_dashboards/")
 
-(def ^:private ^{:arglists '([f])} file->table-type
+(def ^:private ^{:arglists '([f])} file->entity-type
   (comp (partial re-find #".+(?=\.yaml)") str (memfn ^Path getFileName)))
 
-(def ^:private ^{:arglists '([f])} file->parent-dir
-  (comp last #(str/split % #"/") str (memfn ^Path getParent)))
-
-(defmacro ^:private with-resources
-  [identifier & body]
-  `(let [uri# (-> rules-dir io/resource .toURI)]
-     (let [[fs# path#] (-> uri# .toString (str/split #"!" 2))]
-       (if path#
-         (with-open [^FileSystem ~identifier
-                     (-> fs#
-                         java.net.URI/create
-                         (FileSystems/newFileSystem (java.util.HashMap.)))]
-           ~@body)
-         (let [~identifier (FileSystems/getDefault)]
-           ~@body)))))
-
-(defn- resource-path
-  [^FileSystem fs path]
-  (when-let [path (some->> path (str rules-dir) io/resource)]
-    (let [path (if (-> path str (str/starts-with? "jar"))
-                 (-> path str (str/split #"!" 2) second)
-                 (.getPath path))]
-      (.getPath fs path (into-array String [])))))
-
-(declare load-rules)
-
-(defn load-rule
-  "Load and validate rule from file `f`."
-  ([f]
-   (with-resources fs
-     (some->> f (resource-path fs) (load-rule fs))))
-  ([fs ^Path f]
-   (try
-     (-> f
-         .toUri
-         slurp
-         yaml/parse-string
-         (assoc :rule (file->table-type f))
-         (update :applies_to #(or % (file->table-type f)))
-         rules-validator
-         (assoc :indepth (load-rules fs (format "%s/%s"
-                                                (file->parent-dir f)
-                                                (file->table-type f)))))
-     (catch Exception e
-       (log/error (format "Error parsing %s:\n%s"
-                          (.getFileName f)
-                          (or (some-> e
-                                      ex-data
-                                      (select-keys [:error :value])
-                                      u/pprint-to-str)
-                              e)))
-       nil))))
-
-(defn load-rules
-  "Load and validate all rules in dir."
-  ([dir]
-   (with-resources fs
-     (load-rules fs dir)))
-  ([fs dir]
-   (when-let [dir (resource-path fs dir)]
-     (with-open [ds (Files/newDirectoryStream dir)]
-       (->> ds
-            (filter #(str/ends-with? (.toString ^Path %) ".yaml"))
-            (keep (partial load-rule fs))
-            doall)))))
-
-(defn -main
-  "Entry point for lein task `validate-automagic-dashboards`"
-  [& _]
-  (dorun (load-rules "tables"))
-  (dorun (load-rules "metrics"))
-  (dorun (load-rules "fields"))
-  (System/exit 0))
+(defn- load-rule
+  [^Path f]
+  (try
+    (let [entity-type (file->entity-type f)]
+      (-> f
+          .toUri
+          slurp
+          yaml/parse-string
+          (assoc :rule entity-type)
+          (update :applies_to #(or % entity-type))
+          rules-validator))
+    (catch Exception e
+      (log/error (format "Error parsing %s:\n%s"
+                         (.getFileName f)
+                         (or (some-> e
+                                     ex-data
+                                     (select-keys [:error :value])
+                                     u/pprint-to-str)
+                             e)))
+      nil)))
+
+(defn- load-rule-dir
+  ([dir] (load-rule-dir dir [] {}))
+  ([dir path rules]
+   (with-open [ds (Files/newDirectoryStream dir)]
+     (reduce (fn [rules ^Path f]
+               (cond
+                 (Files/isDirectory f (into-array java.nio.file.LinkOption []))
+                 (load-rule-dir f (conj path (str (.getFileName f))) rules)
+
+                 (file->entity-type f)
+                 (assoc-in rules (concat path [(file->entity-type f) ::leaf]) (load-rule f))
+
+                 :else
+                 rules))
+             rules
+             ds))))
+
+(defmacro ^:private with-resource
+  [[identifier path] & body]
+  `(let [[jar# path#] (-> ~path .toString (str/split #"!" 2))]
+     (if path#
+       (with-open [^FileSystem fs# (-> jar#
+                                       java.net.URI/create
+                                       (FileSystems/newFileSystem (java.util.HashMap.)))]
+         (let [~identifier (.getPath fs# path# (into-array String []))]
+           ~@body))
+       (let [~identifier (.getPath (FileSystems/getDefault) (.getPath ~path) (into-array String []))]
+         ~@body))))
+
+(def ^:private rules (delay
+                      (with-resource [path (-> rules-dir io/resource .toURI)]
+                        (into {} (load-rule-dir path)))))
+
+(defn get-rules
+  "Get all rules with prefix `prefix`.
+   prefix is greedy, so [\"table\"] will match table/TransactionTable.yaml, but not
+   table/TransactionTable/ByCountry.yaml"
+  [prefix]
+  (->> prefix
+       (get-in @rules)
+       (keep (comp ::leaf val))))
+
+(defn get-rule
+  "Get rule at path `path`."
+  [path]
+  (get-in @rules (concat path [::leaf])))

test/metabase/automagic_dashboards/core_test.clj

@@ -50,7 +50,7 @@
   (->> (data/id :users)
        Table
        (#'magic/->root)
-       (#'magic/matching-rules (rules/load-rules "table"))
+       (#'magic/matching-rules (rules/get-rules ["table"]))
        (map (comp first :applies_to))))
 
 ;; Test fallback to GenericTable

test/metabase/automagic_dashboards/rules_test.clj

@@ -14,9 +14,12 @@
 (expect "ga:foo"  (#'rules/->type "ga:foo"))
 (expect :type/Foo (#'rules/->type "Foo"))
 
-(expect (every? some? (load-rules "table")))
-(expect (every? some? (load-rules "metrics")))
-(expect (every? some? (load-rules "fields")))
+;; This also tests that all the rules are valid (else there would be nils returned)
+(expect (every? some? (get-rules ["table"])))
+(expect (every? some? (get-rules ["metrics"])))
+(expect (every? some? (get-rules ["fields"])))
+
+(expect (some? (get-rules ["table" "GenericTable" "ByCountry"])))
 
 (expect true  (dimension-form? [:dimension "Foo"]))
 (expect true  (dimension-form? ["dimension" "Foo"]))