Clojure-Contrib

Allow for externally-provided db connection to c.c.sql

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

This small patch allows an :external key in a db-spec map, to allow the use of existing database connection.

Among other benefits, this would make it trivial to use a Proxool db connection-pool with c.c.sql.

(Suggestions for alternative key-names are welcome. Maybe :instance or :connection-object would be more clear?)

Activity

Hide
Assembla Importer added a comment -

scgilardi said: Hi Graham,

I recall now why I didn't have this as an option already. with-connection retrieves an open connection based on the info in db-spec, allows body to use it, then closes it. If we have the connection object in the db-spec object itself, it will only be good for one use.

How about this as an alternative to your patch (in addition to the other methods that will remain): Allow the user to provide a function (:factory) whose argument is a map of the remaining keys and values in the db-spec object. To retrieve a new, open connection, with-connection would call the factory object supplying a map of its params.

I think this will provide enough of an escape hatch for any source of open database connections to be used while still preserving db-spec as an immutable object that specifies how to retrieve them.

Show
Assembla Importer added a comment - scgilardi said: Hi Graham, I recall now why I didn't have this as an option already. with-connection retrieves an open connection based on the info in db-spec, allows body to use it, then closes it. If we have the connection object in the db-spec object itself, it will only be good for one use. How about this as an alternative to your patch (in addition to the other methods that will remain): Allow the user to provide a function (:factory) whose argument is a map of the remaining keys and values in the db-spec object. To retrieve a new, open connection, with-connection would call the factory object supplying a map of its params. I think this will provide enough of an escape hatch for any source of open database connections to be used while still preserving db-spec as an immutable object that specifies how to retrieve them.
Hide
Assembla Importer added a comment -

scgilardi said: [file:d4S8n-6m8r3PaPeJe5aVNr]

Show
Assembla Importer added a comment - scgilardi said: [file:d4S8n-6m8r3PaPeJe5aVNr]
Hide
Assembla Importer added a comment -

gmfawcett said: Hi Steve,

I had considered the :factory approach as an option, and would be fine with it – certainly it's a better general solution.

In my specific case, though, auto-closing is a good thing: Proxool connections are proxy-objects, and closing one signals that the underlying connection can be returned to the connection pool.

I think you're proposing adding logic to (with-connection*), to not use (with-open) if the :factory key is used? If so, perhaps we could also add a optional :factory-close key, whose value is either

  • a cleanup function, e.g. {{{:factory-close (fn [conn] (.close conn))}}}
  • a boolean, e.g. {:factory-close true} implies that (with-connection*) should use (with-open) to close the factory-supplied connection?

I'm happy to provide an alternate patch once we sort out the details. (I know it's only a few minutes of typing; no offense will be taken if you code something yourself and ignore my patch.)

Show
Assembla Importer added a comment - gmfawcett said: Hi Steve, I had considered the :factory approach as an option, and would be fine with it – certainly it's a better general solution. In my specific case, though, auto-closing is a good thing: Proxool connections are proxy-objects, and closing one signals that the underlying connection can be returned to the connection pool. I think you're proposing adding logic to (with-connection*), to not use (with-open) if the :factory key is used? If so, perhaps we could also add a optional :factory-close key, whose value is either
  • a cleanup function, e.g. {{{:factory-close (fn [conn] (.close conn))}}}
  • a boolean, e.g. {:factory-close true} implies that (with-connection*) should use (with-open) to close the factory-supplied connection?
I'm happy to provide an alternate patch once we sort out the details. (I know it's only a few minutes of typing; no offense will be taken if you code something yourself and ignore my patch.)
Hide
Assembla Importer added a comment -

gmfawcett said: Just to clarify my last comment: I understand your comment about not wanting a possibly-closed connection object as a value in the map. In my test code, the :external worked just fine – in spite of the "closed" object in the map, I could re-use it without any trouble. Proxool must be doing some magic to make this work. But this is bad form.

+1 for your :factory patch, and I'm still in favour of :factory-close if you would accept a patch for (with-connection*) as well.

Show
Assembla Importer added a comment - gmfawcett said: Just to clarify my last comment: I understand your comment about not wanting a possibly-closed connection object as a value in the map. In my test code, the :external worked just fine – in spite of the "closed" object in the map, I could re-use it without any trouble. Proxool must be doing some magic to make this work. But this is bad form. +1 for your :factory patch, and I'm still in favour of :factory-close if you would accept a patch for (with-connection*) as well.
Hide
Assembla Importer added a comment -

scgilardi said: I like the uniformity of using with-open on all the connections. I'd like to save :factory-close until we have a usecase where it's needed. Could you please test the :factory patch in your setup? If you find that it works well for you, I'd like to commit it. Thanks!

Show
Assembla Importer added a comment - scgilardi said: I like the uniformity of using with-open on all the connections. I'd like to save :factory-close until we have a usecase where it's needed. Could you please test the :factory patch in your setup? If you find that it works well for you, I'd like to commit it. Thanks!
Hide
Assembla Importer added a comment -

gmfawcett said: I've tested your patch, and :factory works just fine for me.

I withdraw the :factory-close suggestion for now, since I don't need it. (I had suggested it based on a false assumption that you were patching with-connection* as well.)

Thank you!

Show
Assembla Importer added a comment - gmfawcett said: I've tested your patch, and :factory works just fine for me. I withdraw the :factory-close suggestion for now, since I don't need it. (I had suggested it based on a false assumption that you were patching with-connection* as well.) Thank you!
Hide
Assembla Importer added a comment -

importer said: (In revision:be33acd87f190d9ec2ad756d8cb31c88abca7e5f) Add Factory as a method of obtaining open database connections, fixes #50

Branch: master

Show
Assembla Importer added a comment - importer said: (In revision:be33acd87f190d9ec2ad756d8cb31c88abca7e5f) Add Factory as a method of obtaining open database connections, fixes #50 Branch: master

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: