diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index d3311b67613bfac88f4782a330d6640bb7d11de1..f969265547b7f2a2932295abb6975d757d1dc80f 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
@@ -6,21 +6,23 @@
[medley.core :as m]
[metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
[metabase-enterprise.sandbox.query-processor.middleware.row-level-restrictions :as row-level-restrictions]
- [metabase-enterprise.sandbox.test-util :as mt.tu]
[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]
@@ -680,7 +682,7 @@
(is (= [[10]]
(mt/rows result)))))
(testing "Run the query with different User attributes, should not get the cached result"
- (mt.tu/with-user-attributes :rasta {"cat" 40}
+ (mt/with-user-attributes :rasta {"cat" 40}
;; re-bind current user so updated attributes come in to effect
(mt/with-test-user :rasta
(is (= {"cat" 40}
@@ -1002,3 +1004,60 @@
;; this should *NOT* be cached because we're generating a nested query with sandboxing in play.
(is (= {:cached? false, :num-rows 5}
(run-query)))))))))
+
+(deftest persistence-disabled-when-sandboxed
+ (mt/test-driver :postgres
+ (mt/dataset sample-dataset
+ ;; with-gtaps creates a new copy of the database. So make sure to do that before anything else. Gets really
+ ;; confusing when `(mt/id)` and friends change value halfway through the test
+ (mt/with-gtaps {:gtaps {:products
+ {:remappings {:category
+ ["dimension"
+ [:field (mt/id :products :category)
+ nil]]}}}
+ :attributes {"category" nil}}
+ (mt/with-temporary-setting-values [:persisted-models-enabled true]
+ (mt/with-temp* [Card [model {:dataset true
+ :dataset_query (mt/mbql-query
+ products
+ ;; note does not include the field we have to filter on. No way
+ ;; 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))
+ (let [persisted-info (db/select-one 'PersistedInfo
+ :database_id (mt/id)
+ :card_id (:id model))]
+ (is (= "persisted" (:state persisted-info))
+ "Model failed to persist")
+ (is (string? (:table_name persisted-info)))
+ (let [query {:type :query
+ ;; just generate a select count(*) from card__<id>
+ :query {:aggregation [:count]
+ :source-table (str "card__" (:id model))}
+ :database (mt/id)}
+ regular-result (mt/with-test-user :crowberto
+ (qp/process-query query))
+ sandboxed-result (mt/with-user-attributes :rasta {"category" "Gizmo"}
+ (mt/with-test-user :rasta
+ (qp/process-query query)))]
+ (testing "Unsandboxed"
+ (testing "Sees full result set"
+ (is (= 200 (-> regular-result mt/rows ffirst))
+ "Expected 200 product results from cached, non-sandboxed results"))
+ (testing "Uses the cache table"
+ (is (str/includes? (-> regular-result :data :native_form :query)
+ (:table_name persisted-info))
+ "Did not use the persisted model cache")))
+ (testing "Sandboxed"
+ (testing "sees partial result"
+ (is (= 51 (-> sandboxed-result mt/rows ffirst))
+ "Sandboxed user got whole results instead of filtered"))
+ (testing "Does not use the cache table"
+ (is (not (str/includes? (-> sandboxed-result :data :native_form :query)
+ (:table_name persisted-info)))
+ "Erroneously used the persisted model cache")))))))))))
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 32c741cf9e20eedffb96e8317ee738b2987e617f..f697cd11b946dbafd062d690c6ff9db001d754fc 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -125,6 +125,12 @@
: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)"
@@ -142,8 +148,8 @@
(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
@@ -160,6 +166,7 @@
(= (: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"))
source-query (cond
diff --git a/test/metabase/task/persist_refresh_test.clj b/test/metabase/task/persist_refresh_test.clj
index f938fff35408de4ded1940bb9c0fb44f3df23c51..49de4164d2ebf10de109605b179e0f29b3e578c1 100644
--- a/test/metabase/task/persist_refresh_test.clj
+++ b/test/metabase/task/persist_refresh_test.clj
@@ -9,7 +9,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.interface :as qp.i]
[metabase.query-processor.timezone :as qp.timezone]
- [metabase.task.persist-refresh :as pr]
+ [metabase.task.persist-refresh :as task.persist-refresh]
[metabase.test :as mt]
[metabase.util :as u]
[potemkin.types :as p]
@@ -28,48 +28,48 @@
(deftest cron-schedule-test
(testing "creates schedule per hour when less than 24 hours"
(is (= "0 0 0/8 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 0 0/8 * * ? *"))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 0 0/8 * * ? *"))))
(testing "when anchored"
(is (= "0 30 1/8 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 30 1/8 * * ? *"))))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 30 1/8 * * ? *"))))))
(testing "creates schedule string per day when 24 hours"
(is (= "0 0 0 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 0 0 * * ? *"))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 0 0 * * ? *"))))
(testing "when anchored"
(is (= "0 30 1 * * ? *"
- (schedule-string (#'pr/cron-schedule "0 30 1 * * ? *")))))))
+ (schedule-string (#'task.persist-refresh/cron-schedule "0 30 1 * * ? *")))))))
(deftest trigger-job-info-test
(testing "Database refresh trigger"
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= {"db-id" 1 "type" "database"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "0 0 0/5 * * ? *"
(schedule-string tggr)))
(is (= "metabase.task.PersistenceRefresh.database.trigger.1"
(.. tggr getKey getName))))
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0 * * ? *")]
(is (= {"db-id" 1 "type" "database"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "0 0 0 * * ? *"
(schedule-string tggr))))
(testing "in report timezone UTC"
(mt/with-temporary-setting-values [report-timezone "UTC"]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= "UTC"
(.. tggr getTimeZone getID))))))
(testing "in report timezone LA"
(mt/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= "America/Los_Angeles"
(.. tggr getTimeZone getID))))))
(testing "in system timezone"
(mt/with-temporary-setting-values [report-timezone nil]
- (let [tggr (#'pr/database-trigger {:id 1} "0 0 0/5 * * ? *")]
+ (let [tggr (#'task.persist-refresh/database-trigger {:id 1} "0 0 0/5 * * ? *")]
(is (= (qp.timezone/system-timezone-id)
(.. tggr getTimeZone getID)))))))
(testing "Individual refresh trigger"
- (let [tggr (#'pr/individual-trigger {:card_id 5 :id 1})]
+ (let [tggr (#'task.persist-refresh/individual-trigger {:card_id 5 :id 1})]
(is (= {"persisted-id" 1 "type" "individual"}
(qc/from-job-data (.getJobDataMap tggr))))
(is (= "metabase.task.PersistenceRefresh.individual.trigger.1"
@@ -80,15 +80,15 @@
(let [ids (into #{} (map u/the-id dbs))]
(m/map-vals
#(select-keys % [:data :schedule :key])
- (select-keys (pr/job-info-by-db-id) ids))))
+ (select-keys (task.persist-refresh/job-info-by-db-id) ids))))
(deftest reschedule-refresh-test
(mt/with-temp-scheduler
(mt/with-temp* [Database [db-1 {:options {:persist-models-enabled true}}]
Database [db-2 {:options {:persist-models-enabled true}}]]
- (#'pr/job-init!)
+ (#'task.persist-refresh/job-init!)
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 0 0/4 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 0 0/4 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -97,7 +97,7 @@
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-2))}}
(job-info db-1 db-2))))
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 0 0/8 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 0 0/8 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -106,7 +106,7 @@
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-2))}}
(job-info db-1 db-2))))
(mt/with-temporary-setting-values [persisted-model-refresh-cron-schedule "0 30 1/8 * * ? *"]
- (pr/reschedule-refresh!)
+ (task.persist-refresh/reschedule-refresh!)
(is (= {(u/the-id db-1) {:data {"db-id" (u/the-id db-1) "type" "database"}
:schedule "0 30 1/8 * * ? *"
:key (format "metabase.task.PersistenceRefresh.database.trigger.%d" (u/the-id db-1))}
@@ -129,12 +129,12 @@
PersistedInfo [_punmodeled {:card_id (u/the-id unmodeled) :database_id (u/the-id db)}]]
(testing "Calls refresh on each persisted-info row"
(let [card-ids (atom #{})
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _database _definition card]
(swap! card-ids conj (:id card))
{:state :success})
(unpersist! [_ _database _persisted-info]))]
- (#'pr/refresh-tables! (u/the-id db) test-refresher)
+ (#'task.persist-refresh/refresh-tables! (u/the-id db) test-refresher)
(testing "Does not refresh archived cards or cards no longer models."
(is (= #{(u/the-id model1) (u/the-id model2)} @card-ids)))
(is (partial= {:task "persist-refresh"
@@ -145,7 +145,7 @@
{:order-by [[:id :desc]]})))))
(testing "Handles errors and continues"
(let [call-count (atom 0)
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _database _definition _card]
(swap! call-count inc)
;; throw on first persist
@@ -153,7 +153,7 @@
(throw (ex-info "DBs are risky" {:ka :boom})))
{:state :success})
(unpersist! [_ _database _persisted-info]))]
- (#'pr/refresh-tables! (u/the-id db) test-refresher)
+ (#'task.persist-refresh/refresh-tables! (u/the-id db) test-refresher)
(is (= 2 @call-count))
(is (partial= {:task "persist-refresh"
:task_details {:success 1 :error 1}}
@@ -173,17 +173,17 @@
;; need an "old enough" state change
:state_change_at (t/minus (t/local-date-time) (t/hours 2))}]]
(let [called-on (atom #{})
- test-refresher (reify pr/Refresher
+ test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _ _ _]
(is false "refresh! called on a model that should not be refreshed"))
(unpersist! [_ _database persisted-info]
(swap! called-on conj (u/the-id persisted-info))))]
(testing "Query finds deletabable, archived, and unmodeled persisted infos"
- (let [queued-for-deletion (into #{} (map :id) (#'pr/deletable-models))]
+ (let [queued-for-deletion (into #{} (map :id) (#'task.persist-refresh/deletable-models))]
(doseq [deletable-persisted [deletable punmodeled parchived]]
(is (contains? queued-for-deletion (u/the-id deletable-persisted))))))
;; we manually pass in the deleteable ones to not catch others in a running instance
- (#'pr/prune-deletables! test-refresher [deletable parchived punmodeled])
+ (#'task.persist-refresh/prune-deletables! test-refresher [deletable parchived punmodeled])
;; don't assert equality if there are any deletable in the app db
(doseq [deletable-persisted [deletable punmodeled parchived]]
(is (contains? @called-on (u/the-id deletable-persisted))))
@@ -217,7 +217,7 @@
[persisted-info-id] (persisted-info/ready-unpersisted-models! (mt/id))]
;; Persist the model
(ddl.i/check-can-persist (db/select-one Database :id (mt/id)))
- (#'pr/refresh-individual! persisted-info-id (var-get #'pr/dispatching-refresher))
+ (#'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
(let [query-on-top {:database (mt/id)
:type :query