Merge branch 'mmj-deprecate-project-authorizations-insert_authorizations' into 'master'

Remove `ProjectAuthorization.insert_authorizations` method

See merge request gitlab-org/gitlab!74492

app/models/project.rb

@@ -2750,6 +2750,12 @@ class Project < ApplicationRecord
     end
   end
 
+  def remove_project_authorizations(user_ids, per_batch = 1000)
+    user_ids.each_slice(per_batch) do |user_ids_batch|
+      project_authorizations.where(user_id: user_ids_batch).delete_all
+    end
+  end
+
   private
 
   # overridden in EE

app/models/project_authorization.rb

@@ -17,20 +17,6 @@ class ProjectAuthorization < ApplicationRecord
       .group(:project_id)
   end
 
-  def self.insert_authorizations(rows, per_batch = 1000)
-    rows.each_slice(per_batch) do |slice|
-      tuples = slice.map do |tuple|
-        tuple.map { |value| connection.quote(value) }
-      end
-
-      connection.execute <<-EOF.strip_heredoc
-        INSERT INTO project_authorizations (user_id, project_id, access_level)
-        VALUES #{tuples.map { |tuple| "(#{tuple.join(', ')})" }.join(', ')}
-        ON CONFLICT DO NOTHING
-      EOF
-    end
-  end
-
   # This method overrides its ActiveRecord's version in order to work correctly
   # with composite primary keys and fix the tests for Rails 6.1
   #
@@ -39,6 +25,12 @@ class ProjectAuthorization < ApplicationRecord
   def self.insert_all(attributes)
     super(attributes, unique_by: connection.schema_cache.primary_keys(table_name))
   end
+
+  def self.insert_all_in_batches(attributes, per_batch = 1000)
+    attributes.each_slice(per_batch) do |attributes_batch|
+      insert_all(attributes_batch)
+    end
+  end
 end
 
 ProjectAuthorization.prepend_mod_with('ProjectAuthorization')

app/services/authorized_project_update/find_records_due_for_refresh_service.rb

@@ -47,7 +47,11 @@ module AuthorizedProjectUpdate
             missing_auth_found_callback.call(project_id, level)
           end
 
-          array << [user.id, project_id, level]
+          array << {
+            user_id: user.id,
+            project_id: project_id,
+            access_level: level
+          }
         end
       end
 

app/services/authorized_project_update/project_group_link_create_service.rb

@@ -65,16 +65,8 @@ module AuthorizedProjectUpdate
     end
 
     def update_authorizations(user_ids_to_delete, authorizations_to_create)
-      ProjectAuthorization.transaction do
-        if user_ids_to_delete.any?
-          ProjectAuthorization.where(project_id: project.id, user_id: user_ids_to_delete) # rubocop: disable CodeReuse/ActiveRecord
-                              .delete_all
-        end
-
-        if authorizations_to_create.any?
-          ProjectAuthorization.insert_all(authorizations_to_create)
-        end
-      end
+      project.remove_project_authorizations(user_ids_to_delete) if user_ids_to_delete.any?
+      ProjectAuthorization.insert_all_in_batches(authorizations_to_create) if authorizations_to_create.any?
     end
   end
 end

app/services/authorized_project_update/project_recalculate_service.rb

@@ -64,16 +64,8 @@ module AuthorizedProjectUpdate
     end
 
     def refresh_authorizations
-      ProjectAuthorization.transaction do
-        if user_ids_to_remove.any?
-          ProjectAuthorization.where(project_id: project.id, user_id: user_ids_to_remove) # rubocop: disable CodeReuse/ActiveRecord
-                              .delete_all
-        end
-
-        if authorizations_to_create.any?
-          ProjectAuthorization.insert_all(authorizations_to_create)
-        end
-      end
+      project.remove_project_authorizations(user_ids_to_remove) if user_ids_to_remove.any?
+      ProjectAuthorization.insert_all_in_batches(authorizations_to_create) if authorizations_to_create.any?
     end
 
     def apply_scopes(project_authorizations)

app/services/users/refresh_authorized_projects_service.rb

@@ -63,12 +63,12 @@ module Users
     # Updates the list of authorizations for the current user.
     #
     # remove - The IDs of the authorization rows to remove.
