From 4363999a100ce09f57078d32b3c92b37e2382f88 Mon Sep 17 00:00:00 2001
From: David Roetzel <david@roetzel.de>
Date: Mon, 1 Jul 2024 14:50:26 +0200
Subject: [PATCH] Add system check for missing indexes.

As part of #18965 we want to enable admins to see if one or
more indexes are missing. This adds a new "system check" to
display a warning in case indexes are missing.

The list of indexes we expect to be present are extracted from
`db/schema.rb` using ruby's new `prism` parser.
---
 Gemfile                                       |  1 +
 Gemfile.lock                                  |  2 +
 app/lib/admin/db/schema_parser.rb             | 83 +++++++++++++++++++
 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, 239 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..6bd08f2dd8
--- /dev/null
+++ b/app/lib/admin/db/schema_parser.rb
@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+class Admin::Db::SchemaParser
+  Index = Data.define(:name, :table_name, :columns, :options)
+
+  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..73443e93f5
--- /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)).compact
+      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: <code>%{value}</code>.<br> 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