summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKamil Trzciński <ayufan@ayufan.eu>2019-04-08 15:33:36 +0200
committerKamil Trzciński <ayufan@ayufan.eu>2019-04-15 13:05:14 +0200
commit650f40865e5d8136cb366fbde689c4100aafb0c5 (patch)
treef740d9f235693ad8e2a3693d6ec30dee26d2a74a
parent7457c1e1229cd1e90e608e8b247e2fbb217f05b6 (diff)
downloadgitlab-ce-650f40865e5d8136cb366fbde689c4100aafb0c5.tar.gz
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.
-rw-r--r--.rubocop.yml6
-rw-r--r--app/controllers/admin/projects_controller.rb2
-rw-r--r--app/controllers/profiles/passwords_controller.rb2
-rw-r--r--app/controllers/projects/imports_controller.rb2
-rw-r--r--app/controllers/projects_controller.rb2
-rw-r--r--app/models/application_record.rb2
-rw-r--r--app/models/merge_request_diff.rb6
-rw-r--r--app/services/groups/destroy_service.rb2
-rw-r--r--app/services/notes/update_service.rb2
-rw-r--r--app/services/projects/detect_repository_languages_service.rb2
-rw-r--r--app/services/projects/move_project_group_links_service.rb2
-rw-r--r--app/services/projects/transfer_service.rb4
-rw-r--r--app/services/users/migrate_to_ghost_user_service.rb2
-rw-r--r--app/services/users/refresh_authorized_projects_service.rb4
-rw-r--r--app/views/devise/mailer/email_changed.html.haml2
-rw-r--r--app/views/devise/mailer/email_changed.text.erb2
-rw-r--r--lib/api/pipelines.rb2
-rw-r--r--lib/api/projects.rb6
-rw-r--r--lib/gitlab/optimistic_locking.rb2
-rw-r--r--lib/tasks/gitlab/update_templates.rake2
-rw-r--r--rubocop/cop/active_record_association_reload.rb21
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/lib/gitlab/optimistic_locking_spec.rb4
-rw-r--r--spec/rubocop/cop/active_record_association_reload_spec.rb60
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