weavejester / ragtime

Database-independent migration library
Eclipse Public License 1.0
610 stars 85 forks source link

Abstract out jdbc project #135

Closed anthonyshull closed 5 years ago

anthonyshull commented 5 years ago

I am using ragtime with ClickHouse. Everything in the jdbc library works except removing an entry from the migrations table. In ClickHouse the syntax for DELETE is different. So, I created a DataStore. I then also had to create a Migration because the jdbc library I'm using doesn't implement sql/execute!.

In order to do this I had to essentially copy all of the code that grabs SQL files and converts them to Migrations. It should be possible to easily leverage all of that work by moving it outside of the jdbc namespace.

Ideally, you could pass a record creating function in and get any kind of Migration out.

(load-directory ->ClickHouseMigration "./resources/migrations")

You could then use a partial to define the regular JDBC function:

(def load-sql-directory (partial load-directory ->SqlMigration)).

I'm willing to fork the project and do the work to abstract this out. I just want to know if it would be accepted before doing so.

Thanks!

weavejester commented 5 years ago

You could use the JDBC load-directory function, then map over the resulting records and replace them with your own. What's your opinion on that solution?

anthonyshull commented 5 years ago

The issue is the call to sql-migration at the end of the load-files function.

(defmethod load-files ".sql" [files]
  (for [[id files] (->> files
                        (group-by (comp first sql-file-parts))
                        (sort-by key))]
    (let [{:strs [up down]} (group-by (comp second sql-file-parts) files)]
      (sql-migration
       {:id   (basename id)
        :up   (vec (mapcat read-sql (sort-by str up)))
        :down (vec (mapcat read-sql (sort-by str down)))}))))

That's what calls sql/execute! that the jdbc I'm using doesn't support.

weavejester commented 5 years ago

sql-migration doesn't call sql/execute!. It just creates a record: https://github.com/weavejester/ragtime/blob/master/jdbc/src/ragtime/jdbc.clj#L93

anthonyshull commented 5 years ago

Yeah, sorry for the confusion. It doesn’t call sql/execute! but it returns a Migration that does. My suggestion is that you can pass a different migration creation function. In my case my jdbc doesn’t support execute! so I have to rewrite all of the helpers that grab a file, etc. Does that make sense?

anthonyshull commented 5 years ago

So my suggestion would mean having support for SQL stores that don’t have standard JDBC implementations—to separate out a SQL namespace from the JDBC one.

anthonyshull commented 5 years ago

I can work up a pull request. I just want to make sure it isn’t a complete waste of time.

weavejester commented 5 years ago

Yeah, sorry for the confusion. It doesn’t call sql/execute! but it returns a Migration that does.

Right, but the migration it returns is just a record. You can convert it into anything you want. You can treat load-directory as returning a collection of maps.

anthonyshull commented 5 years ago

Ok, great. That worked. I just called jdbc/load-directory and then mapped over the results to convert them to my clickhouse migration record.