diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 92d28120c2b6266cf42adfb308a39c08cf45bd5c..3ce7afc612d5f1e05a1d3bb2811815ed1c4f89e0 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -135,36 +135,48 @@
database))
(defn- pre-update
- [{new-metadata-schedule :metadata_sync_schedule, new-fieldvalues-schedule :cache_field_values_schedule, :as database}]
- (u/prog1 (handle-secrets-changes database)
- ;; TODO - this logic would make more sense in post-update if such a method existed
- ;; if the sync operation schedules have changed, we need to reschedule this DB
- (when (or new-metadata-schedule new-fieldvalues-schedule)
- (let [{old-metadata-schedule :metadata_sync_schedule
- old-fieldvalues-schedule :cache_field_values_schedule
- existing-engine :engine
- existing-name :name} (db/select-one [Database
- :metadata_sync_schedule
- :cache_field_values_schedule
- :engine
- :name]
- :id (u/the-id database))
- ;; if one of the schedules wasn't passed continue using the old one
- new-metadata-schedule (or new-metadata-schedule old-metadata-schedule)
- new-fieldvalues-schedule (or new-fieldvalues-schedule old-fieldvalues-schedule)]
- (when-not (= [new-metadata-schedule new-fieldvalues-schedule]
- [old-metadata-schedule old-fieldvalues-schedule])
- (log/info
- (trs "{0} Database ''{1}'' sync/analyze schedules have changed!" existing-engine existing-name)
- "\n"
- (trs "Sync metadata was: ''{0}'' is now: ''{1}''" old-metadata-schedule new-metadata-schedule)
- "\n"
- (trs "Cache FieldValues was: ''{0}'', is now: ''{1}''" old-fieldvalues-schedule new-fieldvalues-schedule))
- ;; reschedule the database. Make sure we're passing back the old schedule if one of the two wasn't supplied
- (schedule-tasks!
- (assoc database
- :metadata_sync_schedule new-metadata-schedule
- :cache_field_values_schedule new-fieldvalues-schedule)))))))
+ [{new-metadata-schedule :metadata_sync_schedule,
+ new-fieldvalues-schedule :cache_field_values_schedule,
+ new-engine :engine
+ :as database}]
+ (let [{is-sample? :is_sample
+ old-metadata-schedule :metadata_sync_schedule
+ old-fieldvalues-schedule :cache_field_values_schedule
+ 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))
+ new-engine (some-> new-engine keyword)]
+ (if (and is-sample?
+ new-engine
+ (not= new-engine existing-engine))
+ (throw (ex-info (trs "The engine on a sample database cannot be changed.")
+ {:status-code 400
+ :existing-engine existing-engine
+ :new-engine new-engine}))
+ (u/prog1 (handle-secrets-changes database)
+ ;; TODO - this logic would make more sense in post-update if such a method existed
+ ;; if the sync operation schedules have changed, we need to reschedule this DB
+ (when (or new-metadata-schedule new-fieldvalues-schedule)
+ ;; if one of the schedules wasn't passed continue using the old one
+ (let [new-metadata-schedule (or new-metadata-schedule old-metadata-schedule)
+ new-fieldvalues-schedule (or new-fieldvalues-schedule old-fieldvalues-schedule)]
+ (when (not= [new-metadata-schedule new-fieldvalues-schedule]
+ [old-metadata-schedule old-fieldvalues-schedule])
+ (log/info
+ (trs "{0} Database ''{1}'' sync/analyze schedules have changed!" existing-engine existing-name)
+ "\n"
+ (trs "Sync metadata was: ''{0}'' is now: ''{1}''" old-metadata-schedule new-metadata-schedule)
+ "\n"
+ (trs "Cache FieldValues was: ''{0}'', is now: ''{1}''" old-fieldvalues-schedule new-fieldvalues-schedule))
+ ;; reschedule the database. Make sure we're passing back the old schedule if one of the two wasn't supplied
+ (schedule-tasks!
+ (assoc database
+ :metadata_sync_schedule new-metadata-schedule
+ :cache_field_values_schedule new-fieldvalues-schedule)))))))))
(defn- pre-insert [database]
(-> database
diff --git a/src/metabase/sample_data.clj b/src/metabase/sample_data.clj
index 9d8c31019642550dd51e788fa54e332cda5a0d12..878b7b60cbaf80aa7db7e24902080c1b5e994cf3 100644
--- a/src/metabase/sample_data.clj
+++ b/src/metabase/sample_data.clj
@@ -38,6 +38,7 @@
(defn update-sample-database-if-needed!
"Update the path to the sample database DB if it exists in case the JAR has moved."
[]
- (when-let [db (Database :is_sample true)]
- (db/update! Database (:id db)
- :details (db-details))))
+ (when-let [sample-db (Database :is_sample true)]
+ (let [intended (db-details)]
+ (when (not= (:details sample-db) intended)
+ (db/update! Database (:id sample-db) :details intended)))))
diff --git a/test/metabase/models/database_test.clj b/test/metabase/models/database_test.clj
index 50c1e5d25b66c46a07aafbd0718e0465eb2b780b..2d608bba3056108db02a23e75d35619e7933bcba 100644
--- a/test/metabase/models/database_test.clj
+++ b/test/metabase/models/database_test.clj
@@ -6,7 +6,7 @@
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.models :refer [Database]]
- [metabase.models.database :as mdb]
+ [metabase.models.database :as database]
[metabase.models.permissions :as perms]
[metabase.models.secret :as secret :refer [Secret]]
[metabase.models.user :as user]
@@ -62,7 +62,7 @@
(let [encode-decode (fn [obj] (decode (encode obj)))
project-id "random-project-id" ; the actual value here doesn't seem to matter
;; this is trimmed for the parts we care about in the test
- pg-db (mdb/map->DatabaseInstance
+ pg-db (database/map->DatabaseInstance
{:description nil
:name "testpg"
:details {:additional-options nil
@@ -80,7 +80,7 @@
:tunnel-user "a-tunnel-user"
:tunnel-private-key-passphrase "Password1234"}
:id 3})
- bq-db (mdb/map->DatabaseInstance
+ bq-db (database/map->DatabaseInstance
{:description nil
:name "testbq"
:details {:use-service-account nil
@@ -159,9 +159,9 @@
(driver.u/sensitive-fields :test-sensitive-driver))))
(testing "get-sensitive-fields-for-db returns default fields for null or empty database map"
(is (= driver.u/default-sensitive-fields
- (mdb/sensitive-fields-for-db nil)))
+ (database/sensitive-fields-for-db nil)))
(is (= driver.u/default-sensitive-fields
- (mdb/sensitive-fields-for-db {})))))
+ (database/sensitive-fields-for-db {})))))
(deftest secret-db-details-integration-test
(testing "manipulating secret values in db-details works correctly"
@@ -216,6 +216,20 @@
(is (nil? (db/select-one Secret :id secret-id))
(format "Secret ID %d was not removed from the app DB" secret-id)))))))))))
+(deftest user-may-not-update-sample-database-test
+ (mt/with-temp Database [{:keys [id details] :as sample-database} {:engine :h2
+ :is_sample true
+ :name "Sample Database"
+ :details {:db "./resources/sample-database.db;USER=GUEST;PASSWORD=guest"}}]
+ (testing " updating the engine of a sample database is not allowed"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"The engine on a sample database cannot be changed."
+ (db/update! Database id :engine :sqlite))))
+ (testing " updating other attributes of a sample database is allowed"
+ (db/update! Database id :name "My New Name")
+ (is (= "My New Name" (db/select-one-field :name Database :id id))))))
+
(driver/register! ::test, :abstract? true)
(deftest preserve-driver-namespaces-test