root
/
gitlab-ce
mirror of https://gitlab.com/free-releases/gitlab-ce.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

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

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.
This commit is contained in:
Kamil Trzciński 2019-04-08 15:33:36 +02:00
parent 7457c1e122
commit 650f40865e
24 changed files with 116 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
.rubocop.yml
View File

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

2
app/controllers/admin/projects_controller.rb
View File

@ -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

2
app/controllers/profiles/passwords_controller.rb
View File

@ -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

2
app/controllers/projects/imports_controller.rb
View File

@ -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)

2
app/controllers/projects_controller.rb
View File

@ -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

2
app/models/application_record.rb
View File

@ -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

6
app/models/merge_request_diff.rb
View File

@ -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

2
app/services/groups/destroy_service.rb
View File

@ -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

2
app/services/notes/update_service.rb
View File

@ -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

2
app/services/projects/detect_repository_languages_service.rb
View File

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

2
app/services/projects/move_project_group_links_service.rb
View File

@ -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

4
app/services/projects/transfer_service.rb
View File

@ -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

2
app/services/users/migrate_to_ghost_user_service.rb
View File

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

4
app/services/users/refresh_authorized_projects_service.rb
View File

@ -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

2
app/views/devise/mailer/email_changed.html.haml
View File

@ -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}.

2
app/views/devise/mailer/email_changed.text.erb
View File

@ -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 %>

2
lib/api/pipelines.rb
View File

@ -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

6
lib/api/projects.rb
View File

@ -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

2
lib/gitlab/optimistic_locking.rb
View File

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

2
lib/tasks/gitlab/update_templates.rake
View File

@ -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

21
rubocop/cop/active_record_association_reload.rb Normal file
View File

@ -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

1
rubocop/rubocop.rb
View File

@ -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'

4
spec/lib/gitlab/optimistic_locking_spec.rb
View File

@ -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|

60
spec/rubocop/cop/active_record_association_reload_spec.rb Normal file
View File

@ -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