Merge branch 'sarnold-revert-36198' into 'master'

Revert "Refactor AuditEvent to use target_type database value"

See merge request gitlab-org/gitlab!39290

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, :target_type].freeze
+  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path, :target_details].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,7 +29,6 @@ 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
@@ -43,7 +42,6 @@ ELSIF (TG_OP = 'INSERT') THEN
     author_name,
     entity_path,
     target_details,
-    target_type,
     created_at)
   VALUES (NEW.id,
     NEW.author_id,
@@ -55,7 +53,6 @@ ELSIF (TG_OP = 'INSERT') THEN
     NEW.author_name,
     NEW.entity_path,
     NEW.target_details,
-    NEW.target_type,
     NEW.created_at);
 END IF;
 RETURN NULL;
@@ -77,10 +74,8 @@ 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);
@@ -9475,9 +9470,7 @@ 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,7 +15,6 @@ 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
@@ -24,7 +23,7 @@ module EE
             remove: "user_access",
             author_name: @author.name,
             target_id: user_id,
-            target_type: target_type,
+            target_type: "User",
             target_details: user_name
           }
         when :expired
@@ -32,7 +31,7 @@ module EE
             remove: "user_access",
             author_name: member.created_by ? member.created_by.name : 'Deleted User',
             target_id: user_id,
-            target_type: target_type,
+            target_type: "User",
             target_details: user_name,
             system_event: true,
             reason: "access expired on #{member.expires_at}"
@@ -43,7 +42,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: target_type,
+            target_type: "User",
             target_details: user_name
           }
         when :update, :override
@@ -55,7 +54,7 @@ module EE
             expiry_to: member.expires_at,
             author_name: @author.name,
             target_id: user_id,
-            target_type: target_type,
+            target_type: "User",
             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', target_type: 'User' }
+        { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab', target_details: 'Project X' }
       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,11 +42,6 @@ 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, {
@@ -282,22 +277,6 @@ 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) }
@@ -372,55 +351,6 @@ 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 }