search

gocardless / statesman

A statesmanlike state machine library.
https://gocardless.com/blog/statesman/
MIT License
1.78k stars 163 forks source link

Support Rails 6.0 Multi DB #407

Closed ckorhonen closed 10 months ago

ckorhonen commented 4 years ago

This PR addresses the issue described in #406 - since Rails now supports multiple databases, we cannot assume that the transition class uses the connection as defined by ActiveRecord::Base.

thom-oman commented 4 years ago

Hey @ckorhonen thanks for the PR. This looks pretty reasonable, happy to approve if this is ready. Think we may also need to change this line as well.

ckorhonen commented 4 years ago

@thom-oman Good spot, I've updated that line.

The other thing I wasn't too sure about were the class methods here. Realistically I doubt the Rails 3 crutch is still needed, but it is possible that the different databases could use different adapters.

Also not sure if I should be doing anything additional in terms of tests for this specific case?

This has solved the issue in my own application - I'm using multiple state machines, with the transition classes residing in both the primary and secondary databases, no regressions encountered.

thom-oman commented 4 years ago

Yeah IIRC we dropped support for Rails 3 earlier this year, don't have much context on that check but my gut tells me we should make the changes there too and we can remove Rails 3 support independently. The other method was added recently, is probably worth changing that too, but would only cause issues if the transitions table uses a different database (like MySQL instead of PG).

Regarding tests, yes I almost mentioned test, I struggled to think what the tests could look like such that they actually provide value. All scenarios I could think of would, in essence, testing Rails. Will give it me more thought and speak to some of my colleagues. Also will check our codebase for regressions.

ckorhonen commented 4 years ago

Pushed some additional updates - as I was building out a state machine I noticed that after_commit wasn't working for anything using the non-primary db.

thom-oman commented 4 years ago

Awesome. I think we should for sure be adding some tests for this then, so we can confirm that ActiveRecord is querying the correct database and that would've failed without the after_commit change.

thom-oman commented 4 years ago

@ckorhonen any updates?

robotex82 commented 1 year ago

Hi, thank you all for the effort! Any chances to have this merged?

stephenbinns commented 10 months ago

Superseded by https://github.com/gocardless/statesman/pull/522