diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj
index ac1a7c5c19207b1ab933ee8e145e743e8f9e5641..1b5e25ed23f06c631bf31ea062bcf94e0c9152f5 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj
@@ -1,8 +1,13 @@
(ns metabase-enterprise.sandbox.api.permissions-test
- (:require [clojure.test :refer :all]
+ (:require [cheshire.core :as json]
+ [clojure.string :as str]
+ [clojure.test :refer :all]
[metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
- [metabase.models :refer [Database PermissionsGroup Table]]
+ [metabase.models :refer [Card Database PermissionsGroup
+ PersistedInfo Table]]
[metabase.models.permissions-group :as perms-group]
+ [metabase.models.persisted-info :as persisted-info]
+ [metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.util :as u]
[metabase.util.schema :as su]
@@ -128,3 +133,48 @@
(mt/user-http-request :crowberto :put 200 "permissions/graph" graph')
(testing "GTAP should not have been deleted"
(is (db/exists? GroupTableAccessPolicy :group_id (u/the-id (perms-group/all-users)), :table_id (mt/id :venues))))))))))
+
+(defn- fake-persist-card! [card]
+ (let [persisted-info (persisted-info/make-ready! (mt/user->id :rasta) card)]
+ (db/update-where! PersistedInfo {:card_id (u/the-id card)}
+ :definition (json/encode
+ (persisted-info/metadata->definition
+ (:result_metadata card)
+ (:table_name persisted-info)))
+ :active true
+ :state "persisted"
+ :query_hash (persisted-info/query-hash (:dataset_query card)))))
+
+(deftest persistence-and-permissions
+ (mt/with-model-cleanup [PersistedInfo]
+ (testing "Queries from cache if not sandboxed"
+ (mt/with-current-user
+ (mt/user->id :rasta)
+ (mt/with-temp*
+ [Card [card {:dataset_query (mt/mbql-query venues)
+ :dataset true
+ :database_id (mt/id)}]]
+ (fake-persist-card! card)
+ (is (str/includes?
+ (:query (qp/compile
+
+ {:database (mt/id)
+ :query {:source-table (str "card__" (u/the-id card))}
+ :type :query}))
+ "metabase_cache")))))
+ (testing "Queries from source if sandboxed"
+ (mt/with-gtaps
+ {:gtaps {:venues {:query (mt/mbql-query venues)
+ :remappings {:cat ["variable" [:field (mt/id :venues :category_id) nil]]}}}
+ :attributes {"cat" 50}}
+ (mt/with-temp*
+ [Card [card {:dataset_query (mt/mbql-query venues)
+ :dataset true
+ :database_id (mt/id)}]]
+ (fake-persist-card! card)
+ (is (not (str/includes?
+ (:query (qp/compile
+ {:database (mt/id)
+ :query {:source-table (str "card__" (u/the-id card))}
+ :type :query}))
+ "metabase_cache"))))))))
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 959d22701a910659e5e889134b0b16b32f9722bd..92800efbc5517798623f6367baab84f944bfd327 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -41,6 +41,7 @@
[metabase.query-processor.middleware.optimize-temporal-filters :as optimize-temporal-filters]
[metabase.query-processor.middleware.parameters :as parameters]
[metabase.query-processor.middleware.permissions :as qp.perms]
+ [metabase.query-processor.middleware.persistence :as qp.persistence]
[metabase.query-processor.middleware.pre-alias-aggregations :as qp.pre-alias-aggregations]
[metabase.query-processor.middleware.prevent-infinite-recursive-preprocesses :as prevent-infinite-recursive-preprocesses]
[metabase.query-processor.middleware.process-userland-query :as process-userland-query]
@@ -92,6 +93,7 @@
#'qp.add-source-metadata/add-source-metadata-for-source-queries
#'upgrade-field-literals/upgrade-field-literals
(resolve 'ee.sandbox.rows/apply-sandboxing)
+ #'qp.persistence/substitute-persisted-query
#'qp.add-implicit-clauses/add-implicit-clauses
#'qp.add-dimension-projections/add-remapped-columns
#'qp.resolve-fields/resolve-fields
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 146ce8c23aba7daab489f4ab29b9f5191d710396..cb849088fd568f4f177554ec311078ed301af961 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -47,7 +47,8 @@
:database mbql.s/DatabaseID
:source-metadata [mbql.s/SourceQueryMetadata]
- (s/optional-key :source-query/dataset?) s/Bool})
+ (s/optional-key :source-query/dataset?) s/Bool
+ (s/optional-key :persisted-info/native) s/Str})
(def ^:private MapWithResolvedSourceQuery
(s/constrained
@@ -121,8 +122,9 @@
(db/do-post-select Card)
(db/do-post-select PersistedInfo)
first)
- (throw (ex-info (tru "Card {0} does not exist." card-id)
- {:card-id card-id})))
+ (throw (ex-info (tru "Card {0} does not exist." card-id)
+ {:card-id card-id})))
+
{{mbql-query :query
database-id :database
{template-tags :template-tags
@@ -131,42 +133,47 @@
dataset? :dataset}
card
- [persisted? source-query]
- (or (when (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)))
- (= (:state card) "persisted"))
- (when log?
- (log/info (trs "Substituting cached query for card {0} from {1}.{2}"
- card-id
- (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
- (:table_name card))))
- [true
- {:native (format "select %s from %s.%s"
- (str/join ", " (map :field-name (get-in card [:definition :field-definitions])))
- (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
- (:table_name card))}])
- (when mbql-query
- [false mbql-query])
- (when native-query
- ;; rename `:query` to `:native` because source queries have a slightly different shape
- (let [native-query (set/rename-keys native-query {:query :native})]
- [false
- (cond-> native-query
- ;; trim trailing comments from SQL, but not other types of native queries
- (string? (:native native-query)) (update :native (partial trim-sql-query card-id))
- (empty? template-tags) (dissoc :template-tags))]))
- (throw (ex-info (tru "Missing source query in Card {0}" card-id)
- {:card 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)))
+ (= (:state card) "persisted"))
+
+ source-query (cond
+ mbql-query
+ mbql-query
+
+ native-query
+ ;; rename `:query` to `:native` because source queries have a slightly different shape
+ (let [native-query (set/rename-keys native-query {:query :native})]
+ (cond-> native-query
+ ;; trim trailing comments from SQL, but not other types of native queries
+ (string? (:native native-query)) (update :native (partial trim-sql-query card-id))
+ (empty? template-tags) (dissoc :template-tags)))
+
+ :else
+ (throw (ex-info (tru "Missing source query in Card {0}" card-id)
+ {:card card})))]
+ (when (and persisted? log?)
+ (log/info (trs "Found substitute cached query for card {0} from {1}.{2}"
+ card-id
+ (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
+ (:table_name card))))
+
;; log the query at this point, it's useful for some purposes
(log/debug (trs "Fetched source query from Card {0}:" card-id)
"\n"
(u/pprint-to-str 'yellow source-query))
- (cond-> {:source-query source-query
+
+ (cond-> {:source-query (cond-> source-query
+ ;; This will be applied, if still appropriate, by the peristence middleware
+ persisted? (assoc :persisted-info/native (format "select %s from %s.%s"
+ (str/join ", " (map :field-name (get-in card [:definition :field-definitions])))
+ (ddl.i/schema-name {:id database-id} (public-settings/site-uuid))
+ (:table_name card))))
:database database-id
:source-metadata (cond-> (seq (map mbql.normalize/normalize-source-metadata result-metadata))
persisted? sub-cached-field-refs)}
diff --git a/src/metabase/query_processor/middleware/persistence.clj b/src/metabase/query_processor/middleware/persistence.clj
new file mode 100644
index 0000000000000000000000000000000000000000..089eeda9e4da3f2d843a0d13d4fb72e1c8610215
--- /dev/null
+++ b/src/metabase/query_processor/middleware/persistence.clj
@@ -0,0 +1,16 @@
+(ns metabase.query-processor.middleware.persistence
+ (:require [metabase.mbql.util :as mbql.u]
+ [metabase.query-processor.middleware.permissions :as qp.perms]))
+
+(defn substitute-persisted-query
+ "Replaces source-query with the native query to the cache table.
+ `:persisted-info/native` is set in `fetch-source-query`.
+
+ If permissions are applied to the query (sandboxing) then do not use the cached query.
+ It may be be possible to use the persistence cache with sandboxing at a later date with further work."
+ [{::qp.perms/keys [perms] :as query}]
+ (if perms
+ query
+ (mbql.u/replace query
+ (x :guard (every-pred map? :persisted-info/native))
+ {:native (:persisted-info/native x)})))