Merge branch '388015-automatictically-identify-triggers-that-do-not-match-schema-2' into 'master'

Adds triggers to Database Schema validations

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113317



Merged-by: Diogo Frazão <dfrazao@gitlab.com>
Approved-by: Omar Qunsul <oqunsul@gitlab.com>
Approved-by: Tianwen Chen <tchen@gitlab.com>
Approved-by: Diogo Frazão <dfrazao@gitlab.com>
Reviewed-by: Peter Leitzen <pleitzen@gitlab.com>
Reviewed-by: Omar Qunsul <oqunsul@gitlab.com>
Reviewed-by: Tianwen Chen <tchen@gitlab.com>
Co-authored-by: Leonardo Rosa <ldarosa@gitlab.com>

lib/gitlab/database/schema_validation/database.rb

@@ -4,6 +4,8 @@ module Gitlab
   module Database
     module SchemaValidation
       class Database
+        STATIC_PARTITIONS_SCHEMA = 'gitlab_partitions_static'
+
         def initialize(connection)
           @connection = connection
         end
@@ -12,33 +14,69 @@ def fetch_index_by_name(index_name)
           index_map[index_name]
         end
 
-        def indexes
-          index_map.values
+        def fetch_trigger_by_name(trigger_name)
+          trigger_map[trigger_name]
         end
 
         def index_exists?(index_name)
           index_map[index_name].present?
         end
 
+        def trigger_exists?(trigger_name)
+          trigger_map[trigger_name].present?
+        end
+
+        def indexes
+          index_map.values
+        end
+
+        def triggers
+          trigger_map.values
+        end
+
         private
 
+        attr_reader :connection
+
+        def schemas
+          @schemas ||= [STATIC_PARTITIONS_SCHEMA, connection.current_schema]
+        end
+
         def index_map
           @index_map ||=
             fetch_indexes.transform_values! do |index_stmt|
-              Index.new(PgQuery.parse(index_stmt).tree.stmts.first.stmt.index_stmt)
+              SchemaObjects::Index.new(PgQuery.parse(index_stmt).tree.stmts.first.stmt.index_stmt)
             end
         end
 
-        attr_reader :connection
+        def trigger_map
+          @trigger_map ||=
+            fetch_triggers.transform_values! do |trigger_stmt|
+              SchemaObjects::Trigger.new(PgQuery.parse(trigger_stmt).tree.stmts.first.stmt.create_trig_stmt)
+            end
+        end
 
         def fetch_indexes
           sql = <<~SQL
             SELECT indexname, indexdef
             FROM pg_indexes
-            WHERE indexname NOT LIKE '%_pkey' AND schemaname IN ('public', 'gitlab_partitions_static');
+            WHERE indexname NOT LIKE '%_pkey' AND schemaname IN ($1, $2);
+          SQL
+
+          connection.select_rows(sql, nil, schemas).to_h
+        end
+
+        def fetch_triggers
+          sql = <<~SQL
+            SELECT triggers.tgname, pg_get_triggerdef(triggers.oid)
+            FROM pg_catalog.pg_trigger triggers
+            INNER JOIN pg_catalog.pg_class rel ON triggers.tgrelid = rel.oid
+            INNER JOIN pg_catalog.pg_namespace nsp ON nsp.oid = rel.relnamespace
+            WHERE triggers.tgisinternal IS FALSE
+            AND nsp.nspname IN ($1, $2)
           SQL
 
-          @fetch_indexes ||= connection.exec_query(sql).rows.to_h
+          connection.select_rows(sql, nil, schemas).to_h
         end
       end
     end

lib/gitlab/database/schema_validation/indexes.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Database
-    module SchemaValidation
-      class Indexes
-        def initialize(structure_sql, database)
-          @structure_sql = structure_sql
-          @database = database
-        end
-
-        def missing_indexes
-          structure_sql.indexes.map(&:name) - database.indexes.map(&:name)
-        end
-
-        def extra_indexes
-          database.indexes.map(&:name) - structure_sql.indexes.map(&:name)
-        end
-
-        def wrong_indexes
-          structure_sql.indexes.filter_map do |structure_sql_index|
-            database_index = database.fetch_index_by_name(structure_sql_index.name)
-
-            next if database_index.nil?
-            next if database_index.statement == structure_sql_index.statement
-
-            structure_sql_index.name
-          end
-        end
-
-        private
-
-        attr_reader :structure_sql, :database
-      end
-    end
-  end
-end

