diff --git a/lib/active_record_doctor.rb b/lib/active_record_doctor.rb index 8a7b4b26..38f85886 100644 --- a/lib/active_record_doctor.rb +++ b/lib/active_record_doctor.rb @@ -25,6 +25,7 @@ require "active_record_doctor/errors" require "active_record_doctor/help" require "active_record_doctor/runner" +require "active_record_doctor/caching_schema_inspector" require "active_record_doctor/version" require "active_record_doctor/config" require "active_record_doctor/config/loader" diff --git a/lib/active_record_doctor/caching_schema_inspector.rb b/lib/active_record_doctor/caching_schema_inspector.rb new file mode 100644 index 00000000..409f1b06 --- /dev/null +++ b/lib/active_record_doctor/caching_schema_inspector.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module ActiveRecordDoctor + class CachingSchemaInspector # :nodoc: + def initialize(connection) + @connection = connection + @primary_keys = {} + @indexes = {} + @foreign_keys = {} + @check_constraints = {} + end + + def tables + @tables ||= @connection.tables + end + + def data_sources + @data_sources ||= @connection.data_sources + end + + def views + @views ||= @connection.views + end + + def primary_key(table_name) + @primary_keys[table_name] ||= @connection.primary_key(table_name) + end + + def columns(table_name) + @connection.schema_cache.columns(table_name) + end + + def indexes(table_name) + @indexes.fetch(table_name) do + @indexes[table_name] = @connection.indexes(table_name) + end + end + + def foreign_keys(table_name) + @foreign_keys[table_name] ||= @connection.foreign_keys(table_name) + end + + def check_constraints(table_name) + @check_constraints[table_name] ||= + if @connection.supports_check_constraints? + @connection.check_constraints(table_name).select(&:validated?).map(&:expression) + else + # We don't support this Rails/database combination yet. + [] + end + end + end +end diff --git a/lib/active_record_doctor/detectors/base.rb b/lib/active_record_doctor/detectors/base.rb index b02630ba..b3c576be 100644 --- a/lib/active_record_doctor/detectors/base.rb +++ b/lib/active_record_doctor/detectors/base.rb @@ -38,11 +38,12 @@ def locals_and_globals end end - def initialize(config:, logger:, io:) + def initialize(config:, logger:, io:, schema_inspector:) @problems = [] @config = config @logger = logger @io = io + @schema_inspector = schema_inspector end def run @@ -114,18 +115,31 @@ def connection end def indexes(table_name) - connection.indexes(table_name) + @schema_inspector.indexes(table_name) end - def primary_key(table_name) - primary_key_name = connection.primary_key(table_name) - return nil if primary_key_name.nil? + def tables + @schema_inspector.tables + end + + def data_sources + @schema_inspector.data_sources + end - column(table_name, primary_key_name) + def views + @schema_inspector.views + end + + def primary_key(table_name) + @schema_inspector.primary_key(table_name) end def column(table_name, column_name) - connection.columns(table_name).find { |column| column.name == column_name } + columns(table_name).find { |column| column.name == column_name } + end + + def columns(table_name) + @schema_inspector.columns(table_name) end def not_null_check_constraint_exists?(table, column) @@ -135,24 +149,20 @@ def not_null_check_constraint_exists?(table, column) end end + def table_exists?(table_name) + tables.include?(table_name) + end + + def data_source_exists?(name) + data_sources.include?(name) + end + def check_constraints(table_name) - if connection.supports_check_constraints? - connection.check_constraints(table_name).select(&:validated?).map(&:expression) - elsif Utils.postgresql?(connection) - definitions = - connection.select_values(<<-SQL) - SELECT pg_get_constraintdef(oid, true) - FROM pg_constraint - WHERE contype = 'c' - AND convalidated - AND conrelid = #{connection.quote(table_name)}::regclass - SQL - - definitions.map { |definition| definition[/CHECK \((.+)\)/m, 1] } - else - # We don't support this Rails/database combination yet. - [] - end + @schema_inspector.check_constraints(table_name) + end + + def foreign_keys(table_name) + @schema_inspector.foreign_keys(table_name) end def models @@ -175,7 +185,7 @@ def each_model(except: [], abstract: nil, existing_tables_only: false) log("#{model.name} - non-abstract model; skipping") when abstract == false && model.abstract_class? log("#{model.name} - abstract model; skipping") - when existing_tables_only && (model.table_name.nil? || !model.table_exists?) + when existing_tables_only && (model.table_name.nil? || !table_exists?(model.table_name)) log("#{model.name} - backed by a non-existent table #{model.table_name}; skipping") else log(model.name) do @@ -187,7 +197,7 @@ def each_model(except: [], abstract: nil, existing_tables_only: false) end def each_index(table_name, except: [], multicolumn_only: false) - indexes = connection.indexes(table_name) + indexes = indexes(table_name) message = if multicolumn_only @@ -214,7 +224,7 @@ def each_index(table_name, except: [], multicolumn_only: false) def each_attribute(model, except: [], type: nil) log("Iterating over attributes of #{model.name}") do - connection.columns(model.table_name).each do |column| + columns(model.table_name).each do |column| case when ignored?("#{model.name}.#{column.name}", except) log("#{model.name}.#{column.name} - ignored via the configuration; skipping") @@ -231,7 +241,7 @@ def each_attribute(model, except: [], type: nil) def each_column(table_name, only: nil, except: []) log("Iterating over columns of #{table_name}") do - connection.columns(table_name).each do |column| + columns(table_name).each do |column| case when ignored?("#{table_name}.#{column.name}", except) log("#{column.name} - ignored via the configuration; skipping") @@ -253,7 +263,7 @@ def looks_like_foreign_key?(column) def each_foreign_key(table_name) log("Iterating over foreign keys on #{table_name}") do - connection.foreign_keys(table_name).each do |foreign_key| + foreign_keys(table_name).each do |foreign_key| log("#{foreign_key.name} - #{foreign_key.from_table}(#{foreign_key.options[:column]}) to #{foreign_key.to_table}(#{foreign_key.options[:primary_key]})") do # rubocop:disable Layout/LineLength yield(foreign_key) end @@ -263,7 +273,7 @@ def each_foreign_key(table_name) def each_table(except: []) log("Iterating over tables") do - connection.tables.each do |table| + tables.each do |table| case when ignored?(table, except) log("#{table} - ignored via the configuration; skipping") @@ -278,7 +288,7 @@ def each_table(except: []) def each_data_source(except: []) log("Iterating over data sources") do - connection.data_sources.each do |data_source| + data_sources.each do |data_source| if ignored?(data_source, except) log("#{data_source} - ignored via the configuration; skipping") else diff --git a/lib/active_record_doctor/detectors/extraneous_indexes.rb b/lib/active_record_doctor/detectors/extraneous_indexes.rb index ea1574bd..89a4816c 100644 --- a/lib/active_record_doctor/detectors/extraneous_indexes.rb +++ b/lib/active_record_doctor/detectors/extraneous_indexes.rb @@ -61,7 +61,7 @@ def indexed_primary_keys log(__method__) do each_table(except: config(:ignore_tables)) do |table| each_index(table, except: config(:ignore_indexes), multicolumn_only: true) do |index| - primary_key = connection.primary_key(table) + primary_key = primary_key(table) if index.columns == [primary_key] && index.where.nil? problem!(table: table, extraneous_index: index.name, replacement_indexes: nil) end diff --git a/lib/active_record_doctor/detectors/incorrect_dependent_option.rb b/lib/active_record_doctor/detectors/incorrect_dependent_option.rb index fc580fb6..694a3e92 100644 --- a/lib/active_record_doctor/detectors/incorrect_dependent_option.rb +++ b/lib/active_record_doctor/detectors/incorrect_dependent_option.rb @@ -180,7 +180,7 @@ def dependent_models(model) end def foreign_key(from_table, to_table) - connection.foreign_keys(from_table).find do |foreign_key| + foreign_keys(from_table).find do |foreign_key| foreign_key.to_table == to_table end end diff --git a/lib/active_record_doctor/detectors/missing_foreign_keys.rb b/lib/active_record_doctor/detectors/missing_foreign_keys.rb index 18411a40..579fbbb4 100644 --- a/lib/active_record_doctor/detectors/missing_foreign_keys.rb +++ b/lib/active_record_doctor/detectors/missing_foreign_keys.rb @@ -24,7 +24,7 @@ def message(table:, column:) def detect each_model(except: config(:ignore_models), existing_tables_only: true) do |model| - foreign_keys = connection.foreign_keys(model.table_name) + foreign_keys = foreign_keys(model.table_name) foreign_key_columns = foreign_keys.map { |key| key.options[:column] } each_association(model, type: :belongs_to) do |association| diff --git a/lib/active_record_doctor/detectors/missing_non_null_constraint.rb b/lib/active_record_doctor/detectors/missing_non_null_constraint.rb index 63bfc4b0..8278e0c8 100644 --- a/lib/active_record_doctor/detectors/missing_non_null_constraint.rb +++ b/lib/active_record_doctor/detectors/missing_non_null_constraint.rb @@ -34,7 +34,6 @@ def message(column:, table:) def detect table_models = models.select(&:table_exists?).group_by(&:table_name) - views = connection.views table_models.each do |table, models| next if ignored?(table, config(:ignore_tables)) @@ -44,7 +43,7 @@ def detect model.abstract_class? || sti_base_model?(model) end - connection.columns(table).each do |column| + columns(table).each do |column| next if ignored?("#{table}.#{column.name}", config(:ignore_columns)) next if !column.null next if !concrete_models.all? { |model| non_null_needed?(model, column) } diff --git a/lib/active_record_doctor/detectors/missing_presence_validation.rb b/lib/active_record_doctor/detectors/missing_presence_validation.rb index ab1b9130..920533f8 100644 --- a/lib/active_record_doctor/detectors/missing_presence_validation.rb +++ b/lib/active_record_doctor/detectors/missing_presence_validation.rb @@ -36,7 +36,7 @@ def detect each_model(except: config(:ignore_models), existing_tables_only: true) do |model| # List all columns and then remove those that don't need or don't have # a missing validator. - problematic_columns = connection.columns(model.table_name) + problematic_columns = columns(model.table_name) problematic_columns.reject! do |column| # The primary key, timestamps, and counter caches are special # columns that are automatically managed by Rails and don't need diff --git a/lib/active_record_doctor/detectors/missing_unique_indexes.rb b/lib/active_record_doctor/detectors/missing_unique_indexes.rb index 34818239..612c080f 100644 --- a/lib/active_record_doctor/detectors/missing_unique_indexes.rb +++ b/lib/active_record_doctor/detectors/missing_unique_indexes.rb @@ -110,7 +110,7 @@ def has_ones_without_indexes # rubocop:disable Naming/PredicateName table_name = has_one.klass.table_name next if unique_index?(table_name, columns) - next if Array(connection.primary_key(table_name)) == columns + next if Array(primary_key(table_name)) == columns problem!(model: model, table: table_name, columns: columns, problem: :has_ones) end diff --git a/lib/active_record_doctor/detectors/short_primary_key_type.rb b/lib/active_record_doctor/detectors/short_primary_key_type.rb index a1a481b2..3ef8a66e 100644 --- a/lib/active_record_doctor/detectors/short_primary_key_type.rb +++ b/lib/active_record_doctor/detectors/short_primary_key_type.rb @@ -23,7 +23,7 @@ def detect return if ActiveRecordDoctor::Utils.sqlite? each_table(except: config(:ignore_tables)) do |table| - column = primary_key(table) + column = column(table, primary_key(table)) next if column.nil? next if !integer?(column) || bigint?(column) diff --git a/lib/active_record_doctor/detectors/undefined_table_references.rb b/lib/active_record_doctor/detectors/undefined_table_references.rb index 9529b2a4..fea8c8b0 100644 --- a/lib/active_record_doctor/detectors/undefined_table_references.rb +++ b/lib/active_record_doctor/detectors/undefined_table_references.rb @@ -21,7 +21,7 @@ def message(model:, table:) def detect each_model(except: config(:ignore_models), abstract: false) do |model| - next if connection.data_source_exists?(model.table_name) + next if data_source_exists?(model.table_name) problem!(model: model.name, table: model.table_name) end diff --git a/lib/active_record_doctor/detectors/unindexed_foreign_keys.rb b/lib/active_record_doctor/detectors/unindexed_foreign_keys.rb index daf60bb2..ec64be56 100644 --- a/lib/active_record_doctor/detectors/unindexed_foreign_keys.rb +++ b/lib/active_record_doctor/detectors/unindexed_foreign_keys.rb @@ -30,7 +30,7 @@ def detect next unless looks_like_foreign_key?(column) || foreign_key?(table, column) next if indexed?(table, column) next if indexed_as_polymorphic?(table, column) - next if connection.primary_key(table) == column.name + next if primary_key(table) == column.name type_column_name = type_column_name(column) @@ -47,25 +47,25 @@ def detect end def foreign_key?(table, column) - connection.foreign_keys(table).any? do |foreign_key| + foreign_keys(table).any? do |foreign_key| foreign_key.column == column.name end end def indexed?(table, column) - connection.indexes(table).any? do |index| + indexes(table).any? do |index| index.columns.first == column.name end end def indexed_as_polymorphic?(table, column) - connection.indexes(table).any? do |index| + indexes(table).any? do |index| index.columns[0, 2] == [type_column_name(column), column.name] end end def column_exists?(table, column_name) - connection.columns(table).any? { |column| column.name == column_name } + columns(table).any? { |column| column.name == column_name } end def type_column_name(column) diff --git a/lib/active_record_doctor/rake/task.rb b/lib/active_record_doctor/rake/task.rb index 43c2f690..249e4f5e 100644 --- a/lib/active_record_doctor/rake/task.rb +++ b/lib/active_record_doctor/rake/task.rb @@ -64,7 +64,11 @@ def define private def runner - @runner ||= ActiveRecordDoctor::Runner.new(config: config, logger: logger) + @runner ||= begin + connection = ActiveRecord::Base.connection + schema_inspector = ActiveRecordDoctor::CachingSchemaInspector.new(connection) + ActiveRecordDoctor::Runner.new(config: config, logger: logger, schema_inspector: schema_inspector) + end end def config diff --git a/lib/active_record_doctor/runner.rb b/lib/active_record_doctor/runner.rb index 86dc0d17..078bac75 100644 --- a/lib/active_record_doctor/runner.rb +++ b/lib/active_record_doctor/runner.rb @@ -5,9 +5,10 @@ module ActiveRecordDoctor # :nodoc: # and an output device for use by detectors. class Runner # io is injected via constructor parameters to facilitate testing. - def initialize(config:, logger:, io: $stdout) + def initialize(config:, logger:, schema_inspector:, io: $stdout) @config = config @logger = logger + @schema_inspector = schema_inspector @io = io end @@ -16,7 +17,8 @@ def run_one(name) ActiveRecordDoctor.detectors.fetch(name).run( config: config, logger: logger, - io: io + io: io, + schema_inspector: schema_inspector ) end end @@ -41,6 +43,6 @@ def help(name) private - attr_reader :config, :logger, :io + attr_reader :config, :logger, :io, :schema_inspector end end diff --git a/test/active_record_doctor/runner_test.rb b/test/active_record_doctor/runner_test.rb index 573f5137..5af67f1c 100644 --- a/test/active_record_doctor/runner_test.rb +++ b/test/active_record_doctor/runner_test.rb @@ -3,10 +3,14 @@ class ActiveRecordDoctor::RunnerTest < Minitest::Test def setup @io = StringIO.new + connection = ActiveRecord::Base.connection + schema_inspector = ActiveRecordDoctor::CachingSchemaInspector.new(connection) + @runner = ActiveRecordDoctor::Runner.new( config: load_config, logger: ActiveRecordDoctor::Logger::Dummy.new, - io: @io + io: @io, + schema_inspector: schema_inspector ) end diff --git a/test/setup.rb b/test/setup.rb index 6be93235..ec706b18 100644 --- a/test/setup.rb +++ b/test/setup.rb @@ -204,11 +204,17 @@ def detector_name def run_detector io = StringIO.new + + connection = ActiveRecord::Base.connection + schema_inspector = ActiveRecordDoctor::CachingSchemaInspector.new(connection) + runner = ActiveRecordDoctor::Runner.new( config: load_config, logger: ActiveRecordDoctor::Logger::Dummy.new, - io: io + io: io, + schema_inspector: schema_inspector ) + success = runner.run_one(detector_name) [success, io.string] end