From 650f40865e5d8136cb366fbde689c4100aafb0c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 8 Apr 2019 15:33:36 +0200 Subject: 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. --- .rubocop.yml | 6 +++ app/controllers/admin/projects_controller.rb | 2 +- app/controllers/profiles/passwords_controller.rb | 2 +- app/controllers/projects/imports_controller.rb | 2 +- app/controllers/projects_controller.rb | 2 +- app/models/application_record.rb | 2 + app/models/merge_request_diff.rb | 6 +-- app/services/groups/destroy_service.rb | 2 +- app/services/notes/update_service.rb | 2 +- .../detect_repository_languages_service.rb | 2 +- .../projects/move_project_group_links_service.rb | 2 +- app/services/projects/transfer_service.rb | 4 +- .../users/migrate_to_ghost_user_service.rb | 2 +- .../users/refresh_authorized_projects_service.rb | 4 +- app/views/devise/mailer/email_changed.html.haml | 2 +- app/views/devise/mailer/email_changed.text.erb | 2 +- lib/api/pipelines.rb | 2 +- lib/api/projects.rb | 6 +-- lib/gitlab/optimistic_locking.rb | 2 +- lib/tasks/gitlab/update_templates.rake | 2 +- rubocop/cop/active_record_association_reload.rb | 21 ++++++++ rubocop/rubocop.rb | 1 + spec/lib/gitlab/optimistic_locking_spec.rb | 4 +- .../cop/active_record_association_reload_spec.rb | 60 ++++++++++++++++++++++ 24 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 rubocop/cop/active_record_association_reload.rb create mode 100644 spec/rubocop/cop/active_record_association_reload_spec.rb 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 -- cgit v1.2.1