gregnavis / active_record_doctor

Identify database issues before they hit production.
MIT License
1.76k stars 55 forks source link

Error when listing columns for model backed by different DB #149

Open owst opened 1 year ago

owst commented 1 year ago

We have a couple of models that are backed by a different DB to the majority of models in our app. A rough example is something like this:

class MainDBThings < ApplicationRecord
end
class ExternalDBThings < ApplicationRecord
  establish_connection :external_db
end

It seems that a couple of places in active_record_doctor assume that all tables can be accessed from a connection to a single DB (ActiveRecord::Base.connection); when the "different DB" table names are accessed, a "table missing" error is raised because the connection used is to the wrong DB.

For example, this line:

https://github.com/gregnavis/active_record_doctor/blob/f3b8ba515e400b9c36e1d117ed84f91eecfd78ad/lib/active_record_doctor/detectors/missing_non_null_constraint.rb#L35

uses connection, which raises an error along the lines of:

 ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR:  relation "ExternalDBThings" does not exist

A possible fix might be to use models.first.columns instead of connection.columns(table) so that the per-model connection is used?

Similarly, this line has the same issue: https://github.com/gregnavis/active_record_doctor/blob/f3b8ba515e400b9c36e1d117ed84f91eecfd78ad/lib/active_record_doctor/detectors/base.rb#L218

I'm happy to have a stab at a PR for this, but would like some guidance as to any thoughts or suggested approaches before I start. I expect the fix will be to not use connection without reference to a particular model instance.

Thanks!

fatkodima commented 1 year ago

Suggested changes will solve this issue, but this gem still do not have proper support for multiple databases, see https://github.com/gregnavis/active_record_doctor/issues/63.

Please, do open a PR.

gregnavis commented 1 year ago

Thanks for opening the issue, @owst! I'm working hard on adding support for multi-database systems, so expect an update soon. Would you be willing to test a release candidate?

owst commented 1 year ago

Sure thing @gregnavis, that sounds good - thanks!

brunoarueira commented 1 year ago

@gregnavis I would like to test a release candidate too, call me in! Btw, thanks for this gem 😃

owst commented 8 months ago

Hi @gregnavis I just came across this issue again preventing us from upgrading active_record_doctor, have you managed to make any progress on multi-DB support and do you maybe have a release candidate to test?

gregnavis commented 8 months ago

@owst, actually, yes, I've made some progress, but it's not done yet. I released transient_record 2.0 that supports multiple databases. That was a prerequisite for adding multi-database test cases to active_record_doctor.

owst commented 2 months ago

Hi @gregnavis - just following up on this again - any further progress since your last comment?

gregnavis commented 2 months ago

@owst, some progress, but need to put more time into that. I'm preparing 1.15 right now and created a milestone for 2.0. Multi-database support should land in 2.0.