patternsqlMinor
Database Migrations
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
```
(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:
Use
(same thing with
(another opportunity to use
Anytime you have a statement of the form
At the end, I would consider calling
The only other thing that stood out for me is your inclusion of
This way seems more intuitive to me -- the
(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.