Database Migrations

Question

I'm starting to learn Clojure, and would like feedback on some code I wrote to manage database migrations. Any recommendations to make it more robust, efficient, idiomatic, elegant, etc... are welcome!

(ns myapp.models.migrations
  (:require [clojure.java.jdbc     :as sql]
            [myapp.models.database :as db]))

;;;; Manages database migrations.
;;;;
;;;; Usage:
;;;;
;;;; user=> (migrate!)          ; migrate to the latest version
;;;; user=> (migrate! 20140208) ; migrate to a specific version

(let [db-spec db/spec]

  ;; WARNING: Only works with PostgreSQL!
  ;;
  ;; TODO: Can this be made generic to all databases? Look into using the
  ;; JDBC database metadata to determine if a table exists.
  (defn table-exists? [table-name]
    (-> (sql/query db-spec 
                   ["select count(*) from pg_tables where tablename = ?" table-name])
        first :count pos?))

  ;;; The migrations to apply
  ;;;
  ;;; The order in which migrations are apply is determined by the :version property.
  ;;; Each migration must have :apply and :remove functions so we can migrate up or down.

  (def migration-0 {:version 0
                    :description "Starting point. Does nothing, but allows us to remove all other migrations if we want to."
                    :apply (fn [] nil)
                    :remove (fn [] nil)})

  (def migration-20140208 {:version 20140208
                           :description "Create the articles table."
                           :apply (fn []
                                    (when (not (table-exists? "articles"))
                                      (sql/db-do-commands db-spec (sql/create-table-ddl :articles
                                                                                        [:title "varchar(32)"]
                                                                                        [:content "text"]))))
                           :remove (fn []
                                     (when (table-exists? "articles")
                                       (sql/db-do-commands db-spec (sql/drop-table-ddl :articles))))})

  (def db-migrations [ migration-0
                       migration-20140208 ])

  ;;; Forms for processing the migrations.

  (defn create-migrations-table! []
    (when (not (table-exists? "migrations"))
      (sql/db-do-commands db-spec
                          (sql/create-table-ddl :migrations [:version :int]))))

  (defn drop-migrations-table! []
    (when (table-exists? "migrations")
      (sql/db-do-commands db-spec
                          (sql/drop-table-ddl :migrations))))

  (defn migration-recorded? [migration]
    (create-migrations-table!)
    (-> (sql/query db-spec ["select count(*) from migrations where version = ?" (:version migration)])
        first :count pos?))

  (defn record-migration! [migration]
    (create-migrations-table!)
    (when (not (migration-recorded? migration))
      (sql/insert! db-spec :migrations {:version (:version migration)})))

  (defn erase-migration! [migration]
    (create-migrations-table!)
    (when (migration-recorded? migration)
      (sql/delete! db-spec :migrations ["version = ?" (:version migration)])))

  (defn migrate-up! [to-version]
    (let [filtered-migrations (sort-by :version (filter #(<= (:version %) to-version) db-migrations))]
      (doseq [m filtered-migrations]
        (when (not (migration-recorded? m))
          ((:apply m))
          (record-migration! m)))))

  (defn migrate-down! [to-version]
    (let [filtered-migrations (reverse (sort-by :version (filter #(> (:version %) to-version) db-migrations)))]
      (doseq [m filtered-migrations]
        (when (migration-recorded? m)
          ((:remove m))
          (erase-migration! m)))))

  (defn migrate!  
    ([]
     (let [last-migration (last (sort-by :version db-migrations))]
       (when last-migration (migrate! (:version last-migration)))))

    ([to-version]
     (let [version (or to-version 0)
           migration-exists (not (nil? (some #(= (:version %) version) db-migrations)))
           already-applied (migration-recorded? {:version version})]
       (cond
         (not migration-exists)
           (println (format "migration %s was not found" version))
         already-applied
           (migrate-down! version)
         :else
           (migrate-up! version))))))

Dave Yarwood · Accepted Answer · 2014-03-19 14:11:25Z

Honestly, I think this code looks great! Kudos -- this looks especially good for a beginner to Clojure. I have just a few minor improvements:

(defn create-migrations-table! []
  (when-not (table-exists? "migrations")
    (sql/db-do-commands db-spec
                        (sql/create-table-ddl :migrations [:version :int]))))

Use (when-not x) instead of (when (not x)) -- it'll save you a couple parentheses :)

(defn record-migration! [migration]
  (create-migrations-table!)
  (when-not (migration-recorded? migration)
    (sql/insert! db-spec :migrations {:version (:version migration)})))

(same thing with when-not)

(defn migrate-up! [to-version]
  (let [filtered-migrations (sort-by :version (filter #(<= (:version %) to-version) db-migrations))]
    (doseq [m filtered-migrations]
      (when-not (migration-recorded? m)
        ((:apply m))
        (record-migration! m)))))

(another opportunity to use when-not)

(defn migrate!  
  ([]
   (when-let [last-migration (last (sort-by :version db-migrations))]
     (migrate! (:version last-migration))))
...

Anytime you have a statement of the form (let [x (something)] (when x (do-something))), you can simplify it to (when-let [x (something)] (do-something)).

At the end, I would consider calling migration-exists migration-exists?, since it represents a boolean value.

The only other thing that stood out for me is your inclusion of (create-migrations-table!) in a few of the other functions as the first line... this seems like kind of a work-around, and might potentially cause problems from a functional programming perspective. You might consider taking the (when-not (table-exists? "migrations" ... out of the function definition for create-migrations-table! and including it as a check in the other 3 functions, like this:

(defn create-migrations-table! []
  (sql/db-do-commands db-spec
                      (sql/create-table-ddl :migrations [:version :int])))

(defn record-migration! [migration]
  (when-not (table-exists? "migrations") 
    (create-migrations-table!))
  (when-not (migration-recorded? migration)
    (sql/insert! db-spec :migrations {:version (:version migration)})))

This way seems more intuitive to me -- the create-migrations-table! ought to assume that there isn't already one in existence, and you would expect not to use it unless you're checking (table-exists? "migrations") as a condition. On the other hand, this is wordier, so you may prefer to leave it the way it is for the sake of simplicity.

Stack Exchange Network

Database Migrations

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Database Migrations

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions