Commit 9e6f4e3b authored by Qingyu Zhao's avatar Qingyu Zhao

Add rubocop Cop to detect activerecord .count and .each in haml file

Suggest to change `@AR_relation.count; @AR_relation.each...` to
`@AR_relation.load.size; @AR_relation.each...`. This will save one
database query
parent 438d67c1
- page_title _('Deploy Keys')
%h3.page-title.deploy-keys-title
= _('Public deploy keys (%{deploy_keys_count})') % { deploy_keys_count: @deploy_keys.count }
= _('Public deploy keys (%{deploy_keys_count})') % { deploy_keys_count: @deploy_keys.load.size }
.float-right
= link_to _('New deploy key'), new_admin_deploy_key_path, class: 'btn btn-success btn-sm btn-inverted'
......
......@@ -18,7 +18,7 @@
= f.submit _('Add email address'), class: 'btn btn-success', data: { qa_selector: 'add_email_address_button' }
%hr
%h4.prepend-top-0
= _('Linked emails (%{email_count})') % { email_count: @emails.count + 1 }
= _('Linked emails (%{email_count})') % { email_count: @emails.load.size + 1 }
.account-well.append-bottom-default
%ul
%li
......
- if @related_branches.any?
%h2.related-branches-title
= pluralize(@related_branches.count, 'Related Branch')
= pluralize(@related_branches.size, 'Related Branch')
%ul.unstyled-list.related-merge-requests
- @related_branches.each do |branch|
%li
......
- verification_enabled = Gitlab::CurrentSettings.pages_domain_verification_enabled?
- if can?(current_user, :update_pages, @project) && @domains.any?
- if can?(current_user, :update_pages, @project) && @domains.load.any?
.card
.card-header
Domains (#{@domains.count})
Domains (#{@domains.size})
%ul.list-group.list-group-flush.pages-domain-list{ class: ("has-verification-status" if verification_enabled) }
- @domains.each do |domain|
- domain = Gitlab::View::Presenter::Factory.new(domain, current_user: current_user).fabricate!
......
# frozen_string_literal: true
module RuboCop
module Cop
module Performance
class ARCountEach < RuboCop::Cop::Cop
def message(ivar)
"If #{ivar} is AR relation, avoid `#{ivar}.count ...; #{ivar}.each... `, this will trigger two queries. " \
"Use `#{ivar}.load.size ...; #{ivar}.each... ` instead. If #{ivar} is an array, try to use #{ivar}.size."
end
def_node_matcher :count_match, <<~PATTERN
(send (ivar $_) :count)
PATTERN
def_node_matcher :each_match, <<~PATTERN
(send (ivar $_) :each)
PATTERN
def file_name(node)
node.location.expression.source_buffer.name
end
def in_haml_file?(node)
file_name(node).end_with?('.haml.rb')
end
def on_send(node)
return unless in_haml_file?(node)
ivar_count = count_match(node)
return unless ivar_count
node.each_ancestor(:begin) do |begin_node|
begin_node.each_descendant do |n|
ivar_each = each_match(n)
add_offense(node, location: :expression, message: message(ivar_count)) if ivar_each == ivar_count
end
end
end
end
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require_relative '../../../support/helpers/expect_offense'
require_relative '../../../../rubocop/cop/performance/ar_count_each.rb'
describe RuboCop::Cop::Performance::ARCountEach do
include CopHelper
include ExpectOffense
subject(:cop) { described_class.new }
context 'when it is not haml file' do
it 'does not flag it as an offense' do
expect(subject).to receive(:in_haml_file?).with(anything).at_least(:once).and_return(false)
expect_no_offenses <<~SOURCE
show(@users.count)
@users.each { |user| display(user) }
SOURCE
end
end
context 'when it is haml file' do
before do
expect(subject).to receive(:in_haml_file?).with(anything).at_least(:once).and_return(true)
end
context 'when the same object uses count and each' do
it 'flags it as an offense' do
expect_offense <<~SOURCE
show(@users.count)
^^^^^^^^^^^^ If @users is AR relation, avoid `@users.count ...; @users.each... `, this will trigger two queries. Use `@users.load.size ...; @users.each... ` instead. If @users is an array, try to use @users.size.
@users.each { |user| display(user) }
SOURCE
expect(cop.offenses.map(&:cop_name)).to contain_exactly('Performance/ARCountEach')
end
end
context 'when different object uses count and each' do
it 'does not flag it as an offense' do
expect_no_offenses <<~SOURCE
show(@emails.count)
@users.each { |user| display(user) }
SOURCE
end
end
context 'when just using count without each' do
it 'does not flag it as an offense' do
expect_no_offenses '@users.count'
end
end
context 'when just using each without count' do
it 'does not flag it as an offense' do
expect_no_offenses '@users.each { |user| display(user) }'
end
end
end
end
......@@ -17,7 +17,7 @@ describe 'projects/pages/show' do
assign(:project, project)
allow(view).to receive(:current_user).and_return(user)
assign(:domains, [domain])
assign(:domains, project.pages_domains)
end
describe 'validation warning' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment