From e62908effeb2681d683f73fd19d15dbd136bca05 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 3 Aug 2023 21:45:50 +0700
Subject: [PATCH] Define a new persist-models-enabled setting for database.
(#32825)
---
src/metabase/models/database.clj | 33 +++++++++++++++++++----------
test/metabase/api/database_test.clj | 16 +++++++++++++-
2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index a3074adfb8f..1e2a691f3b3 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -10,10 +10,10 @@
[metabase.models.permissions-group :as perms-group]
[metabase.models.secret :as secret :refer [Secret]]
[metabase.models.serialization :as serdes]
- [metabase.models.setting :as setting]
+ [metabase.models.setting :as setting :refer [defsetting]]
[metabase.plugins.classloader :as classloader]
[metabase.util :as u]
- [metabase.util.i18n :refer [trs]]
+ [metabase.util.i18n :refer [deferred-tru trs]]
[metabase.util.log :as log]
[methodical.core :as methodical]
[toucan2.core :as t2]
@@ -247,6 +247,12 @@
[_database]
[:name :engine])
+(defsetting persist-models-enabled
+ (deferred-tru "Whether to enable models persistence for a specific Database.")
+ :default false
+ :type :boolean
+ :visibility :public
+ :database-local :only)
;;; ---------------------------------------------- Hydration / Util Fns ----------------------------------------------
@@ -300,15 +306,20 @@
(sensitive-fields-for-db db)))))]
(update db :settings (fn [settings]
(when (map? settings)
- (into {}
- (filter (fn [[setting-name _v]]
- (try
- (setting/can-read-setting? setting-name
- (setting/current-user-readable-visibilities))
- (catch Throwable _e
- true)))
- settings))))))
-
+ (m/filter-keys
+ (fn [setting-name]
+ (try
+ (setting/can-read-setting? setting-name
+ (setting/current-user-readable-visibilities))
+ (catch Throwable e
+ ;; there is an known issue with exception is ignored when render API response (#32822)
+ ;; If you see this error, you probably need to define a setting for `setting-name`.
+ ;; But ideally, we should resovle the above issue, and remove this try/catch
+ (log/error e (format "Error checking the readability of %s setting. The setting will be hidden in API response." setting-name))
+ ;; let's be conservative and hide it by defaults, if you want to see it,
+ ;; you need to define it :)
+ false)))
+ settings)))))
json-generator))
;;; ------------------------------------------------ Serialization ----------------------------------------------------
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index df0625ccbf6..4891e62de6b 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -1638,6 +1638,17 @@
(is (= {:database-enable-actions false}
(settings)))))))))
+(deftest log-an-error-if-contains-undefined-setting-test
+ (testing "should log an error message if database contains undefined settings"
+ (t2.with-temp/with-temp [Database {db-id :id} {:settings {:undefined-setting true}}]
+ (is (= "Error checking the readability of :undefined-setting setting. The setting will be hidden in API response."
+ (-> (mt/with-log-messages-for-level :error
+ (testing "does not includes undefined keys by default"
+ (is (not (contains? (:settings (mt/user-http-request :crowberto :get 200 (str "database/" db-id)))
+ :undefined-setting)))))
+ first
+ last))))))
+
(deftest persist-database-test-2
(mt/test-drivers (mt/normal-drivers-with-feature :persist-models)
(mt/dataset test-data
@@ -1662,7 +1673,10 @@
:card_id (:id card))))
(is (true? (t2/select-one-fn (comp :persist-models-enabled :settings)
Database
- :id db-id))))
+ :id db-id)))
+ (is (true? (get-in (mt/user-http-request :crowberto :get 200
+ (str "database/" db-id))
+ [:settings :persist-models-enabled]))))
(testing "it's okay to trigger persist even though the database is already persisted"
(mt/user-http-request :crowberto :post 204 (str "database/" db-id "/persist")))))))))
--
GitLab