korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.47k stars 222 forks source link

Generate a connection spec from a database URI (jdbc as well as native formats) for the databases that are client server (not h2, not sqlite). #316

Closed pupeno closed 9 years ago

pupeno commented 9 years ago

We were recently integrating Korma into Luminus and the fact that Korma could not just accept database URIs to connect was problematic, so here I have an initial implementation of it. It can parse both jdbc as well as the native URLs some databases uses, like the one Heroku gives you for PostgreSQL.

I played a bit trying to find the best way to implement this because I'm deconstructing the uri only for your code to reconstruct it later, which seems like a waste. But in the middle you add some sane defaults, like back-ticks for quoting MySQL identifies. I don't think I could have added a way to not deconstruct-reconstruct the URI without a lot of modification to the current code, which was not my goal.

When searching for a solution to this problem I actually found lot's of people running into this problem with either no solutions or hacky ones, which prompted me to put the time to do what I see as a proper fix. What do you think? Is this something you might want to merge? Let me know if you require changes for it.

pupeno commented 9 years ago

This pull request would be needed for this one to actually work: https://github.com/luminus-framework/luminus-template/pull/137

immoh commented 9 years ago

The feature itself is very much welcomed but I think the implementation needs some additional work.

As you already pointed out, deconstructing and reconstrucing the uri is not optimal and more of a hack than a long-term solution. My understanding is that we would only need to parse the subprotocol (scheme) from the uri to set the correct delimiters and such, otherwise it can be passed to connection pool / clojure.java.jdbc as it is.

Also, resolving parsed subprotocol to helper function is something I don’t really like. I think it can be easily avoided with bit of refactoring (maybe intodruce hashmap of subprotocol to configuration options)

From API perspective this should probably look something like:

(defdb mydb {:uri "jdbc:postgresql://localhost/db_name?user=user_name&password=password”})

where uri would take precedence over other keys.

So yeah, it requires more modifications than adding a function but I think it’s totally doable. If you want to give it a go, I’ll be happy answer further questions should you have any.

pupeno commented 9 years ago

That was actually my initial attempt at the API for this feature, but I felt I was modifying too much of the current codebase and I wanted to provide a patch that modified things as little as possible. I prefer this API, so, I'll give it a go and let you know how it goes.

Are you in http://clojurians.net/ ? I found just chatting over there when making changes such as this more efficient that email/github messages.

pupeno commented 9 years ago

I will most likely just close this one and generate other pull requests, like this one: https://github.com/korma/Korma/pull/317

immoh commented 9 years ago

I'm afraid email/github is the best I can do at the moment.

pupeno commented 9 years ago

This approach is abandoned, so, let's kill it.