Merge pull request #7874 from metabase/xray-fix-aggregate-card-fks

X-rays: fix invalid queries in aggregate card with joins x-rays

src/metabase/automagic_dashboards/core.clj

@@ -153,8 +153,7 @@
      :rules-prefix ["field"]}))
 
 (def ^:private ^{:arglists '([card-or-question])} nested-query?
-  (comp (every-pred string? #(str/starts-with? % "card__"))
-        #(qp.util/get-in-normalized % [:dataset_query :query :source_table])))
+  (comp qp.util/query->source-card-id :dataset_query))
 
 (def ^:private ^{:arglists '([card-or-question])} native-query?
   (comp #{:native} qp.util/normalize-token #(qp.util/get-in-normalized % [:dataset_query :type])))
@@ -579,19 +578,23 @@
 
 (defmethod inject-root (type Field)
   [context field]
-  (update context :dimensions
-          (fn [dimensions]
-            (->> dimensions
-                 (keep (fn [[identifier definition]]
-                         (when-let [matches (->> definition
-                                                 :matches
-                                                 (remove (comp #{(id-or-name field)} id-or-name))
-                                                 not-empty)]
-                           [identifier (assoc definition :matches matches)])))
-                 (concat [["this" {:matches [field]
-                                   :name    (:display_name field)
-                                   :score   rules/max-score}]])
-                 (into {})))))
+  (let [field (assoc field :link (->> context
+                                      :tables
+                                      (m/find-first (comp #{(:table_id field)} u/get-id))
+                                      :link))]
+    (update context :dimensions
+            (fn [dimensions]
+              (->> dimensions
+                   (keep (fn [[identifier definition]]
+                           (when-let [matches (->> definition
+                                                   :matches
+                                                   (remove (comp #{(id-or-name field)} id-or-name))
+                                                   not-empty)]
+                             [identifier (assoc definition :matches matches)])))
+                   (concat [["this" {:matches [field]
+                                     :name    (:display_name field)
+                                     :score   rules/max-score}]])
+                   (into {}))))))
 
 (defmethod inject-root (type Metric)
   [context metric]

src/metabase/models/query.clj

@@ -60,9 +60,8 @@
               (throw e))))))
 
 
-(defn- native-query? [query-type]
-  (or (= query-type "native")
-      (= query-type :native)))
+(def ^:private ^{:arglists '([query-type])} native-query?
+  (comp #{:native} qputil/normalize-token))
 
 (defn query->database-and-table-ids
   "Return a map with `:database-id` and source `:table-id` that should be saved for a Card. Handles queries that use

test/metabase/automagic_dashboards/core_test.clj

@@ -12,6 +12,7 @@
              [query :as query]
              [table :refer [Table] :as table]
              [user :as user]]
+            [metabase.query-processor :as qp]
             [metabase.test.data :as data]
             [metabase.test.data.users :as test-users]
             [metabase.test.util :as tu]
@@ -91,22 +92,32 @@
                  ((test-users/user->client :rasta) :get 200 (format "automagic-dashboards/%s"
                                                                     (subs url 16)))))))
 
+(def ^:private valid-card?
+  (comp qp/expand :dataset_query))
+
 (defn- valid-dashboard?
   [dashboard]
   (assert (:name dashboard))
   (assert (-> dashboard :ordered_cards count pos?))
   (assert (valid-urls? dashboard))
+  (assert (every? valid-card? (keep :card (:ordered_cards dashboard))))
   true)
 
 (expect
   (with-rasta
     (with-dashboard-cleanup
-      (->> (Table) (keep #(automagic-analysis % {})) (every? valid-dashboard?)))))
+      (->> (db/select Table :db_id (data/id))
+           (keep #(automagic-analysis % {}))
+           (every? valid-dashboard?)))))
 
 (expect
   (with-rasta
     (with-dashboard-cleanup
-      (->> (Field) (keep #(automagic-analysis % {})) (every? valid-dashboard?)))))
+      (->> (db/select Field
+             :table_id [:in (db/select-field :id Table :db_id (data/id))]
+             :visibility_type "normal")
+           (keep #(automagic-analysis % {}))
+           (every? valid-dashboard?)))))
 
 (expect
   (tt/with-temp* [Metric [{metric-id :id} {:table_id (data/id :venues)
@@ -192,7 +203,7 @@
       (with-dashboard-cleanup
         (-> card-id
             Card
-            (automagic-analysis {:cell-query [:= [:field-id (data/id :venues :category_id) 2]]})
+            (automagic-analysis {:cell-query [:= [:field-id (data/id :venues :category_id)] 2]})
             valid-dashboard?)))))
 
 
@@ -229,6 +240,16 @@
                                   :database (data/id)})]
         (-> q (automagic-analysis {}) valid-dashboard?)))))
 
+(expect
+  (with-rasta
+    (with-dashboard-cleanup
+      (let [q (query/adhoc-query {:query {:aggregation [[:count]]
+                                          :breakout [[:fk-> (data/id :checkins) (data/id :venues :category_id)]]
+                                          :source_table (data/id :checkins)}
+                                  :type :query
+                                  :database (data/id)})]
+        (-> q (automagic-analysis {}) valid-dashboard?)))))
+
 (expect
   (with-rasta
     (with-dashboard-cleanup