Merge branch 'rubocop-ban-usage-keys-first-and-values-first' into 'master'

Ban .keys.first and .values.first

See merge request gitlab-org/gitlab!24590

app/services/projects/container_repository/delete_tags_service.rb

@@ -42,7 +42,7 @@ module Projects
         # Deletes the dummy image
         # All created tag digests are the same since they all have the same dummy image.
         # a single delete is sufficient to remove all tags with it
-        if deleted_tags.any? && container_repository.delete_tag_by_digest(deleted_tags.values.first)
+        if deleted_tags.any? && container_repository.delete_tag_by_digest(deleted_tags.each_value.first)
           success(deleted: deleted_tags.keys)
         else
           error('could not delete tags')

ee/app/models/license.rb

@@ -282,7 +282,7 @@ class License < ApplicationRecord
   end
 
   def data_filename
-    company_name = self.licensee["Company"] || self.licensee.values.first
+    company_name = self.licensee["Company"] || self.licensee.each_value.first
     clean_company_name = company_name.gsub(/[^A-Za-z0-9]/, "")
     "#{clean_company_name}.gitlab-license"
   end

ee/app/services/packages/npm/create_package_service.rb

@@ -45,7 +45,7 @@ module Packages
 
       def version
         strong_memoize(:version) do
-          params[:versions].keys.first
+          params[:versions].each_key.first
         end
       end
 
@@ -58,7 +58,7 @@ module Packages
       end
 
       def dist_tag
-        params['dist-tags'].keys.first
+        params['dist-tags'].each_key.first
       end
 
       def package_file_name

ee/lib/audit/details.rb

@@ -31,7 +31,7 @@ module Audit
     def action_text
       action = @details.slice(*ACTIONS)
 
-      case action.keys.first
+      case action.each_key.first
       when :add
         "Added #{target_name}#{@details[:as] ? " as #{@details[:as]}" : ''}"
       when :remove
@@ -103,7 +103,7 @@ module Audit
     end
 
     def detail_value
-      @details.values.first
+      @details.each_value.first
     end
 
     def oauth_label

ee/lib/gitlab/analytics/type_of_work/tasks_by_type.rb

@@ -26,7 +26,7 @@ module Gitlab
         attr_reader :group, :params, :finder
 
         def finder_class
-          FINDER_CLASSES.fetch(params[:subject], FINDER_CLASSES.keys.first)
+          FINDER_CLASSES.fetch(params[:subject], FINDER_CLASSES.each_key.first)
         end
 
         def format_result(result)

ee/spec/features/admin/licenses/admin_uploads_license_spec.rb

@@ -58,7 +58,7 @@ describe "Admin uploads license" do
         attach_and_upload(path)
 
         expect(page).to have_content("The license was successfully uploaded and is now active.")
-                   .and have_content(license.licensee.values.first)
+                   .and have_content(license.licensee.each_value.first)
       end
     end
 

ee/spec/features/admin/licenses/admin_views_license_spec.rb

@@ -57,7 +57,7 @@ describe "Admin views license" do
     end
 
     it "shows only expiration duration" do
-      expect(page).to have_content(license.licensee.values.first)
+      expect(page).to have_content(license.licensee.each_value.first)
 
       page.within(".js-license-info-panel") do
         expect(page).not_to have_content("Expires: Free trial will expire in")
@@ -83,7 +83,7 @@ describe "Admin views license" do
         license_history = page.find("#license_history")
 
         License.previous.each do |license|
-          expect(license_history).to have_content(license.licensee.values.first)
+          expect(license_history).to have_content(license.licensee.each_value.first)
         end
       end
     end

ee/spec/services/packages/create_dependency_service_spec.rb

@@ -15,7 +15,7 @@ describe Packages::CreateDependencyService do
                 .gsub('1.0.1', version))
                 .with_indifferent_access
       end
-      let(:package_version) { params[:versions].keys.first }
+      let(:package_version) { params[:versions].each_key.first }
       let(:dependencies) { params[:versions][package_version] }
       let(:package) { create(:npm_package) }
       let(:dependency_names) { package.dependency_links.flat_map(&:dependency).map(&:name).sort }

ee/spec/workers/geo/design_repository_shard_sync_worker_spec.rb

@@ -9,7 +9,7 @@ describe Geo::DesignRepositoryShardSyncWorker, :geo, :geo_fdw, :clean_gitlab_red
   let!(:primary) { create(:geo_node, :primary) }
   let!(:secondary) { create(:geo_node) }
 
