yogthos / migratus

MIGRATE ALL THE THINGS!
647 stars 95 forks source link

Code-based migration fails before fn runs #152

Closed bobfejes closed 6 years ago

bobfejes commented 6 years ago

Hi, I'm using migratus 1.1.1, and when trying to run a code-based migration, it fails with:

ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.base/java.lang.String  clojure.edn/read-string (edn.clj:46)
       :trace [[clojure.edn$read_string invokeStatic "edn.clj" 46]
               [clojure.edn$read_string invokeStatic "edn.clj" 37]
               [clojure.edn$read_string invoke "edn.clj" 37]
               [migratus.migration.edn$fn__6951 invokeStatic "edn.clj" 44]
               [migratus.migration.edn$fn__6951 invoke "edn.clj" 41]
...

Looks like line 44 in edn.clj is expecting payload to be the content of the .edn file as a string, but in reality it's a clojure map at that point.

Here's what my .edn looks like:

{:ns support.code-migrations
 :up-fn convert-something
 :down-fn nil}

Thanks for the nice lib!

-Bob

yogthos commented 6 years ago

I'm not able to replicate the issue locally. Here's the setup I've got. There's an EDN file in resources/migrations that looks as follows:

{:ns migratus-test.migrate
 :up-fn migrate-up
 :down-fn migrate-down}

then I have the following namespace for migrations:

(ns migratus-test.migrate)

(defn migrate-up [config]
  (println "migrating up"))

(defn migrate-down [config]
  (println "migrating down"))

and a core namespace:

(ns migratus-test.core
  (:require [migratus.core :as migratus]
            [migratus-test.migrate :as m]))

(def config {:store :database :db {:connection-uri "jdbc:sqlite:foo_dev.db"}})

(defn -main []
  (migratus/migrate config))

project.clj looks as follows:

(defproject migratus-test "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.9.0"]
                 [migratus "1.0.9"]
                 [org.xerial/sqlite-jdbc "3.25.2"]]
  :main migratus-test.core)

lein run results in the following:

Oct 04, 2018 7:53:59 PM clojure.tools.logging$eval888$fn__891 invoke
INFO: Starting migrations
Oct 04, 2018 7:53:59 PM clojure.tools.logging$eval888$fn__891 invoke
INFO: creating migration table 'schema_migrations'
Oct 04, 2018 7:53:59 PM clojure.tools.logging$eval888$fn__891 invoke
INFO: Running up for [20111206155000]
Oct 04, 2018 7:53:59 PM clojure.tools.logging$eval888$fn__891 invoke
INFO: Up 20111206155000-create-tables
migrating up
Oct 04, 2018 7:53:59 PM clojure.tools.logging$eval888$fn__891 invoke
INFO: Ending migrations
bobfejes commented 6 years ago

Very strange. I did insert a (println (class payload) payload) in (defmethod proto/make-migration* :edn ... and before throwing the exception it shows:

clojure.lang.PersistentArrayMap {:up {:ns support.code-migrations
 :up-fn convert-something
 :down-fn none}
}

Puzzling. I'll try to work up a stripped-down example like yours tomorrow.

yogthos commented 6 years ago

Yeah looks like it's getting parsed before hand in your case. If you could put up a sample project with the issue, I can help tracing the issue there as well. It looks maybe there is a different path to make-migration* that parses the file before hand depending on how migratus is run.

bobfejes commented 6 years ago

I got it working, thanks for the quick response. Looks like a dot in the filename causes the error. Like: 20181003223323-convert-stuff.up.edn and 20181003223323-convert-stuff.foo.edn both fail. But if I remove .up and .foo from the file names, they work fine.

yogthos commented 6 years ago

Oh interesting. Good to hear it's working, but would be good to improve the error since it's quite confusing.