HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

Database Migrations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
databasemigrationsstackoverflow

Problem

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? "articl

Solution

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.

Code Snippets

(defn create-migrations-table! []
  (when-not (table-exists? "migrations")
    (sql/db-do-commands db-spec
                        (sql/create-table-ddl :migrations [:version :int]))))
(defn record-migration! [migration]
  (create-migrations-table!)
  (when-not (migration-recorded? migration)
    (sql/insert! 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!  
  ([]
   (when-let [last-migration (last (sort-by :version db-migrations))]
     (migrate! (:version last-migration))))
...
(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)})))

Context

StackExchange Code Review Q#41568, answer score: 3

Revisions (0)

No revisions yet.