lib/gitlab/database/schema_validation/schema_objects/base.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaValidation
+      module SchemaObjects
+        class Base
+          def initialize(parsed_stmt)
+            @parsed_stmt = parsed_stmt
+          end
+
+          def name
+            raise NoMethodError, "subclasses of #{self.class.name} must implement #{__method__}"
+          end
+
+          def statement
+            @statement ||= PgQuery.deparse_stmt(parsed_stmt)
+          end
+
+          private
+
+          attr_reader :parsed_stmt
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_validation/schema_objects/index.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaValidation
+      module SchemaObjects
+        class Index < Base
+          def name
+            parsed_stmt.idxname
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_validation/schema_objects/trigger.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaValidation
+      module SchemaObjects
+        class Trigger < Base
+          def name
+            parsed_stmt.trigname
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_validation/structure_sql.rb

@@ -4,33 +4,56 @@ module Gitlab
   module Database
     module SchemaValidation
       class StructureSql
-        def initialize(structure_file_path)
+        DEFAULT_SCHEMA = 'public'
+
+        def initialize(structure_file_path, schema_name = DEFAULT_SCHEMA)
           @structure_file_path = structure_file_path
+          @schema_name = schema_name
         end
 
         def index_exists?(index_name)
           indexes.find { |index| index.name == index_name }.present?
         end
 
-        def indexes
-          @indexes ||= index_statements.map do |index_statement|
-            index_statement.relation.schemaname = "public" if index_statement.relation.schemaname == ''
+        def trigger_exists?(trigger_name)
+          triggers.find { |trigger| trigger.name == trigger_name }.present?
+        end
 
-            Index.new(index_statement)
+        def indexes
+          @indexes ||= map_with_default_schema(index_statements, SchemaObjects::Index)
         end
+
+        def triggers
+          @triggers ||= map_with_default_schema(trigger_statements, SchemaObjects::Trigger)
         end
 
         private
 
-        attr_reader :structure_file_path
+        attr_reader :structure_file_path, :schema_name
 
         def index_statements
-          parsed_structure_file.tree.stmts.filter_map { |s| s.stmt.index_stmt }
+          statements.filter_map { |s| s.stmt.index_stmt }
+        end
+
+        def trigger_statements
+          statements.filter_map { |s| s.stmt.create_trig_stmt }
+        end
+
+        def statements
+          @statements ||= parsed_structure_file.tree.stmts
         end
 
         def parsed_structure_file
           PgQuery.parse(File.read(structure_file_path))
         end
+
+        def map_with_default_schema(statements, validation_class)
+          statements.map do |statement|
+            statement.relation.schemaname = schema_name if statement.relation.schemaname == ''
+
+            validation_class.new(statement)
+          end
+        end
       end
     end
   end

lib/gitlab/database/schema_validation/validators/base_validator.rb

@@ -15,8 +15,11 @@ def initialize(structure_sql, database)
           def self.all_validators
             [
               ExtraIndexes,
+              ExtraTriggers,
               MissingIndexes,
-              DifferentDefinitionIndexes
+              MissingTriggers,
+              DifferentDefinitionIndexes,
+              DifferentDefinitionTriggers
             ]
           end
 

lib/gitlab/database/schema_validation/validators/different_definition_triggers.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaValidation
+      module Validators
+        class DifferentDefinitionTriggers < BaseValidator
+          def execute
+            structure_sql.triggers.filter_map do |structure_sql_trigger|
+              database_trigger = database.fetch_trigger_by_name(structure_sql_trigger.name)
+
+              next if database_trigger.nil?
+              next if database_trigger.statement == structure_sql_trigger.statement
+
+              build_inconsistency(self.class, structure_sql_trigger)
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_validation/index.rb → lib/gitlab/database/schema_validation/validators/extra_triggers.rb

@@ -3,22 +3,16 @@
 module Gitlab
   module Database
     module SchemaValidation
-      class Index
-        def initialize(parsed_stmt)
-          @parsed_stmt = parsed_stmt
-        end
+      module Validators
+        class ExtraTriggers < BaseValidator
+          def execute
+            database.triggers.filter_map do |trigger|
+              next if structure_sql.trigger_exists?(trigger.name)
 
-        def name
-          parsed_stmt.idxname
+              build_inconsistency(self.class, trigger)
+            end
           end
-
-        def statement
-          @statement ||= PgQuery.deparse_stmt(parsed_stmt)
         end
-
-        private
-
-        attr_reader :parsed_stmt
       end
     end
   end

