Skip models connected to non-primary databases#237
Open
tmaier wants to merge 3 commits into
Open
Conversation
Pin minitest to ~> 5.0 in the Gemfile because minitest 6.0 removed Minitest.run_one_method which minitest-fork_executor depends on. Without the pin, bundle resolves to minitest 6.0.2 and the test suite crashes on startup. Make adapter requires in test/setup.rb graceful so the test suite can run when only one database adapter gem is installed (e.g. sqlite3 only). In CI all adapters are present, so this is a no-op there. Also add SecondaryContext (TransientRecord context for the existing SecondaryRecord class) to enable writing tests that exercise multi-database scenarios.
When a model uses establish_connection or connects_to to point at a different database, detectors should silently skip it. Previously they would crash (e.g. PG::UndefinedTable) because the detector queried the primary connection for a table that only exists in the secondary database. Add two test cases using SecondaryContext: - missing_non_null_constraint: a secondary-DB model with a nullable column and a presence validator must not crash the detector. - undefined_table_references: a secondary-DB model must not be reported as referencing an undefined table.
All detectors use Base#models to discover ActiveRecord models, then inspect their tables via Base#connection which is hardcoded to ActiveRecord::Base.connection (the primary database). When a model uses establish_connection or connects_to for a different database, model.table_exists? returns true (checked against its own connection), but connection.columns(table) crashes because the primary database has no such table. Fix by filtering Base#models to exclude models whose connection_pool differs from ActiveRecord::Base.connection_pool. This is a single guard that protects all detectors at once — no per-detector changes are needed. Document the limitation in the README under "Multiple Databases".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #149 —
active_record_doctorcrashes when the application has modelsconnected to a different database (e.g. a gem using
establish_connectionto SQLite while the app uses PostgreSQL).
Root cause:
Base#connectionis hardcoded toActiveRecord::Base.connection(the primary database). When
missing_non_null_constraintcallsmodels.select(&:table_exists?), it uses each model's own connection — so asecondary-DB model returns
true. Thenconnection.columns(table)queries theprimary connection for that table, which doesn't exist there → crash.
Fix: Filter
Base#modelsto exclude models whoseconnection_pooldiffersfrom
ActiveRecord::Base.connection_pool. This is a single guard that protectsall detectors — no per-detector changes needed.
Changes
lib/active_record_doctor/detectors/base.rb— Filtermodelsby connection pooltest/setup.rb— AddSecondaryContextfor multi-DB tests; graceful adapter loadingtest/.../missing_non_null_constraint_test.rb— Test: secondary-DB model doesn't crashtest/.../undefined_table_references_test.rb— Test: secondary-DB model isn't flaggedREADME.md— Document "Multiple Databases" behaviorGemfile— Pin minitest ~> 5.0 (minitest 6 broke minitest-fork_executor)Test plan
Disclaimer: This PR was created with the help of AI