CI: Check Liquibase 000_migrations.yaml against core.spec (metabase#1…

…4097)

* add step to validate Liquibase migrations to CI

* addColumn should only allow one column.

* Only allow one column added per addColumn change

* Add strict-mode for change sets

* Check that migrations are in order/IDs are unique

* Fix CI

* Tests for the linter!!

* Require remarks when adding new Tables

* Fix CI

.circleci/config.yml

@@ -570,6 +570,25 @@ jobs:
                 command: ./bin/i18n/build-translation-resources
                 no_output_timeout: 2m
 
+  check-migrations:
+    executor:
+      metabase-ci
+    steps:
+      - attach-workspace
+      - create-checksum-file:
+          filename: .MIGRATIONS-CHECKSUM
+          find-args: resources/migrations -type f -name '*.yaml'
+      - create-checksum-file:
+          filename: .MIGRATIONS-LINTER-CHECKSUMS
+          find-args: bin/lint-migrations-file -type f -name '*.clj' -or -name 'deps.edn'
+      - run-on-change:
+          cache-key: check-migrations-{{ checksum ".MIGRATIONS-CHECKSUM" }}-{{ checksum ".MIGRATIONS-LINTER-CHECKSUMS" }}
+          steps:
+            - run:
+                name: Verify Liquibase Migrations
+                command: ./bin/lint-migrations-file.sh
+                no_output_timeout: 10m
+
 
 ########################################################################################################################
 #                                                       BACKEND                                                        #
@@ -735,6 +754,11 @@ jobs:
                 command: |
                   cd /home/circleci/metabase/metabase/bin/release && clojure -M:test
                 no_output_timeout: 10m
+            - run:
+                name: Run Liquibase migrations linter tests
+                command: |
+                  cd /home/circleci/metabase/metabase/bin/lint-migrations-file && clojure -M:test
+                no_output_timeout: 10m
 
 
 ########################################################################################################################
@@ -1048,6 +1072,10 @@ workflows:
           requires:
             - fe-deps
 
+      - check-migrations:
+          requires:
+            - checkout
+
       - be-deps:
           requires:
             - checkout

bin/build-drivers/deps.edn

@@ -6,7 +6,7 @@
   commons-codec/commons-codec {:mvn/version "1.14"}
   hiccup/hiccup               {:mvn/version "1.0.5"}
   io.forward/yaml             {:mvn/version "1.0.9"}  ; don't upgrade to 1.0.10 -- doesn't work on Java 8 (!)
-  org.flatland/ordered        {:mvn/version "1.5.9"}  ; used by io.forward/yaml
+  org.flatland/ordered        {:mvn/version "1.5.9"}  ; used by io.forward/yaml -- need the newer version
   stencil/stencil             {:mvn/version "0.5.0"}}
 
  :aliases

bin/lint-migrations-file.sh

@@ -0,0 +1,13 @@
+#! /usr/bin/env bash
+
+set -euo pipefail
+
+# switch to project root directory if we're not already there
+script_directory=`dirname "${BASH_SOURCE[0]}"`
+cd "$script_directory/.."
+
+source "./bin/check-clojure-cli.sh"
+check_clojure_cli
+
+cd bin/lint-migrations-file
+clojure -M -m lint-migrations-file $@

bin/lint-migrations-file/README.md

@@ -0,0 +1,20 @@
+Linter that validates the Liquibase migrations file against a spec. This lets us check and enforce additional
+constraints for the `000_migrations.yaml` file (e.g. make sure you're not using duplicate migration IDs, or including
+more than one change in a single `changeSet`).
+
+Older/existing migrations are validated with a less-strict spec; newer ones use a stricter spec that enforces some
+additional constraints. Migrations 172 and newer use the stricter spec. The less-strict specs are in `x.unstrict`
+namespaces while equivalent stricter ones are in `x.strict` namespaces.
+
+Run the linter with
+
+```sh
+./bin/lint-migrations-yaml.sh
+```
+
+Add some tests for the checks you add here the `test/` directory; run them with
+
+```sh
+cd bin/lint-migrations-yaml
+clojure -M:test
+```

bin/lint-migrations-file/deps.edn

@@ -0,0 +1,11 @@
+{:paths ["src"]
+
+ :deps
+ {io.forward/yaml      {:mvn/version "1.0.9"}  ; don't upgrade to 1.0.10 -- doesn't work on Java 8 (!)
+  org.flatland/ordered {:mvn/version "1.5.9"}} ; used by io.forward/yaml -- need the newer version
+
+ :aliases
+ {:test {:extra-paths ["test"]
+         :extra-deps  {com.cognitect/test-runner {:git/url "https://github.com/cognitect-labs/test-runner.git"
+                                                  :sha     "209b64504cb3bd3b99ecfec7937b358a879f55c1"}}
+         :main-opts   ["-m" "cognitect.test-runner"]}}}