lib/gitlab/database/schema_validation/validators/missing_triggers.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaValidation
+      module Validators
+        class MissingTriggers < BaseValidator
+          def execute
+            structure_sql.triggers.filter_map do |index|
+              next if database.trigger_exists?(index.name)
+
+              build_inconsistency(self.class, index)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/fixtures/structure.sql

@@ -18,3 +18,11 @@ CREATE TABLE ci_project_mirrors (
     project_id integer NOT NULL,
     namespace_id integer NOT NULL
 );
+
+CREATE TRIGGER trigger AFTER INSERT ON public.t1 FOR EACH ROW EXECUTE FUNCTION t1();
+
+CREATE TRIGGER wrong_trigger BEFORE UPDATE ON public.t2 FOR EACH ROW EXECUTE FUNCTION my_function();
+
+CREATE TRIGGER missing_trigger_1 BEFORE INSERT OR UPDATE ON public.t3 FOR EACH ROW EXECUTE FUNCTION t3();
+
+CREATE TRIGGER projects_loose_fk_trigger AFTER DELETE ON projects REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();

spec/lib/gitlab/database/schema_validation/database_spec.rb

@@ -3,19 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::SchemaValidation::Database, feature_category: :database do
-  let(:database_name) { 'main' }
-  let(:database_indexes) do
-    [['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))']]
-  end
+  subject(:database) { described_class.new(connection) }
 
-  let(:query_result) { instance_double('ActiveRecord::Result', rows: database_indexes) }
-  let(:database_model) { Gitlab::Database.database_base_models[database_name] }
+  let(:database_model) { Gitlab::Database.database_base_models['main'] }
   let(:connection) { database_model.connection }
 
-  subject(:database) { described_class.new(connection) }
+  context 'when having indexes' do
+    let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Index }
+    let(:results) do
+      [['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))']]
+    end
 
     before do
-    allow(connection).to receive(:exec_query).and_return(query_result)
+      allow(connection).to receive(:select_rows).and_return(results)
     end
 
     describe '#fetch_index_by_name' do
@@ -56,8 +56,55 @@
       it 'returns indexes' do
         indexes = database.indexes
 
-      expect(indexes).to all(be_a(Gitlab::Database::SchemaValidation::Index))
+        expect(indexes).to all(be_a(schema_object))
         expect(indexes.map(&:name)).to eq(['index'])
       end
     end
   end
+
+  context 'when having triggers' do
+    let(:schema_object) { Gitlab::Database::SchemaValidation::SchemaObjects::Trigger }
+    let(:results) do
+      { 'my_trigger' => 'CREATE TRIGGER my_trigger BEFORE INSERT ON todos FOR EACH ROW EXECUTE FUNCTION trigger()' }
+    end
+
+    before do
+      allow(database).to receive(:fetch_triggers).and_return(results)
+    end
+
+    describe '#fetch_trigger_by_name' do
+      context 'when trigger does not exist' do
+        it 'returns nil' do
+          expect(database.fetch_trigger_by_name('non_existing_trigger')).to be_nil
+        end
+      end
+
+      it 'returns trigger by name' do
+        expect(database.fetch_trigger_by_name('my_trigger').name).to eq('my_trigger')
+      end
+    end
+
+    describe '#trigger_exists?' do
+      context 'when trigger exists' do
+        it 'returns true' do
+          expect(database.trigger_exists?('my_trigger')).to be_truthy
+        end
+      end
+
+      context 'when trigger does not exist' do
+        it 'returns false' do
+          expect(database.trigger_exists?('non_existing_trigger')).to be_falsey
+        end
+      end
+    end
+
+    describe '#triggers' do
+      it 'returns triggers' do
+        triggers = database.triggers
+
+        expect(triggers).to all(be_a(schema_object))
+        expect(triggers.map(&:name)).to eq(['my_trigger'])
+      end
+    end
+  end
+end

spec/lib/gitlab/database/schema_validation/index_spec.rb

