Fix for rails 6.1

[sj26]

Rails 6.1 now returns an instance of NullPool instead of nil in response to Connection#pool at times, and NullPool does not have #spec so this method blows up. Instead of testing for a nil pool, test if the pool responds to #spec which should cope with both nil and NullPool instances with full backwards compatibility.

[sj26]

Hi folks, rails 6 is out — have you had a chance to look at this? I don't know why the tests aren't running, looks like the suite might be broken.

[sj26]

Ah, I see the test suite is fixed in #90.

[jeremy]

Thanks @sj26! Have you tried this out on older Rails as well? Wary of breaking compat.

[jeremy]

Does this work on older Rails as well?

[sj26]

Sorry for the delay. I haven't tried this on older rails myself, but as mentioned this should retain backwards compatiblity — nil also doesn't respond to spec. If you merge #90 first then I can rebase on there to prove that it works. :-)

[sj26]

(The test suite does pass on my local machine when run against all available gemfiles.)

[sj26]

If anybody needs a workaround for this at the moment, we're using the following:

# config/initializers/marginalia_null_pool_patch.rb

# Marginalia guards against if a connection has a `nil` pool, but Rails 6 now
# returns a `NullPool` instance which doesn't respond to `#spec` and so blows
# up. This patch looks for the `#spec` method instead which covers both cases,
# and has been pushed upstream:
#
# https://github.com/basecamp/marginalia/pull/91

if Gem.loaded_specs["marginalia"].version > Gem::Version.new("1.8.0")
  raise "Remove the patch if the upstream pull request has been merged"
end

Marginalia::Comment.module_eval do
  def self.connection_config
    return unless marginalia_adapter.pool.respond_to?(:spec)
    marginalia_adapter.pool.spec.config
  end
end

[fidalgo]

Just a bit of warning for those who land here that you still need this #91 (comment) as of version 1.9.0 of Marginalia

[kylesnowschwartz]

Just a bit of warning for those who land here that you still need this #91 (comment) as of version 1.9.0 of Marginalia

Hi @fidalgo thanks for this PR and for helping making Marganalia compatible with Rails 6. I'm currently doing an analysis of this gem as we undergo an upgrade from 5.2 to Rails 6, and came across this PR. For my understanding, could you explain how you came to see this issue or how it could be reproduced? In my testing thus far, it seems like my application's usage of Marginalia#with_annotation works just as well in the latest version of rails without any patching necessary.

My understanding of the original Marganlia PR #62 is that this was mainly added for their own internal testing, to make version changing easier. Perhaps I misunderstand as my knowledge is limited. It seems like connection_pools are back in Rails 6 (I wonder where they went in Rails 3?) but like you pointed out, it's possible to get a NullPool object, but I fail to see how this could be returned given there is a active record MySQL adaptor that is active.

Any additional context you could provide would be much appreciated!

Edit: Following up on my own comment, I can trigger this error by activating the other additional comment components, i.e.

config/initializers/marginalia.rb

 Marginalia.application_name = 'my_application'
 Marginalia::Comment.components << :hostname
 Marginalia::Comment.components << :job
+Marginalia::Comment.components << :db_host

[sj26]

@jeremy would you like a hand with maintenance here? It'd be great to get the gem working with rails 6+ natively. I'd be happy to port the test suite to github actions and prove this pull request doesn't affect other rails versions.

[jeremy]

@sj26 Please do go ahead! Thanks for volunteering. 🙇🏼‍♂️

[sj26]

Thanks! Let me throw up a PR for the CI switch and let's go from there.

[sj26]

Neat, so we've got tests running across all supported rubies and rails. This branch adds support for rails 6.1 and adds it to the test coverage. Turns out compatibility required a little more than my initial patch, but not too much.

Ready to rumble?

[sj26]

Rebased on lastest master.

[sj26]

I tested master within our rails 6.1 application and it works great, so I've release this in v1.11.0.

[denis-haskin-elemental]

Just fyi, I think this fix incorrectly targets just ActiveRecord 6.1. The Rails breaking change was released in 6.0, as far as I can tell: rails/rails#36371

I can confirm, because I see this error in our app at the moment; we've just upgraded to 6.0.6.1.

Since we're expecting to go straight to 6.1 as part this upgrade project, I'm just going to remove marginalia from our project until we get to 6.1.