Merge branch 'forbid-the-usage-of-reload-ee' into 'master'

EE: Forbid the use of `#reload` and prefer `#reset`

Closes gitlab-ce#60218

See merge request gitlab-org/gitlab-ee!10647

.rubocop.yml

@@ -207,3 +207,9 @@ Style/ReturnNil:
 # nil values on the left hand side
 Performance/RegexpMatch:
   Enabled: false
+
+ActiveRecordAssociationReload:
+  Enabled: true
+  Exclude:
+    - 'spec/**/*'
+    - 'ee/spec/**/*'

app/controllers/admin/projects_controller.rb

@@ -40,7 +40,7 @@ class Admin::ProjectsController < Admin::ApplicationController
     namespace = Namespace.find_by(id: params[:new_namespace_id])
     ::Projects::TransferService.new(@project, current_user, params.dup).execute(namespace)
 
-    @project.reload
+    @project.reset
     redirect_to admin_project_path(@project)
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/controllers/profiles/passwords_controller.rb

@@ -55,7 +55,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController
       flash[:notice] = "Password was successfully updated. Please login with it"
       redirect_to new_user_session_path
     else
-      @user.reload
+      @user.reset
       render 'edit'
     end
   end

app/controllers/projects/imports_controller.rb

@@ -14,7 +14,7 @@ class Projects::ImportsController < Projects::ApplicationController
 
   def create
     if @project.update(import_params)
-      @project.import_state.reload.schedule
+      @project.import_state.reset.schedule
     end
 
     redirect_to project_import_path(@project)

app/controllers/projects_controller.rb

@@ -237,7 +237,7 @@ class ProjectsController < Projects::ApplicationController
 
   def toggle_star
     current_user.toggle_star(@project)
