Use notification to ensure that lazy-loaded model classes have transactions#20818
Conversation
|
I'll dig into the travis issues. I'm not sure the cause because they are unrelated to anything I think I changed. The postgres failure appears to have hung the test suite. I ran into a problem with a stack overflow on Mocha trying to set an expectation and this might be related to that. When I switched to mocha 1.1.0 I didn't have that problem, but that might affect lots of other things so I thought that should be a separate commit / PR. (BTW. Mocha 0.14.0 is now over two years old.) |
b8c669a to
109ba6d
Compare
|
I think I have these fixed up. We'll see what Travis decides. I kept running into YAML issues but I have them on master too (I rebased to make sure), so it might be something in my rails-dev-box (although I checked that was up-to-date). If this passes Travis, I can squash commits if you want. I left them separate to document the changes in the PR. |
|
Any thoughts as to how to get Travis to pick this up again? Travis says it doesn't run CI for branches that can't be merged without conflict, but GitHub says this branch is "up-to-date with the base branch" and I tested the merge on my dev box and there's no conflict for me. I'm not familiar with Travis so I don't know if there's something else I need to do to get it's attention. |
There was a problem hiding this comment.
We prefer to avoid using try inside of the rails codebase.
|
One minor nitpick. Please squash and rebase as well. |
|
^^ Please squash and rebase. |
109ba6d to
b832a60
Compare
|
@maclover7: Done, thanks for the nudge. Tried to do that in October but there were tons of conflicts I didn't understand. Those seem to have magically disappeared so this was easy (once I destroyed and rebuilt my rails-dev-box to get the latest Bundler and ruby). |
There was a problem hiding this comment.
While I think this use case is valid, I don't think we need to add this to the public API.
There was a problem hiding this comment.
Ok. I removed the docs.
b832a60 to
20dc67e
Compare
There was a problem hiding this comment.
I dont like rescue nil :/ , we should be explicit about error handling .
There was a problem hiding this comment.
@arthurnn Fixed and removed the assignment-in-conditional which I think is also discouraged in Rails style guide.
20dc67e to
66d3852
Compare
|
r? @arthurnn it is related with your work with connections, could you take a look? @jeremywadsack this is a clever solution and it is fine to me. Could you rebase it? I promise you it will be the last time. |
There was a problem hiding this comment.
Can we add a ! prefix in the instrumentation name to say people that it should not be used in production?
message_bus.instrument('!connection.active_record', payload) do66d3852 to
7237cc3
Compare
|
@rafaelfranca Added the |
…hat lazy- loaded model classes have their connections wrapped in transactions. See rails#17776 In Rails 4 config.eager_load was changed to false in the test environment. This means that model classes that connect to alternate databases with establish_connection are not loaded at start up. If use_transactional_fixtures is enabled, transactions are wrapped around the connections that have been established only at the start of the test suite. So model classes loaded later don't have transactions causing data created in the alternate database not to be removed. This change resolves that by creating a new connection.active_record notification that gets fired whenever a connection is established. I then added a subscriber after we set up transactions in the test environment to listen for additional connections and wrap those in transactions as well.
7237cc3 to
31a8588
Compare
Follow up to rails#20818. `retrieve_connection` is passed `spec_name` instead of `klass` since rails#24844.
|
FYI 88cdecc |
|
@arthurnn Thanks for fixing that extra connection. I wasn't sure if the delay caused by the instrumentation would be a problem. Note then, that the method now returns |
Related rails#36456. I grepped the code base by `git grep -n 'connection_id: '` then I found extra `connection_id: object_id` which is added at rails#20818 but unused. Actually the `connection_id: object_id` is not a connection's object_id but a connection_handler's object_id, it is very confusing. Since the `:connection_id` in an internal instrument is not used, we can just remove the incorrect information.
See #17776
In Rails 4
config.eager_loadwas changed tofalsein the test environment. Thismeans that model classes that connect to alternate databases with
establish_connectionare not loaded at start up. Ifuse_transactional_fixturesis enabled for tests, transactions are wrapped around the connections that have been
established only at the start of the test suite. So model classes loaded later
don't have transactions, causing data created in the alternate database not to
be removed.
This change resolves that by creating a new
!connection.active_recordnotification that gets fired whenever a connection is established. I then added
a subscriber after we set up transactions in the test environment to listen for
additional connections and wrap those in transactions as well.