Forbid the use of `#reload` and prefer `#reset`forbid-the-usage-of-reload

The `#reload` makes to load all objects into memory,
and the main purpose of `#reload` is to drop the association cache.

The `#reset` seems to solve exactly that case.

24 files changed, 116 insertions, 26 deletions

diff --git a/.rubocop.yml b/.rubocop.yml
index 648d59e8062..aa49f41ebf4 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -204,3 +204,9 @@ Style/ReturnNil:
 # nil values on the left hand side
 Performance/RegexpMatch:
   Enabled: false
+
+ActiveRecordAssociationReload:
+  Enabled: true
+  Exclude:
+    - 'spec/**/*'
+    - 'ee/spec/**/*'
diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb
index fb135d1a32c..aeff0c96b64 100644
--- a/app/controllers/admin/projects_controller.rb
+++ b/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
diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb
index a0391d677c4..7038447581c 100644
--- a/app/controllers/profiles/passwords_controller.rb
+++ b/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
diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb
index 8ee0bd26daf..4640be015de 100644
--- a/app/controllers/projects/imports_controller.rb
+++ b/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)
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 62b97fc2590..48f4d7a586d 100644
--- a/app/controllers/projects_controller.rb
+++ b/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
diff --git a/app/models/application_record.rb b/app/models/application_record.rb
index 6976185264e..9d71f250466 100644
--- a/app/models/application_record.rb
+++ b/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
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index ac8d3b98266..0b787217410 100644
--- a/app/models/merge_request_diff.rb
+++ b/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
 
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index 641111aeadc..654fe84e3dc 100644
--- a/app/services/groups/destroy_service.rb
+++ b/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
diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb
index d2052bed646..384d1dd2e50 100644
--- a/app/services/notes/update_service.rb
+++ b/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
diff --git a/app/services/projects/detect_repository_languages_service.rb b/app/services/projects/detect_repository_languages_service.rb
index b020e4d9088..d3680637217 100644
--- a/app/services/projects/detect_repository_languages_service.rb
+++ b/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
 
diff --git a/app/services/projects/move_project_group_links_service.rb b/app/services/projects/move_project_group_links_service.rb
index 36afcd0c503..cf4b291c761 100644
--- a/app/services/projects/move_project_group_links_service.rb
+++ b/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
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index 5da1e39a1fb..97ca00b2ea5 100644
--- a/app/services/projects/transfer_service.rb
+++ b/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
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index 04fd6e37501..a66b6627e40 100644
--- a/app/services/users/migrate_to_ghost_user_service.rb
+++ b/app/services/users/migrate_to_ghost_user_service.rb
@@ -33,7 +33,7 @@ module Users
         end
       end
 
-      user.reload
+      user.reset
     end
 
     private
diff --git a/app/services/users/refresh_authorized_projects_service.rb b/app/services/users/refresh_authorized_projects_service.rb
index fe5a82e23fa..4a26d2be2af 100644
--- a/app/services/users/refresh_authorized_projects_service.rb
+++ b/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
diff --git a/app/views/devise/mailer/email_changed.html.haml b/app/views/devise/mailer/email_changed.html.haml
index 5398430fdfd..3689e9c5f61 100644
--- a/app/views/devise/mailer/email_changed.html.haml
+++ b/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}.
diff --git a/app/views/devise/mailer/email_changed.text.erb b/app/views/devise/mailer/email_changed.text.erb
index 18137389e7b..69155db7246 100644
--- a/app/views/devise/mailer/email_changed.text.erb
+++ b/app/views/devise/mailer/email_changed.text.erb
@@ -1,7 +1,7 @@
 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 %>
diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb
index f29a18e94cf..667bf1ec801 100644
--- a/lib/api/pipelines.rb
+++ b/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
 
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 57336e95041..cb0106592f5 100644
--- a/lib/api/projects.rb
+++ b/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
diff --git a/lib/gitlab/optimistic_locking.rb b/lib/gitlab/optimistic_locking.rb
index ce4ba9f752b..868b2ae641a 100644
--- a/lib/gitlab/optimistic_locking.rb
+++ b/lib/gitlab/optimistic_locking.rb
@@ -12,7 +12,7 @@ module Gitlab
       retries -= 1
       raise unless retries >= 0
 
-      subject.reload
+      subject.reset
       retry
     end
 
diff --git a/lib/tasks/gitlab/update_templates.rake b/lib/tasks/gitlab/update_templates.rake
index abe10f5580e..e058e9fe069 100644
--- a/lib/tasks/gitlab/update_templates.rake
+++ b/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
diff --git a/rubocop/cop/active_record_association_reload.rb b/rubocop/cop/active_record_association_reload.rb
new file mode 100644
index 00000000000..dc241cab7d0
--- /dev/null
+++ b/rubocop/cop/active_record_association_reload.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 3e33419eb2e..50eab6f9270 100644
--- a/rubocop/rubocop.rb
+++ b/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'
diff --git a/spec/lib/gitlab/optimistic_locking_spec.rb b/spec/lib/gitlab/optimistic_locking_spec.rb
index 81f81d4f963..6fdf61ee0a7 100644
--- a/spec/lib/gitlab/optimistic_locking_spec.rb
+++ b/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|
diff --git a/spec/rubocop/cop/active_record_association_reload_spec.rb b/spec/rubocop/cop/active_record_association_reload_spec.rb
new file mode 100644
index 00000000000..69eb16a54d2
--- /dev/null
+++ b/spec/rubocop/cop/active_record_association_reload_spec.rb
@@ -0,0 +1,60 @@
+# 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