Substitute persisted queries in parameter card references (#25610)

* Substitute persisted queries in parameter card references

```sql
select o.total, o.quantity, model_p.category, model_p.title
from orders o
left join {{#14-pg-products}} model_p -- reference to products model
on o.product_id = model_p.id
limit 4
```

Testing:

I'm unhappy with how verbose the test is. At some point we're going to
need a better, standardized way to test persistence. But the test makes
a persisted table of the test-data.categories
table (test/metabase/test/data/dataset_definitions/test-data.edn). It
runs some sql to update the values in the table to turn `"Winery"` ->
`"Winery from cached table"`, joins the two tables together to have both
at once.

There's a lot of moving pieces and the setup is a bit verbose. There's
also no obvious place where these types of tests should go. We'll need
to consolidate them in the future (and extend them).

* persisted macro

* docstring for `persisted-info-native-query`

* alignments and small nits

* Reuse persistence macro

* clj-kondo cleanups

* remove clj-kondo change before rebase

.clj-kondo/config.edn

@@ -630,6 +630,7 @@
    metabase.test.util/with-temporary-raw-setting-values                         macros.metabase.test.util/with-temporary-raw-setting-values
    metabase.test/with-group-for-user                                            macros.metabase.test.data.users/with-group-for-user
    metabase.test/with-gtaps                                                     macros.metabase-enterprise.sandbox.test-util/with-gtaps
+   metabase.test/with-persistence-enabled                                       macros.metabase.test.persistence/with-persistence-enabled
    metabase.test/with-temp-env-var-value                                        macros.metabase.test.util/with-temp-env-var-value
    metabase.test/with-temporary-raw-setting-values                              macros.metabase.test.util/with-temporary-raw-setting-values
    toucan.models/defmodel                                                       macros.toucan.models/defmodel}}

.clj-kondo/macros/metabase/test/persistence.clj