-  let(:shard_name) { Gitlab.config.repositories.storages.keys.first }
+  let(:shard_name) { Gitlab.config.repositories.storages.each_key.first }
 
   before do
     stub_current_geo_node(secondary)

ee/spec/workers/geo/repository_shard_sync_worker_spec.rb

@@ -9,7 +9,7 @@ describe Geo::RepositoryShardSyncWorker, :geo, :geo_fdw, :clean_gitlab_redis_cac
   let!(:primary) { create(:geo_node, :primary) }
   let!(:secondary) { create(:geo_node) }
 
-  let(:shard_name) { Gitlab.config.repositories.storages.keys.first }
+  let(:shard_name) { Gitlab.config.repositories.storages.each_key.first }
 
   before do
     stub_current_geo_node(secondary)

ee/spec/workers/geo/repository_verification/primary/shard_worker_spec.rb

@@ -7,7 +7,7 @@ describe Geo::RepositoryVerification::Primary::ShardWorker, :clean_gitlab_redis_
   include ExclusiveLeaseHelpers
 
   let!(:primary)   { create(:geo_node, :primary) }
-  let(:shard_name) { Gitlab.config.repositories.storages.keys.first }
+  let(:shard_name) { Gitlab.config.repositories.storages.each_key.first }
   let(:primary_singleworker) { Geo::RepositoryVerification::Primary::SingleWorker }
 
   before do

ee/spec/workers/geo/repository_verification/secondary/shard_worker_spec.rb

@@ -7,7 +7,7 @@ describe Geo::RepositoryVerification::Secondary::ShardWorker, :geo, :geo_fdw, :r
   include ExclusiveLeaseHelpers
 
   let!(:secondary) { create(:geo_node) }
-  let(:shard_name) { Gitlab.config.repositories.storages.keys.first }
+  let(:shard_name) { Gitlab.config.repositories.storages.each_key.first }
   let(:secondary_single_worker) { Geo::RepositoryVerification::Secondary::SingleWorker }
   let!(:project) { create(:project) }
 

ee/spec/workers/geo/secondary/repository_backfill_worker_spec.rb

@@ -7,7 +7,7 @@ describe Geo::Secondary::RepositoryBackfillWorker, :geo, :geo_fdw, :clean_gitlab
 
   let(:primary) { create(:geo_node, :primary) }
   let(:secondary) { create(:geo_node, repos_max_capacity: 5) }
-  let(:shard_name) { Gitlab.config.repositories.storages.keys.first }
+  let(:shard_name) { Gitlab.config.repositories.storages.each_key.first }
 
   before do
     stub_current_geo_node(secondary)

rubocop/cop/gitlab/keys-first-and-values-first.rb

+module RuboCop
+  module Cop
+    module Gitlab
+      class KeysFirstAndValuesFirst < RuboCop::Cop::Cop
+        FIRST_PATTERN = /\Afirst\z/.freeze
+
+        def message(used_method)
+          <<~MSG
+          Don't use `.keys.first` and `.values.first`.
+          Instead use `.each_key.first` and `.each_value.first` (or `.first.first` and `first.second`)
+
+          This will reduce memory usage and execution time.
+          MSG
+        end
+
+        def on_send(node)
+          if find_on_keys_or_values?(node)
+            add_offense(node, location: :selector, message: message(node.method_name))
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            replace_with = if node.descendants.first.method_name == :values
+                             '.each_value'
+                           elsif node.descendants.first.method_name == :keys
+                             '.each_key'
+                           else
+                             throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first")
+                           end
+
+            upto_including_keys_or_values = node.descendants.first.source_range
+            before_keys_or_values = node.descendants[1].source_range
+            range_to_replace = node.source_range
+                                  .with(begin_pos: before_keys_or_values.end_pos,
+                                        end_pos: upto_including_keys_or_values.end_pos)
+            corrector.replace(range_to_replace, replace_with)
+          end
+        end
+
+        def find_on_keys_or_values?(node)
+          chained_on_node = node.descendants.first
+          node.method_name.to_s =~ FIRST_PATTERN &&
+              chained_on_node.is_a?(RuboCop::AST::SendNode) &&
+              [:keys, :values].include?(chained_on_node.method_name) &&
+              node.descendants[1]
+        end
+
+        def method_name_for_node(node)
+          children[1].to_s
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -5,6 +5,7 @@ require_relative 'cop/gitlab/httparty'
 require_relative 'cop/gitlab/finder_with_find_by'
 require_relative 'cop/gitlab/union'
 require_relative 'cop/gitlab/rails_logger'
