Use a more specific rescue to avoid hiding unexpected errors

[Fryguy]

Follow up to #21037

If this code were in place prior to the Rails 6 upgrade, then we would have found the accidental override at that time.

To test, we can do the following:

$ bin/rake db:drop
$ bin/rails c # should not blow up

$ bin/rake db:create
$ bin/rails c
> Settings.log.level # => "info"

$ bin/rake db:migrate
$ bin/rails c
> Settings.log.level # => "info"

$ bin/rake db:seed
$ bin/rails c
> Settings.log.level # => "info"
> Vmdb::Settings.save!(MiqServer.my_server, :log => {:level => "debug"})
> Settings.reload!
> Settings.log.level # => "debug"

$ bin/rails c
> Settings.log.level # => "debug"

@NickLaMuro @agrare Please review.

[NickLaMuro]

My one suggestion for an improvement.

[NickLaMuro]

I think the other case we want to do is this (as I was working on the same thing 😉 ):

def database_connectivity?
  ActiveRecord::Base.connection && ActiveRecord::Base.connected?
rescue ActiveRecord::NoDatabaseError
  false
rescue => e
  return false if e.message =~ /^Could not load database configuration\./
end

From here:

https://github.com/rails/rails/blob/fe76a95b0d252a2d7c25e69498b720c96b243ea2/railties/lib/rails/application/configuration.rb#L241

(and there is no specified error... so you have to match on message...)

[Fryguy]

Yeah, I can add that. Only problem is how to verify it? Did you run a specific rake task? (rake environment blows up otherwise) When I delete config/database.yml it blows up before it reaches this code.

[NickLaMuro]

@Fryguy I did a mv config/database.yml config/database.yml.bak and then tested it by just doing:

$ bin/rails r "foo = 1"

That said, the regexp could probably just change to:

/^(?:Could |Can)not load database configuration(?:\.|:)/

Which should cover both errors.

[NickLaMuro]

@Fryguy Oh crap... in my code up above, I forgot to add another raise right after the return false, so it would re-raise if it doesn't catch the specific error.

My bad, it is what I had locally, but didn't put it here.

Full code then should be:

def database_connectivity?
  ActiveRecord::Base.connection && ActiveRecord::Base.connected?
rescue ActiveRecord::NoDatabaseError
  false
rescue => e
  return false if e.message =~ /^Could not load database configuration\./
  raise
end

[Fryguy]

But I can't even get to that point...it blows up before this code is even hit. In fact, the code doesn't even get to initializing the settings as it blows up way before this line, which is where the Settings are loaded first.

manageiq/config/application.rb

Line 161 in 0113603

Vmdb::Settings.init

[NickLaMuro]

Okay, so the other way I was tested was doing:

$ mv config/database.yml ...
$ bundle exec rake environment

That is how I got the "Could not load database configuration..." error instead.

[NickLaMuro]

Was also testing with

$ bundle exec rake test:verify_no_db_access_loading_rails_environment

Since the .with_dummy_database_url_configuration is used for stuff like building UI assets.

[Fryguy]

I'll give those a shot...thanks

[Fryguy]

rake test:verify_no_db_access_loading_rails_environment passes with this PR as is (without your addition). rake environment fails with a missing config/database.yml, but not on the code in question (it fails much earlier).

I still can't exercise the code in question. For example, if instead I put

      rescue => err
        raise "XXX HI - #{err}"

I can't seem to find a way to get the XXX error.

[NickLaMuro]

So talking with @Fryguy in DMs, I think this is pretty hard to reach in this class, so adding the extra rescue is probably not necessary or a use case we can currently hit. That said, I also can't see it hurting anything either, since if code surrounding it would ever change to allow the lacking of a config/database.yml to be missing, then this wouldn't half execution (which seems like a valid use case for this class in particular).

That said, it isn't a hill I am willing to die on, so I am going to approve this PR, but leave this "unresolved" just so the comments for this are more prevalent for those looking back.

[miq-bot]

Checked commit Fryguy@76731ae with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[Fryguy]

I think the Travis failure is a sporadic test failure and is unrelated. That spec passes locally. I'm doing some more tests locally the see if I can recreate.