Functions that generate database specifications generate connection-uri.

[pupeno]

To be able to accept connection-uri from the user, we need to have the database specification function generate the connection-uri so that when they get one, they don't generate it and just pass it. This is the only way I found to implement accepting a connection-uri that doesn't require disassembling it to later on assembling it again.

Is this change acceptable? I'd like to settle this one before moving on.

[ls4f]

Why not just do something like (.setJdbcUrl (or connection-uri (str "jdbc:" subprotocol ":" subname))) and avoid changing all the helpers? Seems a bit DRYer to me.

[pupeno]

@ls4f I would still need to change all the helpers, as calling this:

(mysql {:connection-uri "jdbc:mysql://some_host:1234/some_db?user=some_user&psasword=some_password"})

would produce this:

{:classname "com.mysql.jdbc.Driver", 
 :subprotocol "mysql", 
 :subname "//localhost:3306/", 
 :delimiters "`", 
 :make-pool? true, 
 :connection-uri "jdbc:mysql://some_host:1234/some_db?user=some_user&psasword=some_password"}

which has the wrong subname in it. It might still be more DRY, but it means functions as mysql or postgresql have two different mutually exclusive outputs, which increases complexity a little bit.

[ls4f]

My point is to simply ignore :subname/:db/:host/:port/so-on whenever :connection-uri is -present. Are those things needed anywhere besides .setJdbcUrl, which you can simply assign to the connection-uri? Because if they aren't .. having a subname of "L1$P kills" won't really matter, will it?

[pupeno]

I'm not 100% sure if they are used somewhere else or not. It seems the subprotocol might be used here: https://github.com/korma/Korma/blob/master/src/korma/sql/fns.clj#L42 but my understanding of Korma is not good enough yet.

The result of calling functions such as mysql or postgresql is not an internal detail and it might be leaked in some cases (logging, someone inspecting what's going on, indeed, one of the first things we did when we were experimenting with Korma is look at the output of those functions because some of the documentation was not using them). Having the conflicting entries might be confusing to users of Korma as well as future developers.

[ls4f]

You are right (in general). I do think if someone knows what a jdbc uri is and prefers constructing it some other way, they won't really have a problem with it ... As for subprotocol, I see no problem with the helper setting it and korma.db ignoring it, but that's just me. Don't want to sound like a rant:) I do think having the option to override stuff (by hand if need be) is a nice idea. I just have a thing for short code, even if it's not 101% pretty in the parts that don't do harm.

[pupeno]

@ls4f the only thing I really feel uncomfortable with is having conflicting hashmaps, like:

{:subprotocol "mysql"
 :connection-uri "jdbc:postgresql://..."}

I think that will be a factory of bugs and misunderstandings. I can have the the helpers spit out either connection-uri or subprotocol/subname/etc. To me it feels like building a connection-uri is broken up in two separate places when it's easier to understand if it's build in once, but I don't think that's a big deal. This would still require changing all helpers.

Who is the current maintainer of Korma? @ls4f? @ibdknox? @immoh?

[ls4f]

Well I'm not a maintainer:) Just sharing views. To be honest, I'd leave the helpers as is and introduce from-uri helper, for "advanced" cases. That way you can't end up with such a hashmap (at least not Korma constructed). It will also mean the map has either a URI or details (host/port/path/moon phase) but not both. And you get to build the URI just once, if not provided, when setting it. The way the patch is, you have a lot of places building the uri ... which is code duplication. Of course, I've been wrong before.

[pupeno]

Even if I have a from-uri helper, which we might, I need to call the other helpers because there's a lot of logic in those, like using ` as delimiters. I suppose from-uri could call the appropriate helper function and then strip out the offending members of the map, but that will require parsing the URI, which both @immoh and I agree is not such a good idea.

My first attempt at implementing this was going in that direction: https://github.com/korma/Korma/pull/316

[ls4f]

I'm trying to say from-uri shouldn't touch the helpers and the user should provide any needed stuff. Make it sort of bypass existing Korma logic and just allow the user roam free. Or it could accept a keyword for the db type so the driver/delimiters/stuff can remain hard coded, but that seems to be defeating the point. Mixing the two approaches just seems like asking for trouble to me. For the record - parsing the URI sounds like a bad idea to me too.

[pupeno]

I don't see the passing of the URI as an advanced method, Heroku, for example, pretty much forces you to use an URL. In Luminus, passing a URL is the default. Many environments work this way without them being advanced situations where you should be making decisions as to how to quote field names.

I would like to pass an URL and still get all the smarts that Korma has.

[ls4f]

Ok, here is my idea of minimal code changes

(defmacro from-uri
    [uri helper args]
  `(let [subname# (.substring ~uri (inc (.indexOf ~uri ":" 5)))]
     (assoc (~helper ~args) :subname subname#)))

usage goes like (korma.db/from-uri "jdbc:somesubprotocol:somedburi" (korma.db/mysql {})) Korma code assumes the jdbc:subprotocol:subname format of the URL anyway, so I'm not making new assumptions. It looks a bit weird but I think it gets away with no other changes in the code.

PS I already have code assigning subname to the Korma helper ... it works:)

[immoh]

In my opinion having both connection uri and subprotol/subname in the spec is just asking for trouble and should be avoided if possible. Not sure whether we should always construct uri in helper functions or later when calling .setJdbcUrl.Both are fine for me. It’s important that it still works when setting make-pool? the false ie. when when passing db spec directly to clojure.java.jdbc.

Subprotocol is options is used for making aggregate count queries work in MySQL. I don’t know the history, maybe there’s a better way to implement it but I think we could just store mysql? instead of the whole uri. What do you think?

I am one of the maintainers of Korma and pretty much the only one who has been active during last two years or so.

[pupeno]

@immoh I think we need to keep the uri in there for when make-pool? is false. I'll take a look. Otherwise, do you prefer to store :mysql? true or :protocol :mysql ? Ultimately, the URI is the canonical information.

[immoh]

I think it works without connection pool as it is: https://github.com/clojure/java.jdbc/blob/master/src/main/clojure/clojure/java/jdbc.clj#L216

Perhaps :protocol :mysql is better as it requires fewer changes. (That information shouldn't be there anyway, I will take a look at it later if there's a way to get rid of it altogether.)

[pupeno]

@immoh getting rid of what information, :protocol? If that's what you want, I already modified the mysql functions to inspect the connection-uri to know whether it's mysql or not, I think it leads to very succinct data.

[immoh]

You have replaced it with connection uri. That's fine for now, let's not get stuck with this, I'll merge this so that we can continue with the original topic.