Refactor AuditEvent to use target_type database value

Currently, the target_type is stored in a hash
rather than in a more easily accessible database
column. This commit creates the new column
and updates the write logic to write to both.

app/models/audit_event.rb

@@ -5,7 +5,7 @@ class AuditEvent < ApplicationRecord
   include IgnorableColumns
   include BulkInsertSafe
 
-  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path, :target_details].freeze
+  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path, :target_details, :target_type].freeze
 
   ignore_column :updated_at, remove_with: '13.4', remove_after: '2020-09-22'
 

changelogs/unreleased/220322-refactor-target-type.yml

+---
+title: Add new target_type column on audit_events table
+merge_request: 36198
+author:
+type: changed

db/post_migrate/20200810100921_add_target_type_to_audit_event.rb

+# frozen_string_literal: true
+
+class AddTargetTypeToAuditEvent < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::SchemaHelpers
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  SOURCE_TABLE_NAME = 'audit_events'
+  PARTITIONED_TABLE_NAME = 'audit_events_part_5fc467ac26'
+  TRIGGER_FUNCTION_NAME = 'table_sync_function_2be879775d'
+
+  def up
+    with_lock_retries do
+      # rubocop:disable Migration/AddLimitToTextColumns
+      add_column('audit_events', :target_type, :text)
+      add_column('audit_events_part_5fc467ac26', :target_type, :text)
+      # rubocop:enable Migration/AddLimitToTextColumns
+
+      create_trigger_function(TRIGGER_FUNCTION_NAME, replace: true) do
+        <<~SQL
+          IF (TG_OP = 'DELETE') THEN
+            DELETE FROM #{PARTITIONED_TABLE_NAME} where id = OLD.id;
+          ELSIF (TG_OP = 'UPDATE') THEN
+            UPDATE #{PARTITIONED_TABLE_NAME}
+            SET author_id = NEW.author_id,
+              type = NEW.type,
+              entity_id = NEW.entity_id,
+              entity_type = NEW.entity_type,
+              details = NEW.details,
+              ip_address = NEW.ip_address,
+              author_name = NEW.author_name,
+              entity_path = NEW.entity_path,
+              target_details = NEW.target_details,
+              target_type = NEW.target_type,
+              created_at = NEW.created_at
+            WHERE #{PARTITIONED_TABLE_NAME}.id = NEW.id;
+          ELSIF (TG_OP = 'INSERT') THEN
+            INSERT INTO #{PARTITIONED_TABLE_NAME} (id,
+              author_id,
+              type,
+              entity_id,
+              entity_type,
+              details,
+              ip_address,
+              author_name,
+              entity_path,
+              target_details,
+              target_type,
+              created_at)
+            VALUES (NEW.id,
+              NEW.author_id,
+              NEW.type,
+              NEW.entity_id,
+              NEW.entity_type,
+              NEW.details,
+              NEW.ip_address,
+              NEW.author_name,
+              NEW.entity_path,
+              NEW.target_details,
+              NEW.target_type,
+              NEW.created_at);
+          END IF;
+          RETURN NULL;
+        SQL
+      end
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column SOURCE_TABLE_NAME, :target_type
+
+      create_trigger_function(TRIGGER_FUNCTION_NAME, replace: true) do
+        <<~SQL
+          IF (TG_OP = 'DELETE') THEN
+            DELETE FROM #{PARTITIONED_TABLE_NAME} where id = OLD.id;
+          ELSIF (TG_OP = 'UPDATE') THEN
+            UPDATE #{PARTITIONED_TABLE_NAME}
+            SET author_id = NEW.author_id,
+              type = NEW.type,
+              entity_id = NEW.entity_id,
+              entity_type = NEW.entity_type,
+              details = NEW.details,
+              ip_address = NEW.ip_address,
+              author_name = NEW.author_name,
+              entity_path = NEW.entity_path,
+              target_details = NEW.target_details,
+              created_at = NEW.created_at
+            WHERE #{PARTITIONED_TABLE_NAME}.id = NEW.id;
+          ELSIF (TG_OP = 'INSERT') THEN
+            INSERT INTO #{PARTITIONED_TABLE_NAME} (id,
+              author_id,
+              type,
+              entity_id,
+              entity_type,
+              details,
+              ip_address,
+              author_name,
+              entity_path,
+              target_details,
+              created_at)
+            VALUES (NEW.id,
+              NEW.author_id,
+              NEW.type,
+              NEW.entity_id,
+              NEW.entity_type,
+              NEW.details,
+              NEW.ip_address,
+              NEW.author_name,
+              NEW.entity_path,
+              NEW.target_details,
+              NEW.created_at);
+          END IF;
+          RETURN NULL;
+        SQL
+      end
+
+      remove_column PARTITIONED_TABLE_NAME, :target_type
+    end
+  end
+end

