From 87a835f1a4da2abb2bdb886fa9be044bc9463603 Mon Sep 17 00:00:00 2001
From: Simon Belak <simon@metabase.com>
Date: Sat, 12 May 2018 14:38:18 +0200
Subject: [PATCH] Fix rule loading
---
src/metabase/automagic_dashboards/core.clj | 124 +++++++++++---------
src/metabase/automagic_dashboards/rules.clj | 17 ++-
2 files changed, 79 insertions(+), 62 deletions(-)
diff --git a/src/metabase/automagic_dashboards/core.clj b/src/metabase/automagic_dashboards/core.clj
index 861cc111e81..709ce36e2de 100644
--- a/src/metabase/automagic_dashboards/core.clj
+++ b/src/metabase/automagic_dashboards/core.clj
@@ -32,6 +32,7 @@
[metabase.util :as u]
[puppetlabs.i18n.core :as i18n :refer [tru]]
[ring.util.codec :as codec]
+ [schema.core :as s]
[toucan.db :as db]))
(def ^:private public-endpoint "/auto/dashboard/")
@@ -50,7 +51,7 @@
:source table
:database (:db_id table)
:url (format "%stable/%s" public-endpoint (u/get-id table))
- :rules-prefix "table"})
+ :rules-prefix ["table"]})
(defmethod ->root (type Segment)
[segment]
@@ -61,7 +62,7 @@
:database (:db_id table)
:query-filter (-> segment :definition :filter)
:url (format "%ssegment/%s" public-endpoint (u/get-id segment))
- :rules-prefix "table"}))
+ :rules-prefix ["table"]}))
(defmethod ->root (type Metric)
[metric]
@@ -71,7 +72,7 @@
:source table
:database (:db_id table)
:url (format "%smetric/%s" public-endpoint (u/get-id metric))
- :rules-prefix "metric"}))
+ :rules-prefix ["metric"]}))
(defmethod ->root (type Field)
[field]
@@ -81,7 +82,7 @@
:source table
:database (:db_id table)
:url (format "%sfield/%s" public-endpoint (u/get-id field))
- :rules-prefix "field"}))
+ :rules-prefix ["field"]}))
(defmulti
^{:doc "Get a reference for a given model to be injected into a template
@@ -420,14 +421,15 @@
(assoc :score score
:dataset_query query))))))))
-(def ^:private ^{:arglists '([rule])} rule-specificity
- (comp (partial transduce (map (comp count ancestors)) +) :applies_to))
+(s/defn ^:private rule-specificity
+ [rule :- rules/Rule]
+ (transduce (map (comp count ancestors)) + (:applies_to rule)))
-(defn- matching-rules
+(s/defn ^:private matching-rules
"Return matching rules orderd by specificity.
Most specific is defined as entity type specification the longest ancestor
chain."
- [rules {:keys [source entity]}]
+ [rules :- [rules/Rule], {:keys [source entity]}]
(let [table-type (or (:entity_type source) :entity/GenericTable)]
(->> rules
(filter (fn [{:keys [applies_to]}]
@@ -479,8 +481,8 @@
[context _]
context)
-(defn- make-context
- [root rule]
+(s/defn ^:private make-context
+ [root, rule :- rules/Rule]
{:pre [(:source root)]}
(let [source (:source root)
tables (concat [source] (when (instance? (type Table) source)
@@ -524,10 +526,10 @@
vals
(apply concat)))
-(defn- make-dashboard
- ([root rule]
+(s/defn ^:private make-dashboard
+ ([root, rule :- rules/Rule]
(make-dashboard root rule {:tables [(:source root)]}))
- ([root rule context]
+ ([root, rule :- rules/Rule, context]
(let [this {"this" (-> root
:entity
(assoc :full-name (:full-name root)))}]
@@ -540,8 +542,8 @@
(update :groups (partial fill-templates :string context {}))
(assoc :refinements (:cell-query root))))))
-(defn- apply-rule
- [root rule]
+(s/defn ^:private apply-rule
+ [root, rule :- rules/Rule]
(let [context (make-context root rule)
dashboard (make-dashboard root rule context)
filters (->> rule
@@ -568,7 +570,7 @@
[entity]
(let [root (->root entity)
rule (->> root
- (matching-rules (rules/get-rules [(:rules-prefix root)]))
+ (matching-rules (rules/get-rules (:rules-prefix root)))
first)
dashboard (make-dashboard root rule)]
{:url (:url root)
@@ -587,9 +589,9 @@
(take n)
(map ->related-entity)))))
-(defn- indepth
- [root rule]
- (->> (rules/get-rules [(:rules-prefix root) (:rule rule)])
+(s/defn ^:private indepth
+ [root, rule :- rules/Rule]
+ (->> (rules/get-rules (concat (:rules-prefix root) [(:rule rule)]))
(keep (fn [indepth]
(when-let [[dashboard _] (apply-rule root indepth)]
{:title ((some-fn :short-title :title) dashboard)
@@ -598,8 +600,8 @@
(:rule indepth))})))
(take max-related)))
-(defn- related
- [root rule]
+(s/defn ^:private related
+ [root, rule :- rules/Rule]
(let [indepth (indepth root rule)]
{:indepth indepth
:tables (take (- max-related (count indepth)) (others root))}))
@@ -607,40 +609,48 @@
(defn- automagic-dashboard
"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 (rules/get-rule rule))
- (->> root
- (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).
- first))]
- (log/info (format "Applying heuristic %s to %s." (:rule rule) full-name))
- (log/info (format "Dimensions bindings:\n%s"
- (->> dashboard
- :context
- :dimensions
- (m/map-vals #(update % :matches (partial map :name)))
- u/pprint-to-str)))
- (log/info (format "Using definitions:\nMetrics:\n%s\nFilters:\n%s"
- (-> dashboard :context :metrics u/pprint-to-str)
- (-> dashboard :context :filters u/pprint-to-str)))
- (-> (cond-> dashboard
- (or query-filter cell-query)
- (assoc :title (str (tru "A closer look at ") full-name)))
- (populate/create-dashboard (or show max-cards))
- (assoc :related (-> (related root rule)
- (assoc :more (if (and (-> dashboard
- :cards
- count
- (> max-cards))
- (not= show :all))
- [{:title (tru "Show more about this")
- :description nil
- :table (:source root)
- :url (format "%s#show=all"
- (:url root))}]
- [])))))))
+ (if-let [[dashboard rule] (if rule
+ (apply-rule root (rules/get-rule rule))
+ (->> root
+ (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).
+ first))]
+ (do
+ (log/info (format "Applying heuristic %s to %s." (:rule rule) full-name))
+ (log/info (format "Dimensions bindings:\n%s"
+ (->> dashboard
+ :context
+ :dimensions
+ (m/map-vals #(update % :matches (partial map :name)))
+ u/pprint-to-str)))
+ (log/info (format "Using definitions:\nMetrics:\n%s\nFilters:\n%s"
+ (-> dashboard :context :metrics u/pprint-to-str)
+ (-> dashboard :context :filters u/pprint-to-str)))
+ (-> (cond-> dashboard
+ (or query-filter cell-query)
+ (assoc :title (str (tru "A closer look at ") full-name)))
+ (populate/create-dashboard (or show max-cards))
+ (assoc :related (-> (related root rule)
+ (assoc :more (if (and (-> dashboard
+ :cards
+ count
+ (> max-cards))
+ (not= show :all))
+ [{:title (tru "Show more about this")
+ :description nil
+ :table (:source root)
+ :url (format "%s#show=all"
+ (:url root))}]
+ []))))))
+ (throw (ex-info (format "Can't create dashboard for %s" full-name)
+ {:path (or rule rules-prefix)
+ :path-type (if rule
+ :path
+ :prefix)
+ :available-rules (map :rule (or (some-> rule rules/get-rule vector)
+ (rules/get-rules rules-prefix)))}))))
(def ^:private ^{:arglists '([card])} table-like?
(comp empty? #(qp.util/get-in-normalized % [:dataset_query :query :aggregation])))
@@ -698,7 +708,7 @@
(u/get-id card)
(encode-base64-json cell-query))
(format "%squestion/%s" public-endpoint (u/get-id card)))
- :rules-prefix "table"}
+ :rules-prefix ["table"]}
opts)))
nil))
@@ -731,7 +741,7 @@
(encode-base64-json cell-query))
(format "%sadhoc/%s" public-endpoint
(encode-base64-json query)))
- :rules-prefix "table"}
+ :rules-prefix ["table"]}
(update opts :cell-query merge-filter-clauses
(qp.util/get-in-normalized query [:dataset_query :query :filter])))))
nil))
diff --git a/src/metabase/automagic_dashboards/rules.clj b/src/metabase/automagic_dashboards/rules.clj
index 21870cec921..f282ddf7c32 100644
--- a/src/metabase/automagic_dashboards/rules.clj
+++ b/src/metabase/automagic_dashboards/rules.clj
@@ -12,7 +12,7 @@
[core :as s]]
[yaml.core :as yaml])
(:import java.nio.file.Path java.nio.file.FileSystems java.nio.file.FileSystem
- java.nio.file.Files ))
+ java.nio.file.Files))
(def ^Long ^:const max-score
"Maximal (and default) value for heuristics scores."
@@ -186,7 +186,8 @@
schema
(partition 2 constraints)))
-(def ^:private Rules
+(def Rule
+ "Rules defining an automagic dashboard."
(constrained-all
{(s/required-key :title) s/Str
(s/required-key :dimensions) [Dimension]
@@ -234,7 +235,7 @@
(def ^:private rules-validator
(sc/coercer!
- Rules
+ Rule
{[s/Str] ensure-seq
[OrderByPair] ensure-seq
OrderByPair (fn [x]
@@ -277,7 +278,7 @@
(def ^:private rules-dir "automagic_dashboards/")
(def ^:private ^{:arglists '([f])} file->entity-type
- (comp (partial re-find #".+(?=\.yaml)") str (memfn ^Path getFileName)))
+ (comp (partial re-find #".+(?=\.yaml$)") str (memfn ^Path getFileName)))
(defn- load-rule
[^Path f]
@@ -300,6 +301,12 @@
e)))
nil)))
+(defn- trim-trailing-slash
+ [s]
+ (if (str/ends-with? s "/")
+ (subs s 0 (-> s count dec))
+ s))
+
(defn- load-rule-dir
([dir] (load-rule-dir dir [] {}))
([dir path rules]
@@ -307,7 +314,7 @@
(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)
+ (load-rule-dir f (->> f (.getFileName) str trim-trailing-slash (conj path)) rules)
(file->entity-type f)
(assoc-in rules (concat path [(file->entity-type f) ::leaf]) (load-rule f))
--
GitLab