-    # add - Rows to insert in the form `[user id, project id, access level]`
+    # add - Rows to insert in the form `[{ user_id: user_id, project_id: project_id, access_level: access_level}, ...]`
     def update_authorizations(remove = [], add = [])
       log_refresh_details(remove, add)
 
-      user.remove_project_authorizations(remove) unless remove.empty?
-      ProjectAuthorization.insert_authorizations(add) unless add.empty?
+      user.remove_project_authorizations(remove) if remove.any?
+      ProjectAuthorization.insert_all_in_batches(add) if add.any?
 
       # Since we batch insert authorization rows, Rails' associations may get
       # out of sync. As such we force a reload of the User object.
@@ -88,7 +88,7 @@ module Users
                                  # most often there's only a few entries in remove and add, but limit it to the first 5
                                  # entries to avoid flooding the logs
                                  'authorized_projects_refresh.rows_deleted_slice': remove.first(5),
-                                 'authorized_projects_refresh.rows_added_slice': add.first(5))
+                                 'authorized_projects_refresh.rows_added_slice': add.first(5).map(&:values))
     end
   end
 end

spec/models/project_authorization_spec.rb

@@ -3,40 +3,59 @@
 require 'spec_helper'
 
 RSpec.describe ProjectAuthorization do
+  describe 'relations' do
+    it { is_expected.to belong_to(:user) }
+    it { is_expected.to belong_to(:project) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:project) }
+    it { is_expected.to validate_presence_of(:user) }
+    it { is_expected.to validate_presence_of(:access_level) }
+    it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.all_values) }
+  end
+
+  describe '.insert_all' do
     let_it_be(:user) { create(:user) }
-  let_it_be(:project1) { create(:project) }
-  let_it_be(:project2) { create(:project) }
-  let_it_be(:project3) { create(:project) }
+    let_it_be(:project_1) { create(:project) }
+    let_it_be(:project_2) { create(:project) }
+    let_it_be(:project_3) { create(:project) }
 
-  describe '.insert_authorizations' do
-    it 'inserts the authorizations' do
-      described_class
-        .insert_authorizations([[user.id, project1.id, Gitlab::Access::MAINTAINER]])
+    it 'skips duplicates and inserts the remaining rows without error' do
+      create(:project_authorization, user: user, project: project_1, access_level: Gitlab::Access::MAINTAINER)
 