bin/lint-migrations-file/src/change/common.clj

@@ -0,0 +1,11 @@
+(ns change.common
+  (:require [clojure.spec.alpha :as s]))
+
+(s/def ::tableName
+  string?)
+
+(s/def ::addColumn
+  (s/keys :req-un [::tableName]))
+
+(s/def ::createTable
+  (s/keys :req-un [::tableName]))

bin/lint-migrations-file/src/change/strict.clj

@@ -0,0 +1,33 @@
+(ns change.strict
+  (:require change.common
+            [clojure.spec.alpha :as s]
+            column.strict))
+
+(comment change.common/keep-me
+         column.strict/keep-me)
+
+(s/def ::column
+  (s/keys :req-un [:column.strict/column]))
+
+(s/def :change.strict.add-column/columns
+  (s/alt :column ::column))
+
+(s/def ::addColumn
+  (s/merge
+   :change.common/addColumn
+   (s/keys :req-un [:change.strict.add-column/columns])))
+
+(s/def ::remarks
+  string?)
+
+(s/def :change.strict.create-table/columns
+  (s/+ (s/alt :column ::column)))
+
+(s/def ::createTable
+  (s/merge
+   :change.common/createTable
+   ;; remarks are required for new tables in strict mode
+   (s/keys :req-un [:change.strict.create-table/columns ::remarks])))
+
+(s/def ::change
+  (s/keys :opt-un [::addColumn ::createTable]))

bin/lint-migrations-file/src/change/unstrict.clj

@@ -0,0 +1,32 @@
+(ns change.unstrict
+  (:require change.common
+            [clojure.spec.alpha :as s]
+            column.unstrict))
+
+(comment change.common/keep-me
+         column.unstrict/keep-me)
+
+(s/def ::column
+  (s/keys :req-un [:column.unstrict/column]))
+
+(s/def :change.unstrict.add-column/columns
+  (s/alt :column ::column))
+
+(s/def ::addColumn
+  (s/merge
+   :change.common/addColumn
+   (s/keys :req-un [:change.unstrict.add-column/columns])))
+
+(s/def ::remarks string?)
+
+(s/def :change.unstrict.create-table/columns
+  (s/+ (s/alt :column ::column)))
+
+(s/def ::createTable
+  (s/merge
+   :change.common/createTable
+   (s/keys :req-un [:change.unstrict.create-table/columns]
+           :opt-un [::remarks])))
+
+(s/def ::change
+  (s/keys :opt-un [::addColumn ::createTable]))

bin/lint-migrations-file/src/change_set/common.clj

@@ -0,0 +1,18 @@
+(ns change-set.common
+  (:require [clojure.spec.alpha :as s]))
+
+(s/def ::id
+  (s/or
+   :int int?
+   :int-string (s/and
+                string?
+                (fn [^String s]
+                  (try
+                    (Integer/parseInt s)
+                    (catch Throwable _
+                      false))))))
+
+(s/def ::author string?)
+
+(s/def ::change-set
+  (s/keys :req-un [::id ::author]))

bin/lint-migrations-file/src/change_set/strict.clj

