Stop using Sidekiq for authorized projectsrefresh-authorizations-without-sidekiq

Instead of using Sidekiq we now reset the column
users.authorized_projects_populated whenever authorizations have to be
refreshed. The next time the data is requested User#authorized_projects
will automatically refresh the data before returning it. When refreshing
data for single users we refresh the data right away.

This should solve any race conditions that may occur due to the
scheduling taking place in a transaction. It also simplifies the setup
by removing Sidekiq. This does come at the cost of some requests being a
bit slower whenever data has to be refreshed. This however should occur
only when authorizations have been changed, a process that thankfully
does not happen very often.

12 files changed, 74 insertions, 50 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 99675ddb366..7df188036e1 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -197,7 +197,7 @@ class Group < Namespace
   end
 
   def refresh_members_authorized_projects
-    UserProjectAccessChangedService.new(users_with_parents.pluck(:id)).execute
+    UserProjectAccessChangedService.new(users_with_parents).execute
   end
 
   def members_with_parents
diff --git a/app/models/member.rb b/app/models/member.rb
index c585e0b450e..c25a326ad9c 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -147,7 +147,7 @@ class Member < ActiveRecord::Base
         member.save
       end
 
-      UserProjectAccessChangedService.new(user.id).execute if user.is_a?(User)
+      UserProjectAccessChangedService.new(user).execute if user.is_a?(User)
 
       member
     end
@@ -275,12 +275,12 @@ class Member < ActiveRecord::Base
   end
 
   def post_create_hook
-    UserProjectAccessChangedService.new(user.id).execute
+    UserProjectAccessChangedService.new(user).execute
     system_hook_service.execute_hooks_for(self, :create)
   end
 
   def post_update_hook
-    UserProjectAccessChangedService.new(user.id).execute if access_level_changed?
+    UserProjectAccessChangedService.new(user).execute if access_level_changed?
   end
 
   def post_destroy_hook
@@ -294,7 +294,7 @@ class Member < ActiveRecord::Base
     # member is destroyed through association
     return if destroyed_by_association.present?
 
-    UserProjectAccessChangedService.new(user_id).execute
+    UserProjectAccessChangedService.new(user).execute
   end
 
   def after_accept_invite
diff --git a/app/models/user.rb b/app/models/user.rb
index 06dd98a3188..216d8bf5226 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -453,12 +453,21 @@ class User < ActiveRecord::Base
     end
   end
 
+  # Returns the authorized projects of a user, optionally restricted to a
+  # certain access level.
+  #
+  # Permissions are refreshed automatically whenever necessary.
   def authorized_projects(min_access_level = nil)
     refresh_authorized_projects unless authorized_projects_populated
 
-    # We're overriding an association, so explicitly call super with no arguments or it would be passed as `force_reload` to the association
+    # We're overriding an association, so explicitly call super with no
+    # arguments or it would be passed as `force_reload` to the association
     projects = super()
-    projects = projects.where('project_authorizations.access_level >= ?', min_access_level) if min_access_level
+
+    if min_access_level
+      projects = projects.
+        where('project_authorizations.access_level >= ?', min_access_level)
+    end
 
     projects
   end
diff --git a/app/services/user_project_access_changed_service.rb b/app/services/user_project_access_changed_service.rb
index 2469b4f0d7c..03d0fe143ce 100644
--- a/app/services/user_project_access_changed_service.rb
+++ b/app/services/user_project_access_changed_service.rb
@@ -1,9 +1,37 @@
+# Service that updates users so their authorizations are refreshed the next time
+# they are requested.
 class UserProjectAccessChangedService
-  def initialize(user_ids)
-    @user_ids = Array.wrap(user_ids)
+  def initialize(users)
+    @users = Array.wrap(users)
   end
 
   def execute
-    AuthorizedProjectsWorker.bulk_perform_async(@user_ids.map { |id| [id] })
+    @users.length == 1 ? refresh_single : refresh_multiple
+  end
+
+  # In most cases when a single user instance is passed it's probably going to
+  # be re-used afterwards (especially in the case of tests). Because of this we
+  # refresh the object right away in these cases, instead of doing so the next
+  # time authorizations are requested.
+  def refresh_single
+    @users[0].refresh_authorized_projects
+  end
+
+  # When refreshing multiple users we don't want to spend time doing this in the
+  # same request as doing so could take quite a lot of time. Instead we defer
+  # refreshing until the next time the authorizations are requested. This is
+  # based on the observation that multiple user objects are rarely (if ever)
+  # used directly after using this service class.
+  def refresh_multiple
+    user_ids = @users.map do |user|
+      # Ensure that existing instances refresh their data.
+      user.authorized_projects_populated = nil
+      user.id
+    end
+
+    # Ensure that any new instances are refreshed as well. This is necessary
+    # because some of the objects passed to this service might not be used
+    # afterwards.
+    User.where(id: user_ids).update_all(authorized_projects_populated: nil)
   end
 end
diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb
deleted file mode 100644
index 2badd0680fb..00000000000
--- a/app/workers/authorized_projects_worker.rb
+++ /dev/null
@@ -1,14 +0,0 @@
-class AuthorizedProjectsWorker
-  include Sidekiq::Worker
-  include DedicatedSidekiqQueue
-
-  def self.bulk_perform_async(args_list)
-    Sidekiq::Client.push_bulk('class' => self, 'args' => args_list)
-  end
-
-  def perform(user_id)
-    user = User.find_by(id: user_id)
-
-    user.refresh_authorized_projects if user
-  end
-end
diff --git a/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml b/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml
new file mode 100644
index 00000000000..c69a576a9a6
--- /dev/null
+++ b/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml
@@ -0,0 +1,4 @@
+---
+title: Stop using Sidekiq for authorized projects
+merge_request:
+author:
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 022b0e80917..33c5dfac489 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -36,7 +36,6 @@
   - [clear_database_cache, 1]
   - [delete_user, 1]
   - [delete_merged_branches, 1]
-  - [authorized_projects, 1]
   - [expire_build_instance_artifacts, 1]
   - [group_destroy, 1]
   - [irker, 1]
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index 370aeb9e0a9..bf6c7591ac2 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -61,7 +61,7 @@ describe GroupMember, models: true do
 
     describe '#after_accept_request' do
       it 'calls NotificationService.accept_group_access_request' do
-        member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now)
+        member = create(:group_member, user: create(:user), requested_at: Time.now)
 
         expect_any_instance_of(NotificationService).to receive(:new_group_member)
 
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 68f72f5c86e..d24d713edab 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -150,7 +150,7 @@ describe ProjectMember, models: true do
   describe 'notifications' do
     describe '#after_accept_request' do
       it 'calls NotificationService.new_project_member' do
-        member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now)
+        member = create(:project_member, user: create(:user), requested_at: Time.now)
 
         expect_any_instance_of(NotificationService).to receive(:new_project_member)
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 8b20ee81614..d8a1c5c81d2 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1144,9 +1144,13 @@ describe User, models: true do
       user    = create(:user)
 
       member = group.add_developer(user)
+      user.reload
+
       expect(user.authorized_projects).to include(project)
 
       member.destroy
+      user.reload
+
       expect(user.authorized_projects).not_to include(project)
     end
 
diff --git a/spec/services/user_project_access_changed_service_spec.rb b/spec/services/user_project_access_changed_service_spec.rb
new file mode 100644
index 00000000000..6bc6b4ccfe3
--- /dev/null
+++ b/spec/services/user_project_access_changed_service_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe UserProjectAccessChangedService do
+  describe '#execute' do
+    it 'schedules refreshing of user permissions' do
+      user1 = create(:user, authorized_projects_populated: true)
+      user2 = create(:user, authorized_projects_populated: true)
+
+      described_class.new([user1, user2]).execute
+
+      [user1, user2].each do |user|
+        user.reload
+        expect(user.authorized_projects_populated).to eq(nil)
+      end
+    end
+  end
+end
diff --git a/spec/workers/authorized_projects_worker_spec.rb b/spec/workers/authorized_projects_worker_spec.rb
deleted file mode 100644
index b6591f272f6..00000000000
--- a/spec/workers/authorized_projects_worker_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-require 'spec_helper'
-
-describe AuthorizedProjectsWorker do
-  let(:worker) { described_class.new }
-
-  describe '#perform' do
-    it "refreshes user's authorized projects" do
-      user = create(:user)
-
-      expect_any_instance_of(User).to receive(:refresh_authorized_projects)
-
-      worker.perform(user.id)
-    end
-
-    context "when the user is not found" do
-      it "does nothing" do
-        expect_any_instance_of(User).not_to receive(:refresh_authorized_projects)
-
-        described_class.new.perform(-1)
-      end
-    end
-  end
-end