From ebd8e1bbb6465c78f6542fe7f09938fdb768dbb7 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Wed, 3 Jul 2024 09:19:54 +0200 Subject: [PATCH] Add system check for missing database indexes (#30888) --- Gemfile | 1 + Gemfile.lock | 2 + app/lib/admin/db/schema_parser.rb | 92 +++++++++++++++++++ app/lib/admin/system_check.rb | 1 + .../system_check/missing_indexes_check.rb | 36 ++++++++ config/locales/en.yml | 2 + spec/lib/admin/db/schema_parser_spec.rb | 49 ++++++++++ .../missing_indexes_check_spec.rb | 65 +++++++++++++ 8 files changed, 248 insertions(+) create mode 100644 app/lib/admin/db/schema_parser.rb create mode 100644 app/lib/admin/system_check/missing_indexes_check.rb create mode 100644 spec/lib/admin/db/schema_parser_spec.rb create mode 100644 spec/lib/admin/system_check/missing_indexes_check_spec.rb diff --git a/Gemfile b/Gemfile index be3f9e6f98..0d6cc4c4e1 100644 --- a/Gemfile +++ b/Gemfile @@ -229,3 +229,4 @@ gem 'rubyzip', '~> 2.3' gem 'hcaptcha', '~> 7.1' gem 'mail', '~> 2.8' +gem 'prism' diff --git a/Gemfile.lock b/Gemfile.lock index 42cc0e1986..969d18493a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -600,6 +600,7 @@ GEM actionmailer (>= 3) net-smtp premailer (~> 1.7, >= 1.7.9) + prism (0.30.0) propshaft (0.9.0) actionpack (>= 7.0.0) activesupport (>= 7.0.0) @@ -1003,6 +1004,7 @@ DEPENDENCIES pg (~> 1.5) pghero premailer-rails + prism propshaft public_suffix (~> 6.0) puma (~> 6.3) diff --git a/app/lib/admin/db/schema_parser.rb b/app/lib/admin/db/schema_parser.rb new file mode 100644 index 0000000000..e61a2281ee --- /dev/null +++ b/app/lib/admin/db/schema_parser.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +class Admin::Db::SchemaParser + class Index + attr_reader :name, :table_name, :columns, :options + + def initialize(name:, table_name:, columns:, options:) + @name = name + @table_name = table_name + @columns = columns + @options = options + end + end + + attr_reader :indexes_by_table + + def initialize(source) + parse(source) + end + + private + + def parse(source) + @indexes_by_table = {} + queue = [Prism.parse(source).value] + while (node = queue.shift) + if node.type == :call_node && node.name == :create_table + parse_create_table(node) + elsif node.type == :call_node && node.name == :add_index + parse_add_index(node) + else + queue.concat(node.compact_child_nodes) + end + end + end + + def parse_create_table(node) + table_name = parse_arguments(node).first + queue = node.compact_child_nodes + while (node = queue.shift) + if node.type == :call_node && node.name == :index + parse_index(node, table_name:) + else + queue.concat(node.compact_child_nodes) + end + end + end + + def parse_index(node, table_name:) + arguments = parse_arguments(node) + save_index( + name: arguments.last[:name], + table_name: table_name, + columns: arguments.first, + options: arguments.last + ) + end + + def parse_add_index(node) + arguments = parse_arguments(node) + save_index( + name: arguments.last[:name], + table_name: arguments.first, + columns: arguments[1], + options: arguments.last + ) + end + + def parse_arguments(node) + node.arguments.arguments.map { |a| parse_argument(a) } + end + + def parse_argument(argument) + case argument + when Prism::StringNode + argument.unescaped + when Prism::SymbolNode + argument.unescaped.to_sym + when Prism::ArrayNode + argument.elements.map { |e| parse_argument(e) } + when Prism::KeywordHashNode + argument.elements.to_h do |element| + [element.key.unescaped.to_sym, parse_argument(element.value)] + end + end + end + + def save_index(name:, table_name:, columns:, options:) + @indexes_by_table[table_name] ||= [] + @indexes_by_table[table_name] << Index.new(name:, table_name:, columns:, options:) + end +end diff --git a/app/lib/admin/system_check.rb b/app/lib/admin/system_check.rb index 25c88341a4..453011f7a6 100644 --- a/app/lib/admin/system_check.rb +++ b/app/lib/admin/system_check.rb @@ -8,6 +8,7 @@ class Admin::SystemCheck Admin::SystemCheck::SidekiqProcessCheck, Admin::SystemCheck::RulesCheck, Admin::SystemCheck::ElasticsearchCheck, + Admin::SystemCheck::MissingIndexesCheck, ].freeze def self.perform(current_user) diff --git a/app/lib/admin/system_check/missing_indexes_check.rb b/app/lib/admin/system_check/missing_indexes_check.rb new file mode 100644 index 0000000000..b7eecbb067 --- /dev/null +++ b/app/lib/admin/system_check/missing_indexes_check.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +class Admin::SystemCheck::MissingIndexesCheck < Admin::SystemCheck::BaseCheck + def skip? + !current_user.can?(:view_devops) + end + + def pass? + missing_indexes.none? + end + + def message + Admin::SystemCheck::Message.new(:missing_indexes_check, missing_indexes.join(', ')) + end + + private + + def missing_indexes + @missing_indexes ||= begin + expected_indexes_by_table.flat_map do |table, indexes| + expected_indexes = indexes.map(&:name) + expected_indexes - existing_indexes_for(table) + end + end + end + + def expected_indexes_by_table + schema_rb = Rails.root.join('db', 'schema.rb').read + schema_parser = Admin::Db::SchemaParser.new(schema_rb) + schema_parser.indexes_by_table + end + + def existing_indexes_for(table) + ActiveRecord::Base.connection.indexes(table).map(&:name) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 20df80c272..270382dd11 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -862,6 +862,8 @@ en: elasticsearch_version_check: message_html: 'Incompatible Elasticsearch version: %{value}' version_comparison: Elasticsearch %{running_version} is running while %{required_version} is required + missing_indexes_check: + message_html: 'The following indexes are missing from the database and should be recreated: %{value}.
Missing indexes may lead to severely reduced performance and data inconsistencies.' rules_check: action: Manage server rules message_html: You haven't defined any server rules. diff --git a/spec/lib/admin/db/schema_parser_spec.rb b/spec/lib/admin/db/schema_parser_spec.rb new file mode 100644 index 0000000000..e28d5c1f97 --- /dev/null +++ b/spec/lib/admin/db/schema_parser_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Admin::Db::SchemaParser do + let(:dummy_schema) do + <<~SCHEMA + # Comment + ActiveRecord::Schema[7.1].define(version: 23) do + create_table "people", force: :cascade do |t| + t.string "name" + end + + create_table "posts", force: :cascade do |t| + t.string "title", null: false + t.bigint "size", null: false + t.string "description" + # t.index ["size", "title"], name: "index_posts_on_size_and_title" + t.index ["title"], name: "index_posts_on_title", unique: true + t.index ["size"], name: "index_posts_on_size" + end + + # add_index "people", ["name"], name: "commented_out_index" + add_index "people", ["name"], name: "index_people_on_name" + end + SCHEMA + end + let(:schema_parser) { described_class.new(dummy_schema) } + + describe '#indexes_by_table' do + subject { schema_parser.indexes_by_table } + + it 'returns index info for all affected tables' do + expect(subject.keys).to match_array(%w(people posts)) + end + + it 'returns all index information for the `people` table' do + people_info = subject['people'] + expect(people_info.map(&:name)).to contain_exactly('index_people_on_name') + end + + it 'returns all index information for the `posts` table' do + posts_info = subject['posts'] + expect(posts_info.map(&:name)).to contain_exactly( + 'index_posts_on_title', 'index_posts_on_size' + ) + end + end +end diff --git a/spec/lib/admin/system_check/missing_indexes_check_spec.rb b/spec/lib/admin/system_check/missing_indexes_check_spec.rb new file mode 100644 index 0000000000..e183be5620 --- /dev/null +++ b/spec/lib/admin/system_check/missing_indexes_check_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Admin::SystemCheck::MissingIndexesCheck do + subject(:check) { described_class.new(user) } + + let(:user) { Fabricate(:user) } + let(:schema_parser) do + instance_double(Admin::Db::SchemaParser, indexes_by_table: index_info) + end + let(:index_info) do + { + 'users' => [instance_double(Admin::Db::SchemaParser::Index, name: 'index_users_on_profile_id')], + 'posts' => [instance_double(Admin::Db::SchemaParser::Index, name: 'index_posts_on_user_id')], + } + end + let(:posts_indexes) { [] } + let(:users_indexes) { [] } + + before do + allow(Admin::Db::SchemaParser).to receive(:new).and_return(schema_parser) + allow(ActiveRecord::Base.connection).to receive(:indexes).with('posts').and_return(posts_indexes) + allow(ActiveRecord::Base.connection).to receive(:indexes).with('users').and_return(users_indexes) + end + + it_behaves_like 'a check available to devops users' + + describe '#pass?' do + context 'when indexes are missing' do + let(:posts_indexes) do + [instance_double(ActiveRecord::ConnectionAdapters::IndexDefinition, name: 'index_posts_on_user_id')] + end + + it 'returns false' do + expect(check.pass?).to be false + end + end + + context 'when all expected indexes are present' do + let(:posts_indexes) do + [instance_double(ActiveRecord::ConnectionAdapters::IndexDefinition, name: 'index_posts_on_user_id')] + end + let(:users_indexes) do + [instance_double(ActiveRecord::ConnectionAdapters::IndexDefinition, name: 'index_users_on_profile_id')] + end + + it 'returns true' do + expect(check.pass?).to be true + end + end + end + + describe '#message' do + subject { check.message } + + it 'sets the class name as the message key' do + expect(subject.key).to eq(:missing_indexes_check) + end + + it 'sets a list of missing indexes as message value' do + expect(subject.value).to eq('index_users_on_profile_id, index_posts_on_user_id') + end + end +end