db/post_migrate/20200810101029_add_text_limit_to_audit_event_target_type.rb

+# frozen_string_literal: true
+
+class AddTextLimitToAuditEventTargetType < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+  DOWNTIME = false
+  SOURCE_TABLE_NAME = 'audit_events'
+  PARTITIONED_TABLE_NAME = 'audit_events_part_5fc467ac26'
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit(SOURCE_TABLE_NAME, :target_type, 255)
+    add_text_limit(PARTITIONED_TABLE_NAME, :target_type, 255)
+  end
+
+  def down
+    remove_text_limit(SOURCE_TABLE_NAME, :target_type)
+    remove_text_limit(PARTITIONED_TABLE_NAME, :target_type)
+  end
+end

db/schema_migrations/20200810100921

+fe96df46a5f360cafe8f9816c6dfc2d00afdcf458fb38ace37ce59999dba2413
\ No newline at end of file

db/schema_migrations/20200810101029

+8bb03ea2ded957a41aa1efd60a9908a3c597aaaade9190f8f1515bfd2ab9a282
\ No newline at end of file

db/structure.sql

@@ -29,6 +29,7 @@ ELSIF (TG_OP = 'UPDATE') THEN
     author_name = NEW.author_name,
     entity_path = NEW.entity_path,
     target_details = NEW.target_details,
+    target_type = NEW.target_type,
     created_at = NEW.created_at
   WHERE audit_events_part_5fc467ac26.id = NEW.id;
 ELSIF (TG_OP = 'INSERT') THEN
@@ -42,6 +43,7 @@ ELSIF (TG_OP = 'INSERT') THEN
     author_name,
     entity_path,
     target_details,
+    target_type,
     created_at)
   VALUES (NEW.id,
     NEW.author_id,
@@ -53,6 +55,7 @@ ELSIF (TG_OP = 'INSERT') THEN
     NEW.author_name,
     NEW.entity_path,
     NEW.target_details,
+    NEW.target_type,
     NEW.created_at);
 END IF;
 RETURN NULL;
@@ -74,8 +77,10 @@ CREATE TABLE public.audit_events_part_5fc467ac26 (
     entity_path text,
     target_details text,
     created_at timestamp without time zone NOT NULL,
+    target_type text,
     CONSTRAINT check_492aaa021d CHECK ((char_length(entity_path) <= 5500)),
     CONSTRAINT check_83ff8406e2 CHECK ((char_length(author_name) <= 255)),
+    CONSTRAINT check_97a8c868e7 CHECK ((char_length(target_type) <= 255)),
     CONSTRAINT check_d493ec90b5 CHECK ((char_length(target_details) <= 5500))
 )
 PARTITION BY RANGE (created_at);
@@ -9470,7 +9475,9 @@ CREATE TABLE public.audit_events (
     author_name text,
     entity_path text,
     target_details text,
+    target_type text,
     CONSTRAINT check_492aaa021d CHECK ((char_length(entity_path) <= 5500)),
+    CONSTRAINT check_82294106dd CHECK ((char_length(target_type) <= 255)),
     CONSTRAINT check_83ff8406e2 CHECK ((char_length(author_name) <= 255)),
     CONSTRAINT check_d493ec90b5 CHECK ((char_length(target_details) <= 5500))
 );

ee/app/services/ee/audit_event_service.rb

