From 9be77fc0dbb01c1a8a54cd3da97e16c7941df367 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Wed, 3 Jul 2024 15:36:42 +0200 Subject: [PATCH] Revert "Add system check for missing database indexes" (#30909) --- 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 deletions(-) delete mode 100644 app/lib/admin/db/schema_parser.rb delete mode 100644 app/lib/admin/system_check/missing_indexes_check.rb delete mode 100644 spec/lib/admin/db/schema_parser_spec.rb delete mode 100644 spec/lib/admin/system_check/missing_indexes_check_spec.rb diff --git a/Gemfile b/Gemfile index 769f834f4..ef52d50ca 100644 --- a/Gemfile +++ b/Gemfile @@ -229,4 +229,3 @@ gem 'rubyzip', '~> 2.3' gem 'hcaptcha', '~> 7.1' gem 'mail', '~> 2.8' -gem 'prism' diff --git a/Gemfile.lock b/Gemfile.lock index ca36c8cf9..eb6720e45 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -600,7 +600,6 @@ 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) @@ -1004,7 +1003,6 @@ 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 deleted file mode 100644 index e61a2281e..000000000 --- a/app/lib/admin/db/schema_parser.rb +++ /dev/null @@ -1,92 +0,0 @@ -# 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 453011f7a..25c88341a 100644 --- a/app/lib/admin/system_check.rb +++ b/app/lib/admin/system_check.rb @@ -8,7 +8,6 @@ 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 deleted file mode 100644 index b7eecbb06..000000000 --- a/app/lib/admin/system_check/missing_indexes_check.rb +++ /dev/null @@ -1,36 +0,0 @@ -# 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 270382dd1..20df80c27 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -862,8 +862,6 @@ 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 deleted file mode 100644 index e28d5c1f9..000000000 --- a/spec/lib/admin/db/schema_parser_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -# 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 deleted file mode 100644 index e183be562..000000000 --- a/spec/lib/admin/system_check/missing_indexes_check_spec.rb +++ /dev/null @@ -1,65 +0,0 @@ -# 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