search

yogthos / migratus

MIGRATE ALL THE THINGS!
642 stars 93 forks source link

switch to next.jdbc (or provide a choice?) #182

Closed kirillsalykin closed 2 years ago

kirillsalykin commented 4 years ago

AFAIK java.jdbc will not be maintained anymore and next.jdbc is the successor.

That it seems logical to migrate to it.

Happy to make the PR.

yogthos commented 4 years ago

Hi, agree with switching and a PR would be much appreciated. :)

kirillsalykin commented 2 years ago

Hi @yogthos ,

I've finally started to work on the MR :)

Have a question regarding https://github.com/yogthos/migratus/blob/master/test/migratus/test/migration/sql.clj#L13

This syntax not supported by next.jdbc, :dbtype/:dbname is preffered way.

Is it a showstopper?

yogthos commented 2 years ago

Hi, that's great news!

And I think the best way to address this might be to just accept both old style keys and the new ones for jdbc.next. Then we could just do the translation internally where possible. In cases where that doesn't work I'm ok with making this a breaking change that could be documented in the changelog.

ieugen commented 2 years ago

Hi, any news on this ? We are considering moving to next.jdbc and reducing our dependencies.

kirillsalykin commented 2 years ago

Hi, sorry was busy with something else and not a priority for me atm.

Feel free to make the MR ;)

ieugen commented 2 years ago

Thanks for the fast response. It's my understanding that you did not manage to have any work done on this. I will see if I can find the time to do it or ask for client time to work on this.

ieugen commented 2 years ago

@yogthos : do you have any suggestions on how to approach this issue?

yogthos commented 2 years ago

Just what I mentioned above with keeping backwards compatibility with the current approach.

ieugen commented 2 years ago

So I am looking at the code to figure out which is the best way to approach the issue. I will add some info here to serve as documentation. clojure jdbc is used in following places:

src/migratus/database.clj:    [clojure.java.jdbc :as sql]
src/migratus/migration/sql.clj:    [clojure.java.jdbc :as sql]
test/migratus/test/database.clj:            [clojure.java.jdbc :as sql]
test/migratus/test/migration/sql.clj:            [clojure.java.jdbc :as sql]

There is a migration guide https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.2.780/doc/migration-from-clojure-java-jdbc .

@yogthos : So far I might be able to have a PR for this and translate configuration internally. The easiest for me (at this point) would be to replace java.jdbc with next.jdbc and drop the other one. This would break existing clients and would probably mean a major release. The user API might be kept and I think we might make internal conversions.

One of the my goals is to have a single dependency on the classpath: either clojure.jdbc or next.jdbc, not both. Supporting both clojure.jdbc and next.jdbc would require (IMO) splitting the code in different namespaces and refactoring some internal functions (not sure how extensive) to be able to use one API or the other. Or perhaps do another protocol implementation for database.

I will investigate further.

yogthos commented 2 years ago

I think it's probably reasonable to switch to next.jdbc wholesale, it's the one that's actively maintained and there's not much reason to use clojure.jdbc aside from legacy purposes. They can both work together in the same project as well, so I think migratus should still work fine in projects that use clojure.jdbc once it's using next.jdbc.

I don't think anything in the external API migratus exposes is dependent on how clojure.jdbc API works, so from user perspective there shouldn't be any breaking change either.