From 30e731e3c5766561c041802a6349764adbb802ce Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Tue, 21 Feb 2023 13:00:33 -0600
Subject: [PATCH] Custom migrations (#28175)
* Custom migrations
Current syntax:
specify the migration with
```
- changeSet:
id: v46.00-080
author: dpsutton
comment: Uppercases all Card names
changes:
- customChange:
class: "metabase.db.custom_migrations.ReversibleUppercaseCards"
```
and in the new namespace metabase.db.custom-migrations:
```clojure
(defmigration UppercaseCards
(db/execute! {:update :report_card
:set {:name :%upper.name}}))
(def-reversible-migration ReversibleUppercaseCards
(db/execute! {:update :report_card
:set {:name :%upper.name}})
(db/execute! {:update :report_card
:set {:name :%lower.name}}))
```
* Use db provided by liquibase
* edit docstring
* set *warn-on-reflection* to fix lint error & rebase
---------
Co-authored-by: Noah Moss <noahbmoss@gmail.com>
Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
---
.clj-kondo/config.edn | 2 +
.../src/change/strict.clj | 7 ++-
.../src/change_set/strict.clj | 5 +-
.../test/lint_migrations_file_test.clj | 43 +++++++++++++--
src/metabase/db/custom_migrations.clj | 54 +++++++++++++++++++
src/metabase/db/setup.clj | 1 +
6 files changed, 105 insertions(+), 7 deletions(-)
create mode 100644 src/metabase/db/custom_migrations.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 0b385bfe8ed..f80335a4a88 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -399,6 +399,8 @@
metabase.api.search-test/do-test-users clojure.core/let
metabase.async.api-response-test/with-response clojure.core/let
metabase.dashboard-subscription-test/with-dashboard-sub-for-card clojure.core/let
+ metabase.db.custom-migrations/defmigration clj-kondo.lint-as/def-catch-all
+ metabase.db.custom-migrations/def-reversible-migration clj-kondo.lint-as/def-catch-all
metabase.db.data-migrations/defmigration clojure.core/def
metabase.db.liquibase/with-liquibase clojure.core/let
metabase.db.schema-migrations-test.impl/with-temp-empty-app-db clojure.core/let
diff --git a/bin/lint-migrations-file/src/change/strict.clj b/bin/lint-migrations-file/src/change/strict.clj
index 49b40465d35..bff8cc099e6 100644
--- a/bin/lint-migrations-file/src/change/strict.clj
+++ b/bin/lint-migrations-file/src/change/strict.clj
@@ -38,8 +38,13 @@
(s/def ::createIndex
(s/keys :req-un [::indexName]))
+(s/def :custom-change/class (every-pred string? (complement str/blank?)))
+
+(s/def ::customChange
+ (s/keys :req-un [:custom-change/class]))
+
(s/def ::change
- (s/keys :opt-un [::addColumn ::createTable ::createIndex]))
+ (s/keys :opt-un [::addColumn ::createTable ::createIndex ::customChange]))
(s/def :change.strict.dbms-qualified-sql-change.sql/dbms
string?)
diff --git a/bin/lint-migrations-file/src/change_set/strict.clj b/bin/lint-migrations-file/src/change_set/strict.clj
index 63794fafa9f..8003877c375 100644
--- a/bin/lint-migrations-file/src/change_set/strict.clj
+++ b/bin/lint-migrations-file/src/change_set/strict.clj
@@ -55,7 +55,10 @@
:renameSequence
:renameTable
:renameTrigger
- :renameView})
+ :renameView
+ ;; assumes all custom changes use the `def-migration` or `def-reversible-migration` in
+ ;; metabase.db.custom-migrations
+ :customChange})
(defn- major-version
"Returns major version from id string, e.g. 44 from \"v44.00-034\""
diff --git a/bin/lint-migrations-file/test/lint_migrations_file_test.clj b/bin/lint-migrations-file/test/lint_migrations_file_test.clj
index 19ea98ad8d6..76592fd8a6f 100644
--- a/bin/lint-migrations-file/test/lint_migrations_file_test.clj
+++ b/bin/lint-migrations-file/test/lint_migrations_file_test.clj
@@ -1,12 +1,15 @@
(ns lint-migrations-file-test
(:require
+ [clojure.spec.alpha :as s]
[clojure.test :refer :all]
[lint-migrations-file :as lint-migrations-file]))
-(defn mock-change-set [& keyvals]
+(defn mock-change-set
+ "Returns a \"strict\" migration (id > switch to strict). If you want a non-strict migration send :id 1 in `keyvals`. "
+ [& keyvals]
{:changeSet
(merge
- {:id 1
+ {:id 1000
:author "camsaul"
:comment "Added x.37.0"
:changes [{:whatever {}}]}
@@ -31,6 +34,10 @@
(lint-migrations-file/validate-migrations
{:databaseChangeLog changes}))
+(defn- validate-ex-info [& changes]
+ (try (lint-migrations-file/validate-migrations {:databaseChangeLog changes})
+ (catch Exception e (ex-data e))))
+
(deftest require-unique-ids-test
(testing "Make sure all migration IDs are unique"
(is (thrown-with-msg?
@@ -70,13 +77,12 @@
(testing "[strict only] only allow one change per change set"
(is (= :ok
(validate
- (mock-change-set :changes [(mock-add-column-changes) (mock-add-column-changes)]))))
+ (mock-change-set :id 1 :changes [(mock-add-column-changes) (mock-add-column-changes)]))))
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Extra input"
(validate
- (mock-change-set :id 200
- :changes [(mock-add-column-changes) (mock-add-column-changes)]))))))
+ (mock-change-set :changes [(mock-add-column-changes) (mock-add-column-changes)]))))))
(deftest require-comment-test
(testing "[strict only] require a comment for a change set"
@@ -215,3 +221,30 @@
(validate (mock-change-set :id "v45.12-345"
:changes [(mock-add-column-changes
:columns [(mock-column :constraints {:deleteCascade true})])]))))))
+
+(deftest custom-changes-test
+ (let [change-set (mock-change-set
+ :changes
+ [{:customChange {:class "metabase.db.custom_migrations.ReversibleUppercaseCards"}}])]
+ (is (= :ok
+ (validate change-set))))
+ (testing "missing value"
+ (let [change-set (mock-change-set
+ :changes
+ [{:customChange {}}])
+ ex-info (validate-ex-info change-set)]
+ (is (not= :ok ex-info))))
+ (testing "invalid values"
+ (doseq [bad-value [nil 3 ""]]
+ (let [change-set (mock-change-set
+ :changes
+ [{:customChange {:class bad-value}}])
+ ex-info (validate-ex-info change-set)
+ specific (->> ex-info
+ ::s/problems
+ (some (fn [problem]
+ (when (= (:val problem) bad-value)
+ problem))))]
+ (is (not= :ok ex-info))
+ (is (= (take-last 2 (:via specific))
+ [:change.strict/customChange :custom-change/class]))))))
diff --git a/src/metabase/db/custom_migrations.clj b/src/metabase/db/custom_migrations.clj
new file mode 100644
index 00000000000..ee0fbdb421b
--- /dev/null
+++ b/src/metabase/db/custom_migrations.clj
@@ -0,0 +1,54 @@
+(ns metabase.db.custom-migrations
+ (:require [metabase.util.log :as log]
+ [toucan2.connection :as t2.conn])
+ (:import [liquibase.change.custom CustomTaskChange CustomTaskRollback]
+ liquibase.exception.ValidationErrors))
+
+(set! *warn-on-reflection* true)
+
+(defmacro def-reversible-migration
+ "Define a reversible custom migration. Both the forward and reverse migrations are defined using the same structure,
+ similar to the bodies of multi-arity Clojure functions.
+
+ The first thing in each migration body must be a one-element vector containing a binding to use for the database
+ object provided by Liquibase, so that migrations have access to it if needed. This should typically not be used
+ directly, however, because is also set automatically as the current connection for Toucan 2.
+
+ Example:
+
+ ```clj
+ (def-reversible-migration ExampleMigrationName
+ ([_database]
+ (migration-body))
+
+ ([_database]
+ (migration-body)))"
+ [name [[db-binding-1] & migration-body] [[db-binding-2] reverse-migration-body]]
+ `(defrecord ~name []
+ CustomTaskChange
+ (execute [_# database#]
+ (binding [toucan2.connection/*current-connectable* (.getWrappedConnection (.getConnection database#))]
+ (let [~db-binding-1 database#]
+ ~@migration-body)))
+ (getConfirmationMessage [_#]
+ (str "Custom migration: " ~name))
+ (setUp [_#])
+ (validate [_# _database#]
+ (ValidationErrors.))
+ (setFileOpener [_# _resourceAccessor#])
+
+ CustomTaskRollback
+ (rollback [_# database#]
+ (binding [toucan2.connection/*current-connectable* (.getWrappedConnection (.getConnection database#))]
+ (let [~db-binding-2 database#]
+ ~@reverse-migration-body)))))
+
+(defn no-op
+ "No-op logging rollback function"
+ [n]
+ (log/info "No rollback for: " n))
+
+(defmacro defmigration
+ "Define a custom migration."
+ [name migration-body]
+ `(def-reversible-migration ~name ~migration-body ([~'_] (no-op ~(str name)))))
diff --git a/src/metabase/db/setup.clj b/src/metabase/db/setup.clj
index 470011cd191..a3a1b75a5ef 100644
--- a/src/metabase/db/setup.clj
+++ b/src/metabase/db/setup.clj
@@ -9,6 +9,7 @@
(:require
[honey.sql :as sql]
[metabase.db.connection :as mdb.connection]
+ metabase.db.custom-migrations ;; load our custom migrations
[metabase.db.jdbc-protocols :as mdb.jdbc-protocols]
[metabase.db.liquibase :as liquibase]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
--
GitLab