@@ -15,6 +15,7 @@ module EE
       old_access_level = @details[:old_access_level]
       user_id = member.id
       user_name = member.user ? member.user.name : 'Deleted User'
+      target_type = 'User'
 
       @details =
         case action
@@ -23,7 +24,7 @@ module EE
             remove: "user_access",
             author_name: @author.name,
             target_id: user_id,
-            target_type: "User",
+            target_type: target_type,
             target_details: user_name
           }
         when :expired
@@ -31,7 +32,7 @@ module EE
             remove: "user_access",
             author_name: member.created_by ? member.created_by.name : 'Deleted User',
             target_id: user_id,
-            target_type: "User",
+            target_type: target_type,
             target_details: user_name,
             system_event: true,
             reason: "access expired on #{member.expires_at}"
@@ -42,7 +43,7 @@ module EE
             as: ::Gitlab::Access.options_with_owner.key(member.access_level.to_i),
             author_name: @author.name,
             target_id: user_id,
-            target_type: "User",
+            target_type: target_type,
             target_details: user_name
           }
         when :update, :override
@@ -54,7 +55,7 @@ module EE
             expiry_to: member.expires_at,
             author_name: @author.name,
             target_id: user_id,
-            target_type: "User",
+            target_type: target_type,
             target_details: user_name
           }
         end

ee/spec/models/audit_event_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe AuditEvent, type: :model do
   describe 'callbacks' do
     context 'parallel_persist' do
       let_it_be(:details) do
-        { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab', target_details: 'Project X' }
+        { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab', target_details: 'Project X', target_type: 'User' }
       end
       let_it_be(:event) { create(:project_audit_event, details: details, entity_path: nil, target_details: nil) }
 

ee/spec/services/audit_event_service_spec.rb

@@ -42,6 +42,11 @@ RSpec.describe AuditEventService do
       end
     end
 
+    it 'generates a system event' do
+      expect(event[:details][:target_type]).to eq('User')
+      expect(event.target_type).to eq('User')
+    end
+
     context 'updating membership' do
       let(:service) do
         described_class.new(user, project, {
@@ -277,6 +282,22 @@ RSpec.describe AuditEventService do
     end
   end
 
+  describe '#for_project_group_link' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:link) { create(:project_group_link, group: group, project: project) }
+
+    let(:options) { { action: :create } }
+
+    subject(:event) { described_class.new(current_user, project, options).for_project_group_link(link).security_event }
+
+    it 'sets the target_type attribute' do
+      expect(event.details[:target_type]).to eq('Project')
+      expect(event.target_type).to eq('Project')
+    end
+  end
+
   describe '#for_user' do
     let(:author_name) { 'Administrator' }
     let(:current_user) { User.new(name: author_name) }
@@ -351,6 +372,55 @@ RSpec.describe AuditEventService do
     end
   end
 
+  describe '#for_project' do
+    let_it_be(:author_name) { 'Administrator' }
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:action) { :destroy }
+    let_it_be(:options) { { action: action } }
+
+    subject(:event) { described_class.new(current_user, project, options).for_project.security_event }
+
+    it 'sets the details attribute' do
+      expect(subject.details).to eq({
+         remove: 'project',
+         author_name: current_user.name,
+         target_id: project.full_path,
+         target_type: 'Project',
+         target_details: project.full_path
+      })
+    end
+
+    it 'sets the target_type column' do
+      expect(subject.target_type).to eq('Project')
+    end
+  end
+
+  describe '#for_group' do
+    let_it_be(:author_name) { 'Administrator' }
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:action) { :destroy }
+    let_it_be(:options) { { action: action } }
+    let_it_be(:service) { described_class.new(current_user, group, options).for_group }
+
+    subject(:event) { service.security_event }
+
+    it 'sets the details attribute' do
+      expect(event.details).to eq({
+         remove: 'group',
+         author_name: current_user.name,
+         target_id: group.full_path,
+         target_type: 'Group',
+         target_details: group.full_path
+      })
+    end
+
+    it 'stores target_type in a database column' do
+      expect(subject.target_type).to eq('Group')
+    end
+  end
+
   describe 'license' do
     let(:event) { service.for_project.security_event }