+require_relative 'cop/gitlab/keys-first-and-values-first'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/safe_params'
 require_relative 'cop/active_record_association_reload'

spec/controllers/user_callouts_controller_spec.rb

@@ -13,7 +13,7 @@ describe UserCalloutsController do
     subject { post :create, params: { feature_name: feature_name }, format: :json }
 
     context 'with valid feature name' do
-      let(:feature_name) { UserCallout.feature_names.keys.first }
+      let(:feature_name) { UserCallout.feature_names.each_key.first }
 
       context 'when callout entry does not exist' do
         it 'creates a callout entry with dismissed state' do
@@ -28,7 +28,7 @@ describe UserCalloutsController do
       end
 
       context 'when callout entry already exists' do
-        let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.keys.first, user: user) }
+        let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.each_key.first, user: user) }
 
         it 'returns success' do
           subject

spec/lib/gitlab/import_export/hash_util_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::ImportExport::HashUtil do
   describe '.deep_symbolize_array!' do
     it 'symbolizes keys' do
       expect { described_class.deep_symbolize_array!(stringified_array) }.to change {
-        stringified_array.first.keys.first
+        stringified_array.first.each_key.first
       }.from('test').to(:test)
     end
   end
@@ -17,13 +17,13 @@ describe Gitlab::ImportExport::HashUtil do
   describe '.deep_symbolize_array_with_date!' do
     it 'symbolizes keys' do
       expect { described_class.deep_symbolize_array_with_date!(stringified_array_with_date) }.to change {
-        stringified_array_with_date.first.keys.first
+        stringified_array_with_date.first.each_key.first
       }.from('test_date').to(:test_date)
     end
 
     it 'transforms date strings into Time objects' do
       expect { described_class.deep_symbolize_array_with_date!(stringified_array_with_date) }.to change {
-        stringified_array_with_date.first.values.first.class
+        stringified_array_with_date.first.each_value.first.class
       }.from(String).to(ActiveSupport::TimeWithZone)
     end
   end

spec/lib/gitlab/shell_spec.rb

@@ -397,7 +397,7 @@ describe Gitlab::Shell do
   describe 'namespace actions' do
     subject { described_class.new }
 
-    let(:storage) { Gitlab.config.repositories.storages.keys.first }
+    let(:storage) { Gitlab.config.repositories.storages.each_key.first }
 
     describe '#add_namespace' do
       it 'creates a namespace' do

spec/models/concerns/redis_cacheable_spec.rb

@@ -28,7 +28,7 @@ describe RedisCacheable do
   end
 
   describe '#cached_attribute' do
-    subject { instance.cached_attribute(payload.keys.first) }
+    subject { instance.cached_attribute(payload.each_key.first) }
 
     it 'gets the cache attribute' do
       Gitlab::Redis::SharedState.with do |redis|
@@ -36,7 +36,7 @@ describe RedisCacheable do
           .and_return(payload.to_json)
       end
 
-      expect(subject).to eq(payload.values.first)
+      expect(subject).to eq(payload.each_value.first)
     end
   end
 

spec/models/repository_spec.rb

@@ -372,7 +372,7 @@ describe Repository do
 
     context 'when some commits are not found ' do
       let(:oids) do
-        ['deadbeef'] + TestEnv::BRANCH_SHA.values.first(10)
+        ['deadbeef'] + TestEnv::BRANCH_SHA.each_value.first(10)
       end
 
       it 'returns only found commits' do

spec/models/user_spec.rb

@@ -4160,7 +4160,7 @@ describe User, :do_not_mock_admin_mode do
   describe '#dismissed_callout?' do
     subject(:user) { create(:user) }
 
-    let(:feature_name) { UserCallout.feature_names.keys.first }
+    let(:feature_name) { UserCallout.feature_names.each_key.first }
 
     context 'when no callout dismissal record exists' do
       it 'returns false when no ignore_dismissal_earlier_than provided' do

spec/serializers/test_suite_entity_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe TestSuiteEntity do
   let(:pipeline) { create(:ci_pipeline, :with_test_reports) }
-  let(:entity) { described_class.new(pipeline.test_reports.test_suites.values.first) }
+  let(:entity) { described_class.new(pipeline.test_reports.test_suites.each_value.first) }
 
   describe '#as_json' do
     subject(:as_json) { entity.as_json }