From b2d7687f36a3ad9bc8e838546e69b4ae7d440dd4 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Mon, 27 Feb 2023 17:14:58 +0200
Subject: [PATCH] If actions are not supported, actions should be disabled for
a database (#28607)
* Add comment for database-enable-actions
* Enforce actions not supported implies actions are disabled
* Remove test and update check
* Fix typos
* Revert "Remove test and update check"
This reverts commit 8b1f14f44bd97514cd8ce0c9b0a5df048d713b16.
* Fix test
---
src/metabase/models/database.clj | 15 +++++++++++++--
test/metabase/models/database_test.clj | 16 ++++++++++++++++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 10b7d8b3eb1..754d1cc7eb6 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -156,17 +156,20 @@
[{new-metadata-schedule :metadata_sync_schedule,
new-fieldvalues-schedule :cache_field_values_schedule,
new-engine :engine
+ new-settings :settings
:as database}]
(let [{is-sample? :is_sample
old-metadata-schedule :metadata_sync_schedule
old-fieldvalues-schedule :cache_field_values_schedule
+ existing-settings :settings
existing-engine :engine
existing-name :name} (db/select-one [Database
:metadata_sync_schedule
:cache_field_values_schedule
:engine
:name
- :is_sample] :id (u/the-id database))
+ :is_sample
+ :settings] :id (u/the-id database))
new-engine (some-> new-engine keyword)]
(if (and is-sample?
new-engine
@@ -194,7 +197,15 @@
(schedule-tasks!
(assoc database
:metadata_sync_schedule new-metadata-schedule
- :cache_field_values_schedule new-fieldvalues-schedule)))))))))
+ :cache_field_values_schedule new-fieldvalues-schedule)))))
+ ;; This maintains a constraint that if a driver doesn't support actions, it can never be enabled
+ ;; If we drop support for actions for a driver, we'd need to add a migration to disable actions for all databases
+ (when (and (:database-enable-actions (or new-settings existing-settings))
+ (not (driver/database-supports? (or new-engine existing-engine) :actions database)))
+ (throw (ex-info (trs "The database does not support actions.")
+ {:status-code 400
+ :existing-engine existing-engine
+ :new-engine new-engine})))))))
(defn- pre-insert [{:keys [details], :as database}]
(-> (cond-> database
diff --git a/test/metabase/models/database_test.clj b/test/metabase/models/database_test.clj
index 356ab042d31..5906948b736 100644
--- a/test/metabase/models/database_test.clj
+++ b/test/metabase/models/database_test.clj
@@ -94,6 +94,22 @@
"id" 3}
(encode-decode pg-db)))))))
+(deftest driver-supports-actions-and-database-enable-actions-test
+ (mt/test-drivers #{:sqlite}
+ (testing "Updating database-enable-actions to true should fail if the engine doesn't support actions"
+ (mt/with-temp Database [database {:engine :sqlite}]
+ (is (= false (driver/database-supports? :sqlite :actions database)))
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"The database does not support actions."
+ (db/update! Database (:id database) :settings {:database-enable-actions true})))))
+ (testing "Updating the engine when database-enable-actions is true should fail if the engine doesn't support actions"
+ (mt/with-temp Database [database {:engine :h2 :settings {:database-enable-actions true}}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"The database does not support actions."
+ (db/update! Database (:id database) :engine :sqlite)))))))
+
(deftest sensitive-data-redacted-test
(let [encode-decode (fn [obj] (decode (encode obj)))
project-id "random-project-id" ; the actual value here doesn't seem to matter
--
GitLab