diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-19 12:35:24 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-19 12:35:24 +0300 |
commit | 1d20d43661f3e98bde2bb5dc163a45fd90f8ac88 (patch) | |
tree | 53feb6983348a364ed4f52a4108866bf4ff0a599 /rubocop | |
parent | 2774ddc308f96f49a0f26871ff544681229f4eee (diff) |
Add latest changes from gitlab-org/gitlab@12-9-stable-ee
Diffstat (limited to 'rubocop')
-rw-r--r-- | rubocop/cop/ban_catch_throw.rb | 34 | ||||
-rw-r--r-- | rubocop/cop/gitlab/keys-first-and-values-first.rb | 2 | ||||
-rw-r--r-- | rubocop/cop/migration/add_column_with_default.rb | 44 | ||||
-rw-r--r-- | rubocop/cop/migration/schedule_async.rb | 54 | ||||
-rw-r--r-- | rubocop/cop/migration/with_lock_retries_with_change.rb | 30 | ||||
-rw-r--r-- | rubocop/cop/scalability/idempotent_worker.rb | 59 | ||||
-rw-r--r-- | rubocop/migration_helpers.rb | 4 |
7 files changed, 220 insertions, 7 deletions
diff --git a/rubocop/cop/ban_catch_throw.rb b/rubocop/cop/ban_catch_throw.rb new file mode 100644 index 00000000000..42301d5512f --- /dev/null +++ b/rubocop/cop/ban_catch_throw.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Bans the use of 'catch/throw', as exceptions are better for errors and + # they are equivalent to 'goto' for flow control, with all the problems + # that implies. + # + # @example + # # bad + # catch(:error) do + # throw(:error) + # end + # + # # good + # begin + # raise StandardError + # rescue StandardError => err + # # ... + # end + # + class BanCatchThrow < RuboCop::Cop::Cop + MSG = "Do not use catch or throw unless a gem's API demands it." + + def on_send(node) + receiver, method_name, _ = *node + + return unless receiver.nil? && %i[catch throw].include?(method_name) + + add_offense(node, location: :expression) + end + end + end +end diff --git a/rubocop/cop/gitlab/keys-first-and-values-first.rb b/rubocop/cop/gitlab/keys-first-and-values-first.rb index 9b68957cba2..544f9800304 100644 --- a/rubocop/cop/gitlab/keys-first-and-values-first.rb +++ b/rubocop/cop/gitlab/keys-first-and-values-first.rb @@ -26,7 +26,7 @@ module RuboCop 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") + throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") # rubocop:disable Cop/BanCatchThrow end upto_including_keys_or_values = node.descendants.first.source_range diff --git a/rubocop/cop/migration/add_column_with_default.rb b/rubocop/cop/migration/add_column_with_default.rb index d9f8fe62a86..68e53b17f19 100644 --- a/rubocop/cop/migration/add_column_with_default.rb +++ b/rubocop/cop/migration/add_column_with_default.rb @@ -10,7 +10,37 @@ module RuboCop class AddColumnWithDefault < RuboCop::Cop::Cop include MigrationHelpers - WHITELISTED_TABLES = [:application_settings].freeze + # Tables >= 10 GB on GitLab.com as of 02/2020 + BLACKLISTED_TABLES = %i[ + audit_events + ci_build_trace_sections + ci_builds + ci_builds_metadata + ci_job_artifacts + ci_pipeline_variables + ci_pipelines + ci_stages + deployments + events + issues + merge_request_diff_commits + merge_request_diff_files + merge_request_diffs + merge_request_metrics + merge_requests + note_diff_files + notes + project_authorizations + projects + push_event_payloads + resource_label_events + sent_notifications + system_note_metadata + taggings + todos + users + web_hook_logs + ].freeze MSG = '`add_column_with_default` without `allow_null: true` may cause prolonged lock situations and downtime, ' \ 'see https://gitlab.com/gitlab-org/gitlab/issues/38060'.freeze @@ -23,21 +53,23 @@ module RuboCop return unless in_migration?(node) add_column_with_default?(node) do |table, options| - break if table_whitelisted?(table) || nulls_allowed?(options) - - add_offense(node, location: :selector) + add_offense(node, location: :selector) if offensive?(table, options) end end private + def offensive?(table, options) + table_blacklisted?(table) && !nulls_allowed?(options) + end + def nulls_allowed?(options) options.find { |opt| opt.key.value == :allow_null && opt.value.true_type? } end - def table_whitelisted?(symbol) + def table_blacklisted?(symbol) symbol && symbol.type == :sym && - WHITELISTED_TABLES.include?(symbol.children[0]) + BLACKLISTED_TABLES.include?(symbol.children[0]) end end end diff --git a/rubocop/cop/migration/schedule_async.rb b/rubocop/cop/migration/schedule_async.rb new file mode 100644 index 00000000000..f296628c3d6 --- /dev/null +++ b/rubocop/cop/migration/schedule_async.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + class ScheduleAsync < RuboCop::Cop::Cop + include MigrationHelpers + + ENFORCED_SINCE = 2020_02_12_00_00_00 + + MSG = <<~MSG + Don't call the background migration worker directly, use the `#migrate_async`, + `#migrate_in`, `#bulk_migrate_async` or `#bulk_migrate_in` migration helpers + instead. + MSG + + def_node_matcher :calls_background_migration_worker?, <<~PATTERN + (send (const nil? :BackgroundMigrationWorker) {:perform_async :perform_in :bulk_perform_async :bulk_perform_in} ... ) + PATTERN + + def on_send(node) + return unless in_migration?(node) + return if version(node) < ENFORCED_SINCE + + add_offense(node, location: :expression) if calls_background_migration_worker?(node) + end + + def autocorrect(node) + # This gets rid of the receiver `BackgroundMigrationWorker` and + # replaces `perform` with `schedule` + schedule_method = method_name(node).to_s.sub('perform', 'migrate') + arguments = arguments(node).map(&:source).join(', ') + + replacement = "#{schedule_method}(#{arguments})" + lambda do |corrector| + corrector.replace(node.source_range, replacement) + end + end + + private + + def method_name(node) + node.children.second + end + + def arguments(node) + node.children[2..-1] + end + end + end + end +end diff --git a/rubocop/cop/migration/with_lock_retries_with_change.rb b/rubocop/cop/migration/with_lock_retries_with_change.rb new file mode 100644 index 00000000000..36fc1f92833 --- /dev/null +++ b/rubocop/cop/migration/with_lock_retries_with_change.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that prevents usage of `with_lock_retries` within the `change` method. + class WithLockRetriesWithChange < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = '`with_lock_retries` cannot be used within `change` so you must manually define ' \ + 'the `up` and `down` methods in your migration class and use `with_lock_retries` in both methods'.freeze + + def on_send(node) + return unless in_migration?(node) + return unless node.children[1] == :with_lock_retries + + node.each_ancestor(:def) do |def_node| + add_offense(def_node, location: :name) if method_name(def_node) == :change + end + end + + def method_name(node) + node.children.first + end + end + end + end +end diff --git a/rubocop/cop/scalability/idempotent_worker.rb b/rubocop/cop/scalability/idempotent_worker.rb new file mode 100644 index 00000000000..a38b457b7c7 --- /dev/null +++ b/rubocop/cop/scalability/idempotent_worker.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers.rb' + +module RuboCop + module Cop + module Scalability + # This cop checks for the `idempotent!` call in the worker scope. + # + # @example + # + # # bad + # class TroubleMakerWorker + # def perform + # end + # end + # + # # good + # class NiceWorker + # idempotent! + # + # def perform + # end + # end + # + class IdempotentWorker < RuboCop::Cop::Cop + include CodeReuseHelpers + + HELP_LINK = 'https://github.com/mperham/sidekiq/wiki/Best-Practices#2-make-your-job-idempotent-and-transactional' + + MSG = <<~MSG + Avoid adding not idempotent workers. + + A worker is considered idempotent if: + + 1. It can safely run multiple times with the same arguments + 2. The application side-effects are expected to happen once (or side-effects of a second run are not impactful) + 3. It can safely be skipped if another job with the same arguments is already in the queue + + If all the above is true, make sure to mark it as so by calling the `idempotent!` + method in the worker scope. + + See #{HELP_LINK} + MSG + + def_node_search :idempotent?, <<~PATTERN + (send nil? :idempotent!) + PATTERN + + def on_class(node) + return unless in_worker?(node) + return if idempotent?(node) + + add_offense(node, location: :expression) + end + end + end + end +end diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb index 577f768da67..767cacaecaf 100644 --- a/rubocop/migration_helpers.rb +++ b/rubocop/migration_helpers.rb @@ -10,6 +10,10 @@ module RuboCop dirname(node).end_with?('db/post_migrate', 'db/geo/post_migrate') end + def version(node) + File.basename(node.location.expression.source_buffer.name).split('_').first.to_i + end + private def dirname(node) |