active_record_doctor icon indicating copy to clipboard operation
active_record_doctor copied to clipboard

Error when listing columns for model backed by different DB

Open owst opened this issue 2 years ago • 10 comments

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!

owst avatar Aug 25 '23 13:08 owst

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.

fatkodima avatar Oct 08 '23 19:10 fatkodima

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?

gregnavis avatar Oct 14 '23 08:10 gregnavis

Sure thing @gregnavis, that sounds good - thanks!

owst avatar Oct 16 '23 08:10 owst

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

brunoarueira avatar Nov 04 '23 19:11 brunoarueira

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?

owst avatar Feb 28 '24 15:02 owst

@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.

gregnavis avatar Mar 02 '24 00:03 gregnavis

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

owst avatar Aug 22 '24 09:08 owst