From 4a567c28de3aacfecc9d1d0ecf9b19f5817cae9d Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 25 May 2022 11:54:14 -0600
Subject: [PATCH] Persist model integration changes for front end (#22933)
* Add can-manage to databases to help front-end
* Only return persisted true when the state agrees
* Add event for persisted-info to catch new models and turn on peristence when appropriate
* Fix bad threading
* Add comment about the case being detected to add the persisted-info
* Fix pre-existing bug in expand-db-details where a function was used as the condition
---
src/metabase/api/database.clj | 9 ++--
src/metabase/events/persisted_info.clj | 45 ++++++++++++++++++++
src/metabase/models/persisted_info.clj | 4 +-
test/metabase/api/database_test.clj | 2 +-
test/metabase/events/persisted_info_test.clj | 19 +++++++++
5 files changed, 74 insertions(+), 5 deletions(-)
create mode 100644 src/metabase/events/persisted_info.clj
create mode 100644 test/metabase/events/persisted_info_test.clj
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 6cb115ca8db..fa199b4beee 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -330,14 +330,17 @@
{include (s/maybe (s/enum "tables" "tables.fields"))}
(let [include-editable-data-model? (Boolean/parseBoolean include_editable_data_model)
exclude-uneditable-details? (Boolean/parseBoolean exclude_uneditable_details)
- filter-by-data-access? (not (or include-editable-data-model? exclude-uneditable-details?))]
- (cond-> (api/check-404 (Database id))
+ filter-by-data-access? (not (or include-editable-data-model? exclude-uneditable-details?))
+ database (api/check-404 (Database id))]
+ (cond-> database
filter-by-data-access? api/read-check
exclude-uneditable-details? api/write-check
true add-expanded-schedules
true (get-database-hydrate-include include)
include-editable-data-model? check-db-data-model-perms
- mi/can-write? secret/expand-db-details-inferred-secret-values)))
+ (mi/can-write? database) (->
+ secret/expand-db-details-inferred-secret-values
+ (assoc :can-manage true)))))
;;; ----------------------------------------- GET /api/database/:id/metadata -----------------------------------------
diff --git a/src/metabase/events/persisted_info.clj b/src/metabase/events/persisted_info.clj
new file mode 100644
index 00000000000..4f7c3ca7f5b
--- /dev/null
+++ b/src/metabase/events/persisted_info.clj
@@ -0,0 +1,45 @@
+(ns metabase.events.persisted-info
+ (:require [clojure.core.async :as a]
+ [clojure.tools.logging :as log]
+ [metabase.events :as events]
+ [metabase.models :refer [Database PersistedInfo]]
+ [metabase.models.persisted-info :as persisted-info]
+ [metabase.public-settings :as public-settings]
+ [toucan.db :as db]))
+
+(def ^:private ^:const persisted-info-topics
+ "The `Set` of event topics which are subscribed to add persisted-info to new models."
+ #{:card-create
+ :card-update})
+
+
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to persist new models"}
+ persisted-info-channel
+ (a/chan))
+
+
+;;; ## ---------------------------------------- EVENT PROCESSING ----------------------------------------
+
+
+(defn process-event
+ "Handle processing for a single event notification received on the persisted-info-channel"
+ [{_topic :topic card :item :as event}]
+ ;; try/catch here to prevent individual topic processing exceptions from bubbling up. better to handle them here.
+ (try
+ ;; We only want to add a persisted-info for newly created models where dataset is being set to true.
+ ;; If there is already a PersistedInfo, even in "off" or "deletable" state, we skip it as this
+ ;; is only supposed to be that initial edge when the dataset is being changed.
+ (when (and (:dataset card)
+ (public-settings/persisted-models-enabled)
+ (get-in (Database (:database_id card)) [:options :persist-models-enabled])
+ (nil? (db/select-one-field :id PersistedInfo :card_id (:id card))))
+ (persisted-info/turn-on! (:actor_id card) card))
+ (catch Throwable e
+ (log/warn (format "Failed to process persisted-info event. %s" (:topic event)) e))))
+
+
+;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
+
+(defmethod events/init! ::PersistedInfo
+ [_]
+ (events/start-event-listener! persisted-info-topics persisted-info-channel process-event))
diff --git a/src/metabase/models/persisted_info.clj b/src/metabase/models/persisted_info.clj
index 2bf48550bb7..e982385f329 100644
--- a/src/metabase/models/persisted_info.clj
+++ b/src/metabase/models/persisted_info.clj
@@ -74,7 +74,9 @@
{:batched-hydrate :persisted}
[cards]
(when (seq cards)
- (let [existing-ids (db/select-field :card_id PersistedInfo :card_id [:in (map :id cards)])]
+ (let [existing-ids (db/select-field :card_id PersistedInfo
+ :card_id [:in (map :id cards)]
+ :state [:not-in ["off" "deletable"]])]
(map (fn [{id :id :as card}]
(assoc card :persisted (contains? existing-ids id)))
cards))))
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index b1ec229c41d..6567a2e3fc4 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -87,7 +87,7 @@
(dissoc :schedules)))))
(testing "Superusers should see DB details"
- (is (= (db-details)
+ (is (= (assoc (db-details) :can-manage true)
(-> (mt/user-http-request :crowberto :get 200 (format "database/%d" (mt/id)))
(dissoc :schedules))))))
diff --git a/test/metabase/events/persisted_info_test.clj b/test/metabase/events/persisted_info_test.clj
new file mode 100644
index 00000000000..54d4847225a
--- /dev/null
+++ b/test/metabase/events/persisted_info_test.clj
@@ -0,0 +1,19 @@
+(ns metabase.events.persisted-info-test
+ (:require [clojure.test :refer [deftest is]]
+ [metabase.events.persisted-info :as events.persisted-info]
+ [metabase.models :refer [Card Database PersistedInfo]]
+ [metabase.test :as mt]
+ [metabase.util :as u]
+ [toucan.db :as db]))
+
+(deftest event-test
+ (mt/with-temporary-setting-values [persisted-models-enabled true]
+ (mt/with-temp* [Database [db {:options {:persist-models-enabled true}}]
+ Card [card {:database_id (u/the-id db)}]]
+ (events.persisted-info/process-event {:topic :card-create
+ :item card})
+ (is (zero? (count (db/select PersistedInfo :card_id (u/the-id card)))))
+
+ (events.persisted-info/process-event {:topic :card-update
+ :item (assoc card :dataset true)})
+ (is (= "creating" (:state (db/select-one PersistedInfo :card_id (u/the-id card))))))))
--
GitLab