-      expect(user.project_authorizations.count).to eq(1)
-    end
+      attributes = [
+        { user_id: user.id, project_id: project_1.id, access_level: Gitlab::Access::MAINTAINER },
+        { user_id: user.id, project_id: project_2.id, access_level: Gitlab::Access::MAINTAINER },
+        { user_id: user.id, project_id: project_3.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
 
-    it 'inserts rows in batches' do
-      described_class.insert_authorizations([
-        [user.id, project1.id, Gitlab::Access::MAINTAINER],
-        [user.id, project2.id, Gitlab::Access::MAINTAINER]
-      ], 1)
+      described_class.insert_all(attributes)
 
-      expect(user.project_authorizations.count).to eq(2)
+      expect(user.project_authorizations.pluck(:user_id, :project_id, :access_level)).to match_array(attributes.map(&:values))
+    end
   end
 
-    it 'skips duplicates and inserts the remaining rows without error' do
-      create(:project_authorization, user: user, project: project1, access_level: Gitlab::Access::MAINTAINER)
+  describe '.insert_all_in_batches' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project_1) { create(:project) }
+    let_it_be(:project_2) { create(:project) }
+    let_it_be(:project_3) { create(:project) }
+
+    let(:per_batch_size) { 2 }
 
-      rows = [
-        [user.id, project1.id, Gitlab::Access::MAINTAINER],
-        [user.id, project2.id, Gitlab::Access::MAINTAINER],
-        [user.id, project3.id, Gitlab::Access::MAINTAINER]
+    it 'inserts the rows in batches, as per the `per_batch` size' do
+      attributes = [
+        { user_id: user.id, project_id: project_1.id, access_level: Gitlab::Access::MAINTAINER },
+        { user_id: user.id, project_id: project_2.id, access_level: Gitlab::Access::MAINTAINER },
+        { user_id: user.id, project_id: project_3.id, access_level: Gitlab::Access::MAINTAINER }
       ]
 
-      described_class.insert_authorizations(rows)
+      expect(described_class).to receive(:insert_all).twice.and_call_original
+
+      described_class.insert_all_in_batches(attributes, per_batch_size)
 
-      expect(user.project_authorizations.pluck(:user_id, :project_id, :access_level)).to match_array(rows)
+      expect(user.project_authorizations.pluck(:user_id, :project_id, :access_level)).to match_array(attributes.map(&:values))
     end
   end
 end

spec/models/project_spec.rb

@@ -851,6 +851,23 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#remove_project_authorizations' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:user_1) { create(:user) }
+    let_it_be(:user_2) { create(:user) }
+    let_it_be(:user_3) { create(:user) }
+
+    it 'removes the project authorizations of the specified users in the current project' do
+      create(:project_authorization, user: user_1, project: project)
+      create(:project_authorization, user: user_2, project: project)
+      create(:project_authorization, user: user_3, project: project)
+
+      project.remove_project_authorizations([user_1.id, user_2.id])
+
+      expect(project.project_authorizations.pluck(:user_id)).not_to include(user_1.id, user_2.id)
+    end
+  end
+
   describe 'reference methods' do
     let_it_be(:owner)     { create(:user, name: 'Gitlab') }
     let_it_be(:namespace) { create(:namespace, name: 'Sample namespace', path: 'sample-namespace', owner: owner) }

spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb

@@ -59,7 +59,9 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do
             .create!(project: project2, access_level: Gitlab::Access::MAINTAINER)
 
           to_be_removed = [project2.id]
-          to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]]
+          to_be_added = [
+            { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+          ]
 
           expect(service.execute).to eq([to_be_removed, to_be_added])
         end
@@ -70,7 +72,9 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do
           end
 
           to_be_removed = [project.id]
-          to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]]
+          to_be_added = [
+            { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+          ]
 
           expect(service.execute).to eq([to_be_removed, to_be_added])
         end
@@ -80,7 +84,9 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do
             .create!(project: project, access_level: Gitlab::Access::DEVELOPER)
 
           to_be_removed = [project.id]
-          to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]]
+          to_be_added = [
+            { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+          ]
 
           expect(service.execute).to eq([to_be_removed, to_be_added])
         end

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -67,11 +67,17 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
 
     it 'updates the authorized projects of the user' do
       project2 = create(:project)
-      to_remove = user.project_authorizations
+      project_authorization = user.project_authorizations
         .create!(project: project2, access_level: Gitlab::Access::MAINTAINER)
 
+      to_be_removed = [project_authorization.project_id]
+
+      to_be_added = [
+        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
+
       expect(service).to receive(:update_authorizations)
-        .with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
+        .with(to_be_removed, to_be_added)
 
       service.execute_without_lease
     end
@@ -81,9 +87,14 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
         user.project_authorizations.create!(project: project, access_level: access_level)
       end
 
+      to_be_removed = [project.id]
+
+      to_be_added = [
+        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
       expect(service).to(
         receive(:update_authorizations)
-          .with([project.id], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
+          .with(to_be_removed, to_be_added)
           .and_call_original)
 
       service.execute_without_lease
@@ -99,11 +110,17 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
     it 'sets the access level of a project to the highest available level' do
       user.project_authorizations.delete_all
 
-      to_remove = user.project_authorizations
+      project_authorization = user.project_authorizations
         .create!(project: project, access_level: Gitlab::Access::DEVELOPER)
 
+      to_be_removed = [project_authorization.project_id]
+
+      to_be_added = [
+        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
+
       expect(service).to receive(:update_authorizations)
-        .with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
+        .with(to_be_removed, to_be_added)
 
       service.execute_without_lease
     end
@@ -134,7 +151,11 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
     it 'inserts authorizations that should be added' do
       user.project_authorizations.delete_all
 
-      service.update_authorizations([], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
+      to_be_added = [
+        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
+
+      service.update_authorizations([], to_be_added)
 
       authorizations = user.project_authorizations
 
@@ -160,7 +181,11 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
                 'authorized_projects_refresh.rows_added_slice': [[user.id, project.id, Gitlab::Access::MAINTAINER]])
       )
 
-      service.update_authorizations([], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
+      to_be_added = [
+        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER }
+      ]
+
+      service.update_authorizations([], to_be_added)
     end
   end
 end