+(ns macros.metabase.test.persistence)
+
+(defmacro with-persistence-enabled [[persist-fn-sym] & body]
+  `(clojure.core/let [~persist-fn-sym (fn [])]
+     ~@body))

enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj

@@ -8,21 +8,18 @@
             [metabase-enterprise.sandbox.query-processor.middleware.row-level-restrictions :as row-level-restrictions]
             [metabase.api.common :as api]
             [metabase.driver :as driver]
-            [metabase.driver.ddl.interface :as ddl.i]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.mbql.normalize :as mbql.normalize]
             [metabase.mbql.util :as mbql.u]
             [metabase.models :refer [Card Collection Field Table]]
             [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as perms-group]
-            [metabase.models.persisted-info :as persisted-info]
             [metabase.query-processor :as qp]
             [metabase.query-processor.middleware.cache-test :as cache-test]
             [metabase.query-processor.middleware.permissions :as qp.perms]
             [metabase.query-processor.pivot :as qp.pivot]
             [metabase.query-processor.util :as qp.util]
             [metabase.query-processor.util.add-alias-info :as add]
-            [metabase.task.persist-refresh :as task.persist-refresh]
             [metabase.test :as mt]
             [metabase.test.data.env :as tx.env]
             [metabase.util :as u]
@@ -1016,7 +1013,7 @@
                                                   [:field (mt/id :products :category)
                                                    nil]]}}}
                       :attributes {"category" nil}}
-        (mt/with-temporary-setting-values [:persisted-models-enabled true]
+        (mt/with-persistence-enabled [persist-models!]
           (mt/with-temp* [Card [model {:dataset       true
                                        :dataset_query (mt/mbql-query
                                                        products
@@ -1024,11 +1021,7 @@
                                                        ;; to use the sandbox filter on the cached table
                                                        {:fields [$id $price]})}]]
             ;; persist model
-            (ddl.i/check-can-persist (mt/db))
-            (persisted-info/ready-database! (mt/id))
-            (#'task.persist-refresh/refresh-tables!
-             (mt/id)
-             (var-get #'task.persist-refresh/dispatching-refresher))
+            (persist-models!)
             (let [persisted-info (db/select-one 'PersistedInfo
                                                 :database_id (mt/id)
                                                 :card_id (:id model))]

src/metabase/driver/common/parameters/values.clj

@@ -14,9 +14,11 @@
             [metabase.mbql.schema :as mbql.s]
             [metabase.models.card :refer [Card]]
             [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
+            [metabase.models.persisted-info :refer [PersistedInfo]]
             [metabase.query-processor :as qp]
             [metabase.query-processor.error-type :as qp.error-type]
             [metabase.query-processor.store :as qp.store]
+            [metabase.query-processor.util.persisted-cache :as qp.persistence]
             [metabase.util :as u]
             [metabase.util.i18n :refer [tru]]
             [metabase.util.schema :as su]
@@ -139,15 +141,20 @@
   (when-not card-id
     (throw (ex-info (tru "Invalid :card parameter: missing `:card-id`")
                     {:tag tag, :type qp.error-type/invalid-parameter})))
-  (let [query (or (db/select-one-field :dataset_query Card :id card-id)
-                  (throw (ex-info (tru "Card {0} not found." card-id)
-                                  {:card-id card-id, :tag tag, :type qp.error-type/invalid-parameter})))]
+  (let [card           (db/select-one Card :id card-id)
+        persisted-info (when (:dataset card)
+                         (db/select-one PersistedInfo :card_id card-id))
+        query          (or (:dataset_query card)
+                           (throw (ex-info (tru "Card {0} not found." card-id)
+                                           {:card-id card-id, :tag tag, :type qp.error-type/invalid-parameter})))]
     (try
       (params/map->ReferencedCardQuery
        (let [query (assoc query :info {:card-id card-id})]
          (log/tracef "Compiling referenced query for Card %d\n%s" card-id (u/pprint-to-str query))
          (merge {:card-id card-id}
-                (qp/compile query))))
+                (or (when (qp.persistence/can-substitute? card persisted-info)
+                      {:query (qp.persistence/persisted-info-native-query persisted-info)})
+                    (qp/compile query)))))
       (catch ExceptionInfo e
         (throw (ex-info
                 (tru "The sub-query from referenced question #{0} failed with the following error: {1}"

src/metabase/query_processor/middleware/fetch_source_query.clj

@@ -25,10 +25,7 @@
             [clojure.string :as str]
             [clojure.tools.logging :as log]
             [medley.core :as m]
-            [metabase.driver :as driver]
             [metabase.driver.ddl.interface :as ddl.i]
-            [metabase.driver.sql.util :as sql.u]
-            [metabase.driver.util :as driver.u]
             [metabase.mbql.normalize :as mbql.normalize]
             [metabase.mbql.schema :as mbql.s]
             [metabase.mbql.util :as mbql.u]
@@ -36,6 +33,7 @@
             [metabase.models.persisted-info :as persisted-info :refer [PersistedInfo]]
             [metabase.public-settings :as public-settings]
             [metabase.query-processor.middleware.permissions :as qp.perms]
+            [metabase.query-processor.util.persisted-cache :as qp.persisted]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs tru]]
             [metabase.util.schema :as su]
@@ -108,48 +106,17 @@
          (assoc m :field_ref [:field (:name m) {:base-type (:base_type m)}]))
        metadata))
 
-(defn- persisted-info-native-query [card database-id]
-  (let [driver (or driver/*driver* (driver.u/database->driver database-id))]
-    (format "select %s from %s.%s"
-            (str/join ", " (map #(sql.u/quote-name
-                                   driver
-                                   :field
-                                   (:field-name %))
-                                (get-in card [:definition :field-definitions])))
-            (sql.u/quote-name
-              driver
-              :table
-              (ddl.i/schema-name {:id database-id} (public-settings/site-uuid)))
-            (sql.u/quote-name
-              driver
-              :table
-              (:table_name card)))))
-
-(defn- segmented-user?
-  []
-  (if-let [segmented? (resolve 'metabase-enterprise.sandbox.api.util/segmented-user?)]
-    (segmented?)
-    false))
-
 (s/defn card-id->source-query-and-metadata :- SourceQueryAndMetadata
   "Return the source query info for Card with `card-id`. Pass true as the optional second arg `log?` to enable
   logging. (The circularity check calls this and will print more than desired)"
   ([card-id :- su/IntGreaterThanZero]
    (card-id->source-query-and-metadata card-id false))
   ([card-id :- su/IntGreaterThanZero log? :- s/Bool]
-   (let [card
-         ;; todo: we need to cache this. We are running this in preprocess, compile, and then again
-         (or (->> (db/query {:select    [:card.dataset_query :card.database_id :card.result_metadata :card.dataset
-                                         :persisted.table_name :persisted.definition :persisted.query_hash
-                                         :persisted.state :persisted.active]
-                             :from      [[Card :card]]
-                             :left-join [[PersistedInfo :persisted] [:= :card.id :persisted.card_id]]
-                             :where     [:= :card.id card-id]})
-                  (db/do-post-select Card)
-                  (db/do-post-select PersistedInfo)
-                  first)
-             (throw (ex-info (tru "Card {0} does not exist." card-id)
+   (let [;; todo: we need to cache this. We are running this in preprocess, compile, and then again
+         card           (or (db/select-one Card :id card-id)
+                            (throw (ex-info (tru "Card {0} does not exist." card-id)
                              {:card-id card-id})))
+         persisted-info (db/select-one PersistedInfo :card_id card-id)
 
          {{mbql-query                   :query
            database-id                  :database
@@ -159,15 +126,7 @@
           dataset?                               :dataset}
          card
 
-         persisted? (and persisted-info/*allow-persisted-substitution*
-                         (:active card)
-                         (:definition card)
-                         (:query_hash card)
-                         (= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
-                         (= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
-                                                                                    (:table_name card)))
-                         (not (segmented-user?))
-                         (= (:state card) "persisted"))
+         persisted? (qp.persisted/can-substitute? card persisted-info)
 
          source-query (cond
                         mbql-query
@@ -197,7 +156,9 @@
 
      (cond-> {:source-query    (cond-> source-query
                                  ;; This will be applied, if still appropriate, by the peristence middleware
-                                 persisted? (assoc :persisted-info/native (persisted-info-native-query card database-id)))
+                                 persisted?
+                                 (assoc :persisted-info/native
+                                        (qp.persisted/persisted-info-native-query persisted-info)))
               :database        database-id
               :source-metadata (cond-> (seq (map mbql.normalize/normalize-source-metadata result-metadata))
                                  persisted? sub-cached-field-refs)}

src/metabase/query_processor/util/persisted_cache.clj

+(ns metabase.query-processor.util.persisted-cache
+  (:require    [clojure.string :as str]
+               [metabase.driver :as driver]
+               [metabase.driver.ddl.interface :as ddl.i]
+               [metabase.driver.sql.util :as sql.u]
+               [metabase.driver.util :as driver.u]
+               [metabase.models.persisted-info :as persisted-info]
+               [metabase.public-settings :as public-settings]))
+
+(defn- segmented-user?
+  []
+  (if-let [segmented? (resolve 'metabase-enterprise.sandbox.api.util/segmented-user?)]
+    (segmented?)
+    false))
+
+(defn can-substitute?
+  "Taking a card and a persisted-info record (possibly nil), returns whether the card's query can be substituted for a
+  persisted version."
+  [card persisted-info]
+  (and persisted-info
+       persisted-info/*allow-persisted-substitution*
+       (:active persisted-info)
+       (= (:state persisted-info) "persisted")
+       (:definition persisted-info)
+       (:query_hash persisted-info)
+       (= (:query_hash persisted-info) (persisted-info/query-hash (:dataset_query card)))
+       (= (:definition persisted-info)
+          (persisted-info/metadata->definition (:result_metadata card)
+                                               (:table_name persisted-info)))
+       (not (segmented-user?))))
+
+(defn persisted-info-native-query
+  "Returns a native query that selects from the persisted cached table from `persisted-info`. Does not check if
+  persistence is appropriate. Use [[can-substitute?]] for that check."
+  [persisted-info]
+  (let [database-id (:database_id persisted-info)
+        driver      (or driver/*driver* (driver.u/database->driver database-id))]
+    (format "select %s from %s.%s"
+            (str/join ", " (map #(sql.u/quote-name
+                                  driver
+                                  :field
+                                  (:field-name %))
+                                (get-in persisted-info [:definition :field-definitions])))
+            (sql.u/quote-name
+             driver
+             :table
+             (ddl.i/schema-name {:id database-id} (public-settings/site-uuid)))
+            (sql.u/quote-name
+             driver
+             :table
+             (:table_name persisted-info)))))

test/metabase/driver/common/parameters/values_test.clj

 (ns metabase.driver.common.parameters.values-test
-  (:require [clojure.test :refer :all]
+  (:require [clojure.java.jdbc :as jdbc]
+            [clojure.test :refer :all]
             [metabase.driver :as driver]
             [metabase.driver.common.parameters :as params]
             [metabase.driver.common.parameters.values :as params.values]
+            [metabase.driver.ddl.interface :as ddl.i]
+            [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
             [metabase.models :refer [Card Collection NativeQuerySnippet]]
             [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as perms-group]
+            [metabase.public-settings :as public-settings]
             [metabase.query-processor :as qp]
             [metabase.query-processor.middleware.permissions :as qp.perms]
             [metabase.query-processor.store :as qp.store]
             [metabase.test :as mt]
             [metabase.util :as u]
             [metabase.util.schema :as su]
-            [schema.core :as s])
+            [schema.core :as s]
+            [toucan.db :as db])
   (:import clojure.lang.ExceptionInfo
            java.util.UUID
            metabase.driver.common.parameters.ReferencedCardQuery))
@@ -278,6 +283,8 @@
              clojure.lang.ExceptionInfo
              (query->params-map query)))))))
 
+
+
 (deftest card-query-test
   (testing "Card query template tag gets card's native query"
     (let [test-query "SELECT 1"]
@@ -317,6 +324,56 @@
                      :card-id      (:id card)}
                     []))))))))
 
+  (testing "Persisted Models are substituted"
+    (mt/test-driver :postgres
+      (mt/dataset test-data
+        (mt/with-persistence-enabled [persist-models!]
+          (let [mbql-query (mt/mbql-query categories)]
+            (mt/with-temp* [Card [model {:name "model"
+                                         :dataset true
+                                         :dataset_query mbql-query
+                                         :database_id (mt/id)}]]
+              (persist-models!)
+              (testing "tag uses persisted table"
+                (let [pi (db/select-one 'PersistedInfo :card_id (u/the-id model))]
+                  (is (= "persisted" (:state pi)))
+                  (is (re-matches #"select \"id\", \"name\" from \"metabase_cache_[a-z0-9]+_[0-9]+\".\"model_[0-9]+_model\""
+                                  (:query
+                                   (value-for-tag
+                                    {:name         "card-template-tag-test"
+                                     :display-name "Card template tag test"
+                                     :type         :card
+                                     :card-id      (:id model)}
+                                    []))))
+                  (testing "query hits persisted table"
+                    (let [persisted-schema (ddl.i/schema-name {:id (mt/id)}
+                                                              (public-settings/site-uuid))
+                          update-query     (format "update %s.%s set name = name || ' from cached table'"
+                                                   persisted-schema (:table_name pi))
+                          model-query (format "select c_orig.name, c_cached.name
+                                               from categories c_orig
+                                               left join {{#%d}} c_cached
+                                               on c_orig.id = c_cached.id
+                                               order by c_orig.id desc limit 3"
+                                              (u/the-id model))
+                          tag-name    (format "#%d" (u/the-id model))]
+                      (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec (mt/db))
+                                     [update-query])
+                      (is (= [["Winery" "Winery from cached table"]
+                              ["Wine Bar" "Wine Bar from cached table"]
+                              ["Vegetarian / Vegan" "Vegetarian / Vegan from cached table"]]
+                             (mt/rows (qp/process-query
+                                       {:database (mt/id)
+                                        :type :native
+                                        :native {:query model-query
+                                                 :template-tags
+                                                 {(keyword tag-name)
+                                                  {:id "c6558da4-95b0-d829-edb6-45be1ee10d3c"
+                                                   :name tag-name
+                                                   :display-name tag-name
+                                                   :type "card"
+                                                   :card-id (u/the-id model)}}}}))))))))))))))
+
   (testing "Card query template tag wraps error in tag details"
     (mt/with-temp Card [param-card {:dataset_query
                                     (mt/native-query

test/metabase/task/persist_refresh_test.clj

@@ -3,9 +3,7 @@
             [clojurewerkz.quartzite.conversion :as qc]
             [java-time :as t]
             [medley.core :as m]
-            [metabase.driver.ddl.interface :as ddl.i]
             [metabase.models :refer [Card Database PersistedInfo TaskHistory]]
-            [metabase.models.persisted-info :as persisted-info]
             [metabase.query-processor :as qp]
             [metabase.query-processor.interface :as qp.i]
             [metabase.query-processor.timezone :as qp.timezone]
@@ -199,26 +197,22 @@
     (with-redefs [qp.i/absolute-max-results 3]
       (mt/dataset daily-bird-counts
         (mt/test-driver :postgres
-          (mt/with-temporary-setting-values [:persisted-models-enabled true]
-            (db/update! Database (mt/id) :options {:persist-models-enabled true})
-            (mt/with-temp* [Card          [model {:dataset       true
-                                                  :database_id   (mt/id)
-                                                  :query_type    :query
-                                                  :dataset_query {:database (mt/id)
-                                                                  :type     :query
-                                                                  :query    {:source-table (mt/id :bird-count)}}}]]
-              (let [;; Get the number of rows before the model is persisted
+          (mt/with-persistence-enabled [persist-models!]
+            (mt/with-temp* [Card [model {:dataset       true
+                                         :database_id   (mt/id)
+                                         :query_type    :query
+                                         :dataset_query {:database (mt/id)
+                                                         :type     :query
+                                                         :query    {:source-table (mt/id :bird-count)}}}]]
+              (let [ ;; Get the number of rows before the model is persisted
                     query-on-top       {:database (mt/id)
                                         :type     :query
                                         :query    {:aggregation  [[:count]]
                                                    :source-table (str "card__" (:id model))}}
-                    [[num-rows-query]] (mt/rows (qp/process-query query-on-top))
-                    ;; Create PersistedInfo for model
-                    [persisted-info-id] (persisted-info/ready-unpersisted-models! (mt/id))]
+                    [[num-rows-query]] (mt/rows (qp/process-query query-on-top))]
                 ;; Persist the model
-                (ddl.i/check-can-persist (db/select-one Database :id (mt/id)))
-                (#'task.persist-refresh/refresh-individual! persisted-info-id (var-get #'task.persist-refresh/dispatching-refresher))
-                 ;; Check the number of rows is the same after persisting
+                (persist-models!)
+                ;; Check the number of rows is the same after persisting
                 (let [query-on-top {:database (mt/id)
                                     :type     :query
                                     :query    {:aggregation [[:count]]

test/metabase/test.clj

@@ -32,6 +32,7 @@
             [metabase.test.data.interface :as tx]
             [metabase.test.data.users :as test.users]
             [metabase.test.initialize :as initialize]
+            [metabase.test.persistence :as test.persistence]
             metabase.test.redefs
             [metabase.test.util :as tu]
             [metabase.test.util.async :as tu.async]
@@ -59,6 +60,7 @@
   initialize/keep-me
   metabase.test.redefs/keep-me
   mw.session/keep-me
+  test.persistence/keep-me
   qp/keep-me
   qp.test-util/keep-me
   qp.test/keep-me
@@ -164,6 +166,9 @@
  [test-runner.assert-exprs
   derecordize]
 
+ [test.persistence
+  with-persistence-enabled]
+
  [test.users
   fetch-user
   test-user?

test/metabase/test/persistence.clj

+(ns metabase.test.persistence
+  (:require [metabase.driver.ddl.interface :as ddl.i]
+            [metabase.models.persisted-info :as persisted-info]
+            [metabase.task.persist-refresh :as task.persist-refresh]
+            [metabase.test.data :as data]
+            [metabase.test.util :as tu]))
+
+(defn with-persistence-enabled*
+  [f]
+  (tu/with-temporary-setting-values [:persisted-models-enabled true]
+    (ddl.i/check-can-persist (data/db))
+    (persisted-info/ready-database! (data/id))
+    (let [persist-fn (fn persist-fn []
+                       (#'task.persist-refresh/refresh-tables!
+                        (data/id)
+                        (var-get #'task.persist-refresh/dispatching-refresher)))]
+      (f persist-fn))))
+
+(defmacro with-persistence-enabled
+  "Does the necessary setup to enable persistence on the current db. Provide a binding for a function to persist
+  everything.
+
+  (with-persisted [persist-models!]
+    (let [mbql-query (mt/mbql-query categories)]
+      (mt/with-temp* [Card [model {:name \"model\"
+                                   :dataset true
+                                   :dataset_query mbql-query
+                                   :database_id (mt/id)}]]
+        (persist-models!))
+        ...))"
+  [[persist-fn-binding] & body]
+  `(with-persistence-enabled* (fn [~persist-fn-binding] ~@body)))