@@ -0,0 +1,22 @@
+(ns change-set.strict
+  (:require change-set.common
+            change.strict
+            [clojure.spec.alpha :as s]))
+
+(comment change-set.common/keep-me
+         change.strict/keep-me)
+
+;; comment is required for strict change set spec
+(s/def ::comment
+  (s/and
+   string?
+   (partial re-find #"Added [\d.x]+")))
+
+;; only one change allowed per change set for the strict schema.
+(s/def ::changes
+  (s/alt :change :change.strict/change))
+
+(s/def ::change-set
+  (s/merge
+   :change-set.common/change-set
+   (s/keys :req-un [::changes ::comment])))

bin/lint-migrations-file/src/change_set/unstrict.clj

@@ -0,0 +1,19 @@
+(ns change-set.unstrict
+  (:require change-set.common
+            change.unstrict
+            [clojure.spec.alpha :as s]))
+
+(comment change-set.common/keep-me
+         change.unstrict/keep-me)
+
+(s/def ::comment string?)
+
+;; unstrict change set: one or more changes
+(s/def ::changes
+  (s/+ :change.unstrict/change))
+
+(s/def ::change-set
+  (s/merge
+   :change-set.common/change-set
+   (s/keys :req-un [::changes]
+           :opt-un [::comment])))

bin/lint-migrations-file/src/column/common.clj

@@ -0,0 +1,25 @@
+(ns column.common
+  (:require [clojure.spec.alpha :as s]))
+
+(s/def ::name string?)
+(s/def ::type string?)
+(s/def ::remarks string?)
+
+(s/def :column.common.constraints/onDelete
+  (fn [_]
+    "Don't try to use onDelete in constraints! onDelete is only for addForeignKeyConstraints. Use deleteCascade!"
+    false))
+
+(s/def ::constraints
+  ;; TODO -- require foreignKeyName if this is an FK
+  (s/keys :opt-un [:column.common.constraints/nullable
+                   :column.common.constraints/references
+                   :column.common.constraints/foreignKeyName
+                   :column.common.constraints/deferrable
+                   :column.common.constraints/initiallyDeferred
+                   :column.common.constraints/deleteCascade
+                   :column.common.constraints/onDelete]))
+
+(s/def ::column
+  (s/keys :req-un [::name ::type]
+          :opt-un [::remarks ::constraints]))

bin/lint-migrations-file/src/column/strict.clj

@@ -0,0 +1,9 @@
+(ns column.strict
+  (:require [clojure.spec.alpha :as s]
+            column.common))
+
+(comment column.common/keep-me)
+
+(s/def ::column
+  (s/merge
+   :column.common/column))

bin/lint-migrations-file/src/column/unstrict.clj

@@ -0,0 +1,9 @@
+(ns column.unstrict
+  (:require [clojure.spec.alpha :as s]
+            column.common))
+
+(comment column.common/keep-me)
+
+(s/def ::column
+  (s/merge
+   :column.common/column))

bin/lint-migrations-file/src/lint_migrations_file.clj

@@ -0,0 +1,98 @@
+(ns lint-migrations-file
+  (:require [change-set strict unstrict]
+            [clojure.java.io :as io]
+            [clojure.pprint :as pprint]
+            [clojure.spec.alpha :as s]
+            [yaml.core :as yaml]))
+
+(comment change-set.strict/keep-me
+         change-set.unstrict/keep-me)
+
+;; just print ordered maps like normal maps.
+(defmethod print-method flatland.ordered.map.OrderedMap
+  [m writer]
+  (print-method (into {} m) writer))
+
+(s/def ::migrations
+  (s/keys :req-un [::databaseChangeLog]))
+
+(defn- change-set-ids [change-log]
+  (for [{{id :id} :changeSet} change-log
+        :when                 id]
+    (if (string? id)
+      (Integer/parseInt id)
+      id)))
+
+(defn distinct-change-set-ids? [change-log]
+  ;; there are actually two migration 32s, so that's the only exception we're allowing.
+  (let [ids (remove (partial = 32) (change-set-ids change-log))]
+    ;; can't apply distinct? with so many IDs
+    (= (count ids) (count (set ids)))))
+
+(defn change-set-ids-in-order? [change-log]
+  (let [ids (change-set-ids change-log)]
+    (= ids (sort ids))))
+
+;; TODO -- change sets must be distinct by ID.
+(s/def ::databaseChangeLog
+  (s/and distinct-change-set-ids?
+         change-set-ids-in-order?
+         (s/+ (s/alt :property  (s/keys :req-un [::property])
+                     :changeSet (s/keys :req-un [::changeSet])))))
+
+(def strict-change-set-cutoff
+  "All change sets with an ID >= this number will be validated with the strict spec."
+  172)
+
+(defn change-set-validation-level [{id :id}]
+  (let [id (cond
+             (integer? id) id
+             (string? id)  (Integer/parseInt ^String id)
+             :else         (throw (ex-info "Invalid ID" {:id id})))]
+    (if (>= id strict-change-set-cutoff)
+      :strict
+      :unstrict)))
+
+(defmulti change-set
+  change-set-validation-level)
+
+(defmethod change-set :strict
+  [_]
+  :change-set.strict/change-set)
+
+(defmethod change-set :unstrict
+  [_]
+  :change-set.unstrict/change-set)
+
+(s/def ::changeSet
+  (s/multi-spec change-set change-set-validation-level))
+
+(defn validate-migrations [migrations]
+  (when (= (s/conform ::migrations migrations) :clojure.spec.alpha/invalid)
+    (let [data (s/explain-data ::migrations migrations)]
+      (throw (ex-info (str "Validation failed:\n" (with-out-str (pprint/pprint (mapv #(dissoc % :val)
+                                                                                     (:clojure.spec.alpha/problems data)))))
+                      (or (dissoc data :clojure.spec.alpha/value) {})))))
+  :ok)
+
+(def filename
+  "../../resources/migrations/000_migrations.yaml")
+
+(defn migrations []
+  (let [file (io/file filename)]
+    (assert (.exists file) (format "%s does not exist" filename))
+    (yaml/from-file file)))
+
+(defn- validate-all []
+  (validate-migrations (migrations)))
+
+(defn -main []
+  (println "Check Liquibase migrations file...")
+  (try
+    (validate-all)
+    (println "Ok.")
+    (System/exit 0)
+    (catch Throwable e
+      (pprint/pprint (Throwable->map e))
+      (println (.getMessage e))
+      (System/exit 1))))