-# frozen_string_literal: true
-require 'spec_helper'
-
-RSpec.describe Gitlab::Database::SchemaValidation::Index, feature_category: :database do
-  let(:index_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' }
-
-  let(:stmt) { PgQuery.parse(index_statement).tree.stmts.first.stmt.index_stmt }
-
-  let(:index) { described_class.new(stmt) }
-
-  describe '#name' do
-    it 'returns index name' do
-      expect(index.name).to eq('index_name')
-    end
-  end
-
-  describe '#statement' do
-    it 'returns index statement' do
-      expect(index.statement).to eq(index_statement)
-    end
-  end
-end

spec/lib/gitlab/database/schema_validation/runner_spec.rb

@@ -7,7 +7,7 @@
   let(:connection) { ActiveRecord::Base.connection }
 
   let(:database) { Gitlab::Database::SchemaValidation::Database.new(connection) }
-  let(:structure_sql) { Gitlab::Database::SchemaValidation::StructureSql.new(structure_file_path) }
+  let(:structure_sql) { Gitlab::Database::SchemaValidation::StructureSql.new(structure_file_path, 'public') }
 
   describe '#execute' do
     subject(:inconsistencies) { described_class.new(structure_sql, database).execute }

spec/lib/gitlab/database/schema_validation/schema_objects/index_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Index, feature_category: :database do
+  let(:statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' }
+  let(:name) { 'index_name' }
+
+  include_examples 'schema objects assertions for', 'index_stmt'
+end

spec/lib/gitlab/database/schema_validation/schema_objects/trigger_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::SchemaValidation::SchemaObjects::Trigger, feature_category: :database do
+  let(:statement) { 'CREATE TRIGGER my_trigger BEFORE INSERT ON todos FOR EACH ROW EXECUTE FUNCTION trigger()' }
+  let(:name) { 'my_trigger' }
+
+  include_examples 'schema objects assertions for', 'create_trig_stmt'
+end

spec/lib/gitlab/database/schema_validation/structure_sql_spec.rb

@@ -4,9 +4,11 @@
 
 RSpec.describe Gitlab::Database::SchemaValidation::StructureSql, feature_category: :database do
   let(:structure_file_path) { Rails.root.join('spec/fixtures/structure.sql') }
+  let(:schema_name) { 'public' }
 
-  subject(:structure_sql) { described_class.new(structure_file_path) }
+  subject(:structure_sql) { described_class.new(structure_file_path, schema_name) }
 
+  context 'when having indexes' do
     describe '#index_exists?' do
       subject(:index_exists) { structure_sql.index_exists?(index_name) }
 
@@ -40,8 +42,41 @@
           index_users_on_public_email_excluding_null_and_empty
         ]
 
-      expect(indexes).to all(be_a(Gitlab::Database::SchemaValidation::Index))
+        expect(indexes).to all(be_a(Gitlab::Database::SchemaValidation::SchemaObjects::Index))
         expect(indexes.map(&:name)).to eq(expected_indexes)
       end
     end
   end
+
+  context 'when having triggers' do
+    describe '#trigger_exists?' do
+      subject(:trigger_exists) { structure_sql.trigger_exists?(name) }
+
+      context 'when the trigger does not exist' do
+        let(:name) { 'non-existent-trigger' }
+
+        it 'returns false' do
+          expect(trigger_exists).to be_falsey
+        end
+      end
+
+      context 'when the trigger exists' do
+        let(:name) { 'trigger' }
+
+        it 'returns true' do
+          expect(trigger_exists).to be_truthy
+        end
+      end
+    end
+
+    describe '#triggers' do
+      it 'returns triggers' do
+        triggers = structure_sql.triggers
+        expected_triggers = %w[trigger wrong_trigger missing_trigger_1 projects_loose_fk_trigger]
+
+        expect(triggers).to all(be_a(Gitlab::Database::SchemaValidation::SchemaObjects::Trigger))
+        expect(triggers.map(&:name)).to eq(expected_triggers)
+      end
+    end
+  end
+end

spec/lib/gitlab/database/schema_validation/validators/base_validator_spec.rb

@@ -9,8 +9,11 @@
     it 'returns an array of all validators' do
       expect(all_validators).to eq([
         Gitlab::Database::SchemaValidation::Validators::ExtraIndexes,
+        Gitlab::Database::SchemaValidation::Validators::ExtraTriggers,
         Gitlab::Database::SchemaValidation::Validators::MissingIndexes,
-        Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes
+        Gitlab::Database::SchemaValidation::Validators::MissingTriggers,
+        Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionIndexes,
+        Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTriggers
       ])
     end
   end

spec/lib/gitlab/database/schema_validation/validators/different_definition_triggers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::SchemaValidation::Validators::DifferentDefinitionTriggers,
+  feature_category: :database do
+  include_examples 'trigger validators', described_class, ['wrong_trigger']
+end

spec/lib/gitlab/database/schema_validation/validators/extra_triggers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::SchemaValidation::Validators::ExtraTriggers, feature_category: :database do
+  include_examples 'trigger validators', described_class, ['extra_trigger']
+end