-    @project.reload
+    @project.reset
 
     render json: {
       star_count: @project.star_count

app/models/application_record.rb

@@ -3,6 +3,8 @@
 class ApplicationRecord < ActiveRecord::Base
   self.abstract_class = true
 
+  alias_method :reset, :reload
+
   def self.id_in(ids)
     where(id: ids)
   end

app/models/merge_request_diff.rb

@@ -387,7 +387,7 @@ class MergeRequestDiff < ApplicationRecord
       save!
     end
 
-    merge_request_diff_files.reload
+    merge_request_diff_files.reset
   end
 
   private
@@ -541,10 +541,10 @@ class MergeRequestDiff < ApplicationRecord
   def save_commits
     MergeRequestDiffCommit.create_bulk(self.id, compare.commits.reverse)
 
-    # merge_request_diff_commits.reload is preferred way to reload associated
+    # merge_request_diff_commits.reset is preferred way to reload associated
     # objects but it returns cached result for some reason in this case
     # we can circumvent that by specifying that we need an uncached reload
-    commits = self.class.uncached { merge_request_diff_commits.reload }
+    commits = self.class.uncached { merge_request_diff_commits.reset }
     self.commits_count = commits.size
   end
 

app/services/groups/destroy_service.rb

@@ -20,7 +20,7 @@ module Groups
       end
 
       # reload the relation to prevent triggering destroy hooks on the projects again
-      group.projects.reload
+      group.projects.reset
 
       group.children.each do |group|
         # This needs to be synchronous since the namespace gets destroyed below

app/services/notes/update_service.rb

@@ -22,7 +22,7 @@ module Notes
 
         # We need to refresh the previous suggestions call cache
         # in order to get the new records.
-        note.reload
+        note.reset
       end
 
       note

app/services/projects/detect_repository_languages_service.rb

@@ -29,7 +29,7 @@ module Projects
         set_detected_repository_languages
       end
 
-      project.repository_languages.reload
+      project.repository_languages.reset
     end
     # rubocop: enable CodeReuse/ActiveRecord
 

app/services/projects/move_project_group_links_service.rb

@@ -26,7 +26,7 @@ module Projects
 
     # Remove remaining project group links from source_project
     def remove_remaining_project_group_links
-      source_project.reload.project_group_links.destroy_all # rubocop: disable DestroyAll
+      source_project.reset.project_group_links.destroy_all # rubocop: disable DestroyAll
     end
 
     def group_links_in_target_project

app/services/projects/transfer_service.rb

@@ -30,7 +30,7 @@ module Projects
 
       true
     rescue Projects::TransferService::TransferError => ex
-      project.reload
+      project.reset
       project.errors.add(:new_namespace, ex.message)
       false
     end
@@ -122,7 +122,7 @@ module Projects
 
     def rollback_side_effects
       rollback_folder_move
-      project.reload
+      project.reset
       update_namespace_and_visibility(@old_namespace)
       update_repository_configuration(@old_path)
     end

app/services/users/migrate_to_ghost_user_service.rb

@@ -33,7 +33,7 @@ module Users
         end
       end
 
-      user.reload
+      user.reset
     end
 
     private

app/services/users/refresh_authorized_projects_service.rb

@@ -25,7 +25,7 @@ module Users
       # We need an up to date User object that has access to all relations that
       # may have been created earlier. The only way to ensure this is to reload
       # the User object.
-      user.reload
+      user.reset
     end
 
     def execute
@@ -84,7 +84,7 @@ module Users
 
       # Since we batch insert authorization rows, Rails' associations may get
       # out of sync. As such we force a reload of the User object.
-      user.reload
+      user.reset
     end
 
     def fresh_access_levels_per_project

app/views/devise/mailer/email_changed.html.haml

@@ -2,7 +2,7 @@
 
 - if @resource.try(:unconfirmed_email?)
   %p
-    We're contacting you to notify you that your email is being changed to #{@resource.reload.unconfirmed_email}.
+    We're contacting you to notify you that your email is being changed to #{@resource.reset.unconfirmed_email}.
 - else
   %p
     We're contacting you to notify you that your email has been changed to #{@resource.email}.

app/views/devise/mailer/email_changed.text.erb

 Hello, <%= @resource.name %>!
 
 <% if @resource.try(:unconfirmed_email?) %>
-We're contacting you to notify you that your email is being changed to <%= @resource.reload.unconfirmed_email %>.
+We're contacting you to notify you that your email is being changed to <%= @resource.reset.unconfirmed_email %>.
 <% else %>
 We're contacting you to notify you that your email has been changed to <%= @resource.email %>.
 <% end %>

ee/app/models/concerns/visible_approvable.rb

@@ -77,9 +77,9 @@ module VisibleApprovable
   end
 
   def reset_approval_cache!
-    approvals.reload
-    approved_by_users.reload
-    approval_rules.reload
+    approvals.reset
+    approved_by_users.reset
+    approval_rules.reset
 
     clear_memoization(:approvers_left)
     clear_memoization(:all_approvers_including_groups)

ee/app/models/project_feature_usage.rb

@@ -25,7 +25,7 @@ class ProjectFeatureUsage < ApplicationRecord
       touch(self.class.jira_dvcs_integration_field(cloud: cloud))
     end
   rescue ActiveRecord::RecordNotUnique
-    reload
+    reset
     retry
   end
 end

ee/app/services/approval_rules/base_service.rb

@@ -9,7 +9,7 @@ module ApprovalRules
       filter_eligible_groups!
 
       if rule.update(params)
-        rule.reload
+        rule.reset
         success
       else
         error(rule.errors.messages)

ee/app/services/concerns/cleanup_approvers.rb

@@ -16,8 +16,8 @@ module CleanupApprovers
     # If the target already has `approvers` and/or `approver_groups` loaded then we need to
     # force a reload in order to not return stale information after the destroys above
     if reload
-      target.approvers.reload
-      target.approver_groups.reload
+      target.approvers.reset
+      target.approver_groups.reset
     end
 
     target

ee/app/services/ee/issues/move_service.rb

@@ -18,7 +18,7 @@ module EE
         return unless can?(current_user, :update_epic, epic_issue.epic.group)
 
         epic_issue.update(issue_id: new_entity.id)
-        original_entity.reload
+        original_entity.reset
       end
     end
   end

ee/app/services/epics/update_service.rb

@@ -18,7 +18,7 @@ module Epics
 
       if have_epic_dates_changed?(epic)
         epic.update_start_and_due_dates
-        epic.reload
+        epic.reset
       end
 
       epic

ee/lib/gitlab/geo.rb

@@ -48,7 +48,7 @@ module Gitlab
       # No caching of the enabled! If we cache it and an admin disables
       # this node, an active Geo::RepositorySyncWorker would keep going for up
       # to max run time after the node was disabled.
-      Gitlab::Geo.current_node.reload.enabled?
+      Gitlab::Geo.current_node.reset.enabled?
     end
 
     def self.geo_database_configured?

lib/api/pipelines.rb

@@ -137,7 +137,7 @@ module API
         pipeline.cancel_running
 
         status 200
-        present pipeline.reload, with: Entities::Pipeline
+        present pipeline.reset, with: Entities::Pipeline
       end
     end
 

lib/api/projects.rb

@@ -351,7 +351,7 @@ module API
           not_modified!
         else
           current_user.toggle_star(user_project)
-          user_project.reload
+          user_project.reset
 
           present user_project, with: Entities::Project, current_user: current_user
         end
@@ -363,7 +363,7 @@ module API
       post ':id/unstar' do
         if current_user.starred?(user_project)
           current_user.toggle_star(user_project)
-          user_project.reload
+          user_project.reset
 
           present user_project, with: Entities::Project, current_user: current_user
         else
@@ -403,7 +403,7 @@ module API
         result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project)
 
         if result
