search

yogthos / migratus

MIGRATE ALL THE THINGS!
641 stars 95 forks source link

Allow calling code to manage connection disconnects #256

Closed b-ryan closed 8 months ago

b-ryan commented 8 months ago

Resolves #255

The current implementation in this branch is not exactly what was discussed in #255. I got more familiar with the code and wanted to put something out there and discuss further.

The change in this branch appears to be the one that requires the fewest changes. The Database record now does not call disconnect* if :managed-connection? is set on the :db config.

Alternative that was discussed in #255 was to create a new implementation of Store. This would require:

  1. Adding a new method for proto/make-store that I guess would check for the :store to be :managed-database, example config: 
    {:store :managed-database                                   
:properties {:env ["database.table"]               
            :map {:database {:user "bob"}}}       
:db {:connection ...}}

    (I haven't looked through all of the code to see what else might need to change to handle a new :store key.)

  2. Most of the functionality inside the Database record would need to factored out into standalone functions that both Database and the new ManagedDatabase could call.

A second alternative that I thought of would be to introduce either a protocol or a multimethod that is used by the disconnect* function (and perhaps for consistency, connect* as well).

I'm currently leaning towards doing this, since it will mean the disconnect* function will be safe to use in the future, without the calling code needing to worry about whether the disconnect should really happen.

yogthos commented 8 months ago

@ieugen I recall we chatted about adding this a little while back, so just looping you in on this

yogthos commented 8 months ago

hmm does look like we're seeing some test failures with the new changes

b-ryan commented 8 months ago

Hey sorry - I should have marked this as WIP. Didn't do much testing, as I mostly wanted to confirm the direction. I can take a look at failures now and maybe introduce additional tests.

yogthos commented 8 months ago

Ah no worries, just ping me when you have a chance to take a look at that.