-          present user_project.reload, with: Entities::Project, current_user: current_user
+          present user_project.reset, with: Entities::Project, current_user: current_user
         else
           render_api_error!("Project already forked", 409) if user_project.forked?
         end

lib/gitlab/optimistic_locking.rb

@@ -12,7 +12,7 @@ module Gitlab
       retries -= 1
       raise unless retries >= 0
 
-      subject.reload
+      subject.reset
       retry
     end
 

lib/tasks/gitlab/update_templates.rake

@@ -50,7 +50,7 @@ namespace :gitlab do
         puts "Waiting for the import to finish"
 
         sleep(5)
-        project.reload
+        project.reset
       end
 
       Projects::ImportExport::ExportService.new(project, admin).execute

rubocop/cop/active_record_association_reload.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Cop that blacklists the use of `reload`.
+    class ActiveRecordAssociationReload < RuboCop::Cop::Cop
+      MSG = 'Use reset instead of reload. ' \
+        'For more details check the https://gitlab.com/gitlab-org/gitlab-ce/issues/60218.'
+
+      def_node_matcher :reload?, <<~PATTERN
+        (send _ :reload ...)
+      PATTERN
+
+      def on_send(node)
+        return unless reload?(node)
+
+        add_offense(node, location: :selector)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -6,6 +6,7 @@ require_relative 'cop/gitlab/finder_with_find_by'
 require_relative 'cop/gitlab/union'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/safe_params'
+require_relative 'cop/active_record_association_reload'
 require_relative 'cop/avoid_return_from_blocks'
 require_relative 'cop/avoid_break_from_strong_memoize'
 require_relative 'cop/avoid_route_redirect_leading_slash'

spec/lib/gitlab/optimistic_locking_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::OptimisticLocking do
 
   describe '#retry_lock' do
     it 'does not reload object if state changes' do
-      expect(pipeline).not_to receive(:reload)
+      expect(pipeline).not_to receive(:reset)
       expect(pipeline).to receive(:succeed).and_call_original
 
       described_class.retry_lock(pipeline) do |subject|
@@ -17,7 +17,7 @@ describe Gitlab::OptimisticLocking do
     it 'retries action if exception is raised' do
       pipeline.succeed
 
-      expect(pipeline2).to receive(:reload).and_call_original
+      expect(pipeline2).to receive(:reset).and_call_original
       expect(pipeline2).to receive(:drop).twice.and_call_original
 
       described_class.retry_lock(pipeline2) do |subject|

spec/rubocop/cop/active_record_association_reload_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop'
+require_relative '../../../rubocop/cop/active_record_association_reload'
+
+describe RuboCop::Cop::ActiveRecordAssociationReload do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when using ActiveRecord::Base' do
+    it 'registers an offense on reload usage' do
+      expect_offense(<<~PATTERN.strip_indent)
+        users = User.all
+        users.reload
+              ^^^^^^ Use reset instead of reload. For more details check the https://gitlab.com/gitlab-org/gitlab-ce/issues/60218.
+        PATTERN
+    end
+
+    it 'does not register an offense on reset usage' do
+      expect_no_offenses(<<~PATTERN.strip_indent)
+        users = User.all
+        users.reset
+      PATTERN
+    end
+  end
+
+  context 'when using ActiveRecord::Relation' do
+    it 'registers an offense on reload usage' do
+      expect_offense(<<~PATTERN.strip_indent)
+        user = User.new
+        user.reload
+             ^^^^^^ Use reset instead of reload. For more details check the https://gitlab.com/gitlab-org/gitlab-ce/issues/60218.
+      PATTERN
+    end
+
+    it 'does not register an offense on reset usage' do
+      expect_no_offenses(<<~PATTERN.strip_indent)
+        user = User.new
+        user.reset
+      PATTERN
+    end
+  end
+
+  context 'when using on self' do
+    it 'registers an offense on reload usage' do
+      expect_offense(<<~PATTERN.strip_indent)
+        reload
+        ^^^^^^ Use reset instead of reload. For more details check the https://gitlab.com/gitlab-org/gitlab-ce/issues/60218.
+      PATTERN
+    end
+
+    it 'does not register an offense on reset usage' do
+      expect_no_offenses(<<~PATTERN.strip_indent)
+        reset
+      PATTERN
+    end
+  end
+end