diff options
Diffstat (limited to 'rubocop/cop')
| -rw-r--r-- | rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers.rb | 20 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/change_timzone.rb | 20 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/json.rb | 36 | ||||
| -rw-r--r-- | rubocop/cop/inject_enterprise_edition_module.rb | 10 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_column.rb | 50 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_column_with_default.rb | 28 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_columns_to_wide_tables.rb | 1 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_concurrent_foreign_key.rb | 14 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_limit_to_string_columns.rb | 59 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_limit_to_text_columns.rb | 121 | ||||
| -rw-r--r-- | rubocop/cop/migration/prevent_strings.rb | 52 | ||||
| -rw-r--r-- | rubocop/cop/migration/reversible_add_column_with_default.rb | 35 | ||||
| -rw-r--r-- | rubocop/cop/migration/with_lock_retries_disallowed_method.rb | 58 | ||||
| -rw-r--r-- | rubocop/cop/migration/with_lock_retries_without_ddl_transaction.rb | 36 | ||||
| -rw-r--r-- | rubocop/cop/performance/ar_exists_and_present_blank.rb | 57 | ||||
| -rw-r--r-- | rubocop/cop/rspec/empty_line_after_shared_example.rb | 64 |
16 files changed, 453 insertions, 208 deletions
diff --git a/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers.rb b/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers.rb new file mode 100644 index 00000000000..eba38c1630f --- /dev/null +++ b/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers.rb @@ -0,0 +1,20 @@ +module RuboCop + module Cop + # Cop that blacklists keyword arguments usage in Sidekiq workers + class AvoidKeywordArgumentsInSidekiqWorkers < RuboCop::Cop::Cop + MSG = "Do not use keyword arguments in Sidekiq workers. " \ + "For details, check https://github.com/mperham/sidekiq/issues/2372".freeze + OBSERVED_METHOD = :perform + + def on_def(node) + return if node.method_name != OBSERVED_METHOD + + node.arguments.each do |argument| + if argument.type == :kwarg || argument.type == :kwoptarg + add_offense(node, location: :expression) + end + end + end + end + end +end diff --git a/rubocop/cop/gitlab/change_timzone.rb b/rubocop/cop/gitlab/change_timzone.rb new file mode 100644 index 00000000000..63e6dd411f3 --- /dev/null +++ b/rubocop/cop/gitlab/change_timzone.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + class ChangeTimezone < RuboCop::Cop::Cop + MSG = "Do not change timezone in the runtime (application or rspec), " \ + "it could result in silently modifying other behavior.".freeze + + def_node_matcher :changing_timezone?, <<~PATTERN + (send (const nil? :Time) :zone= ...) + PATTERN + + def on_send(node) + changing_timezone?(node) { add_offense(node) } + end + end + end + end +end diff --git a/rubocop/cop/gitlab/json.rb b/rubocop/cop/gitlab/json.rb new file mode 100644 index 00000000000..8c9027223aa --- /dev/null +++ b/rubocop/cop/gitlab/json.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + class Json < RuboCop::Cop::Cop + MSG_SEND = <<~EOL.freeze + Avoid calling `JSON` directly. Instead, use the `Gitlab::Json` + wrapper. This allows us to alter the JSON parser being used. + EOL + + def_node_matcher :json_node?, <<~PATTERN + (send (const nil? :JSON)...) + PATTERN + + def on_send(node) + add_offense(node, location: :expression, message: MSG_SEND) if json_node?(node) + end + + def autocorrect(node) + autocorrect_json_node(node) + end + + def autocorrect_json_node(node) + _, method_name, *arg_nodes = *node + + replacement = "Gitlab::Json.#{method_name}(#{arg_nodes.map(&:source).join(', ')})" + + lambda do |corrector| + corrector.replace(node.source_range, replacement) + end + end + end + end + end +end diff --git a/rubocop/cop/inject_enterprise_edition_module.rb b/rubocop/cop/inject_enterprise_edition_module.rb index 6f007e667f2..7edce94eaee 100644 --- a/rubocop/cop/inject_enterprise_edition_module.rb +++ b/rubocop/cop/inject_enterprise_edition_module.rb @@ -17,6 +17,8 @@ module RuboCop CHECK_LINE_METHODS = Set.new(%i[include_if_ee extend_if_ee prepend_if_ee]).freeze + CHECK_LINE_METHODS_REGEXP = Regexp.union(CHECK_LINE_METHODS.map(&:to_s)).freeze + DISALLOW_METHODS = Set.new(%i[include extend prepend]).freeze def ee_const?(node) @@ -48,7 +50,13 @@ module RuboCop # the expression is the last line _of code_. last_line -= 1 if buffer.source.end_with?("\n") - add_offense(node, message: INVALID_LINE) if line < last_line + last_line_content = buffer.source.split("\n")[-1] + + if CHECK_LINE_METHODS_REGEXP.match?(last_line_content) + ignore_node(node) + elsif line < last_line + add_offense(node, message: INVALID_LINE) + end end def verify_argument_type(node) diff --git a/rubocop/cop/migration/add_column.rb b/rubocop/cop/migration/add_column.rb deleted file mode 100644 index 0af90fb7fd1..00000000000 --- a/rubocop/cop/migration/add_column.rb +++ /dev/null @@ -1,50 +0,0 @@ -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # Cop that checks if columns are added in a way that doesn't require - # downtime. - class AddColumn < RuboCop::Cop::Cop - include MigrationHelpers - - MSG = '`add_column` with a default value requires downtime, ' \ - 'use `add_column_with_default` instead'.freeze - - def on_send(node) - return unless in_migration?(node) - - name = node.children[1] - - return unless name == :add_column - - # Ignore whitelisted tables. - return if table_whitelisted?(node.children[2]) - - opts = node.children.last - - return unless opts && opts.type == :hash - - opts.each_node(:pair) do |pair| - if hash_key_type(pair) == :sym && hash_key_name(pair) == :default - add_offense(node, location: :selector) - end - end - end - - def table_whitelisted?(symbol) - symbol && symbol.type == :sym && - WHITELISTED_TABLES.include?(symbol.children[0]) - end - - def hash_key_type(pair) - pair.children[0].type - end - - def hash_key_name(pair) - pair.children[0].children[0] - end - end - end - end -end diff --git a/rubocop/cop/migration/add_column_with_default.rb b/rubocop/cop/migration/add_column_with_default.rb index 383653ef5a5..355319b0dfe 100644 --- a/rubocop/cop/migration/add_column_with_default.rb +++ b/rubocop/cop/migration/add_column_with_default.rb @@ -5,39 +5,17 @@ require_relative '../../migration_helpers' module RuboCop module Cop module Migration - # Cop that checks if columns are added in a way that doesn't require - # downtime. class AddColumnWithDefault < RuboCop::Cop::Cop include MigrationHelpers - 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 - - def_node_matcher :add_column_with_default?, <<~PATTERN - (send _ :add_column_with_default $_ ... (hash $...)) - PATTERN + MSG = '`add_column_with_default` is deprecated, use `add_column` instead'.freeze def on_send(node) return unless in_migration?(node) - add_column_with_default?(node) do |table, options| - 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 + name = node.children[1] - def table_blacklisted?(symbol) - symbol && symbol.type == :sym && - BLACKLISTED_TABLES.include?(symbol.children[0]) + add_offense(node, location: :selector) if name == :add_column_with_default end end end diff --git a/rubocop/cop/migration/add_columns_to_wide_tables.rb b/rubocop/cop/migration/add_columns_to_wide_tables.rb index 4618e4ae890..2880783dc3e 100644 --- a/rubocop/cop/migration/add_columns_to_wide_tables.rb +++ b/rubocop/cop/migration/add_columns_to_wide_tables.rb @@ -14,7 +14,6 @@ module RuboCop BLACKLISTED_METHODS = %i[ add_column - add_column_with_default add_reference add_timestamps_with_timezone ].freeze diff --git a/rubocop/cop/migration/add_concurrent_foreign_key.rb b/rubocop/cop/migration/add_concurrent_foreign_key.rb index d78c7b9b043..236de6224a4 100644 --- a/rubocop/cop/migration/add_concurrent_foreign_key.rb +++ b/rubocop/cop/migration/add_concurrent_foreign_key.rb @@ -10,17 +10,29 @@ module RuboCop MSG = '`add_foreign_key` requires downtime, use `add_concurrent_foreign_key` instead'.freeze + def_node_matcher :false_node?, <<~PATTERN + (false) + PATTERN + def on_send(node) return unless in_migration?(node) name = node.children[1] - add_offense(node, location: :selector) if name == :add_foreign_key + if name == :add_foreign_key && !not_valid_fk?(node) + add_offense(node, location: :selector) + end end def method_name(node) node.children.first end + + def not_valid_fk?(node) + node.each_node(:pair).any? do |pair| + pair.children[0].children[0] == :validate && false_node?(pair.children[1]) + end + end end end end diff --git a/rubocop/cop/migration/add_limit_to_string_columns.rb b/rubocop/cop/migration/add_limit_to_string_columns.rb deleted file mode 100644 index 30affcbb089..00000000000 --- a/rubocop/cop/migration/add_limit_to_string_columns.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # Cop that enforces length constraints to string columns - class AddLimitToStringColumns < RuboCop::Cop::Cop - include MigrationHelpers - - ADD_COLUMNS_METHODS = %i(add_column add_column_with_default).freeze - - MSG = 'String columns should have a limit constraint. 255 is suggested'.freeze - - def on_def(node) - return unless in_migration?(node) - - node.each_descendant(:send) do |send_node| - next unless string_operation?(send_node) - - add_offense(send_node, location: :selector) unless limit_on_string_column?(send_node) - end - end - - private - - def string_operation?(node) - modifier = node.children[0] - migration_method = node.children[1] - - if migration_method == :string - modifier.type == :lvar - elsif ADD_COLUMNS_METHODS.include?(migration_method) - modifier.nil? && string_column?(node.children[4]) - end - end - - def string_column?(column_type) - column_type.type == :sym && column_type.value == :string - end - - def limit_on_string_column?(node) - migration_method = node.children[1] - - if migration_method == :string - limit_present?(node.children) - elsif ADD_COLUMNS_METHODS.include?(migration_method) - limit_present?(node) - end - end - - def limit_present?(statement) - !(statement.to_s =~ /:limit/).nil? - end - end - end - end -end diff --git a/rubocop/cop/migration/add_limit_to_text_columns.rb b/rubocop/cop/migration/add_limit_to_text_columns.rb new file mode 100644 index 00000000000..15c28bb9266 --- /dev/null +++ b/rubocop/cop/migration/add_limit_to_text_columns.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that enforces always adding a limit on text columns + class AddLimitToTextColumns < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Text columns should always have a limit set (255 is suggested). ' \ + 'You can add a limit to a `text` column by using `add_text_limit`'.freeze + + def_node_matcher :reverting?, <<~PATTERN + (def :down ...) + PATTERN + + def_node_matcher :add_text_limit?, <<~PATTERN + (send _ :add_text_limit ...) + PATTERN + + def on_def(node) + return unless in_migration?(node) + + # Don't enforce the rule when on down to keep consistency with existing schema + return if reverting?(node) + + node.each_descendant(:send) do |send_node| + next unless text_operation?(send_node) + + # We require a limit for the same table and attribute name + if text_limit_missing?(node, *table_and_attribute_name(send_node)) + add_offense(send_node, location: :selector) + end + end + end + + private + + def text_operation?(node) + modifier = node.children[0] + migration_method = node.children[1] + + if migration_method == :text + modifier.type == :lvar + elsif ADD_COLUMN_METHODS.include?(migration_method) + modifier.nil? && text_column?(node.children[4]) + end + end + + def text_column?(column_type) + column_type.type == :sym && column_type.value == :text + end + + # For a given node, find the table and attribute this node is for + # + # Simple when we have calls to `add_column_XXX` helper methods + # + # A little bit more tricky when we have attributes defined as part of + # a create/change table block: + # - The attribute name is available on the node + # - Finding the table name requires to: + # * go up + # * find the first block the attribute def is part of + # * go back down to find the create_table node + # * fetch the table name from that node + def table_and_attribute_name(node) + migration_method = node.children[1] + table_name, attribute_name = '' + + if migration_method == :text + # We are inside a node in a create/change table block + block_node = node.each_ancestor(:block).first + create_table_node = block_node + .children + .find { |n| TABLE_METHODS.include?(n.children[1])} + + if create_table_node + table_name = create_table_node.children[2].value + else + # Guard against errors when a new table create/change migration + # helper is introduced and warn the author so that it can be + # added in TABLE_METHODS + table_name = 'unknown' + add_offense(block_node, message: 'Unknown table create/change helper') + end + + attribute_name = node.children[2].value + else + # We are in a node for one of the ADD_COLUMN_METHODS + table_name = node.children[2].value + attribute_name = node.children[3].value + end + + [table_name, attribute_name] + end + + # Check if there is an `add_text_limit` call for the provided + # table and attribute name + def text_limit_missing?(node, table_name, attribute_name) + limit_found = false + + node.each_descendant(:send) do |send_node| + next unless add_text_limit?(send_node) + + limit_table = send_node.children[2].value + limit_attribute = send_node.children[3].value + + if limit_table == table_name && limit_attribute == attribute_name + limit_found = true + break + end + end + + !limit_found + end + end + end + end +end diff --git a/rubocop/cop/migration/prevent_strings.rb b/rubocop/cop/migration/prevent_strings.rb new file mode 100644 index 00000000000..25c00194698 --- /dev/null +++ b/rubocop/cop/migration/prevent_strings.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that enforces using text instead of the string data type + class PreventStrings < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Do not use the `string` data type, use `text` instead. ' \ + 'Updating limits on strings requires downtime. This can be avoided ' \ + 'by using `text` and adding a limit with `add_text_limit`'.freeze + + def_node_matcher :reverting?, <<~PATTERN + (def :down ...) + PATTERN + + def on_def(node) + return unless in_migration?(node) + + # Don't enforce the rule when on down to keep consistency with existing schema + return if reverting?(node) + + node.each_descendant(:send) do |send_node| + next unless string_operation?(send_node) + + add_offense(send_node, location: :selector) + end + end + + private + + def string_operation?(node) + modifier = node.children[0] + migration_method = node.children[1] + + if migration_method == :string + modifier.type == :lvar + elsif ADD_COLUMN_METHODS.include?(migration_method) + modifier.nil? && string_column?(node.children[4]) + end + end + + def string_column?(column_type) + column_type.type == :sym && column_type.value == :string + end + end + end + end +end diff --git a/rubocop/cop/migration/reversible_add_column_with_default.rb b/rubocop/cop/migration/reversible_add_column_with_default.rb deleted file mode 100644 index dd49188defa..00000000000 --- a/rubocop/cop/migration/reversible_add_column_with_default.rb +++ /dev/null @@ -1,35 +0,0 @@ -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # Cop that checks if `add_column_with_default` is used with `up`/`down` methods - # and not `change`. - class ReversibleAddColumnWithDefault < RuboCop::Cop::Cop - include MigrationHelpers - - def_node_matcher :add_column_with_default?, <<~PATTERN - (send nil? :add_column_with_default $...) - PATTERN - - def_node_matcher :defines_change?, <<~PATTERN - (def :change ...) - PATTERN - - MSG = '`add_column_with_default` is not reversible so you must manually define ' \ - 'the `up` and `down` methods in your migration class, using `remove_column` in `down`'.freeze - - def on_send(node) - return unless in_migration?(node) - return unless add_column_with_default?(node) - - node.each_ancestor(:def) do |def_node| - next unless defines_change?(def_node) - - add_offense(def_node, location: :name) - end - end - end - end - end -end diff --git a/rubocop/cop/migration/with_lock_retries_disallowed_method.rb b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb new file mode 100644 index 00000000000..309ddcc9746 --- /dev/null +++ b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + class WithLockRetriesDisallowedMethod < RuboCop::Cop::Cop + include MigrationHelpers + + ALLOWED_MIGRATION_METHODS = %i[ + create_table + drop_table + add_foreign_key + remove_foreign_key + add_column + remove_column + execute + change_column_default + remove_foreign_key_if_exists + remove_foreign_key_without_error + table_exists? + index_exists_by_name? + foreign_key_exists? + index_exists? + column_exists? + ].sort.freeze + + MSG = "The method is not allowed to be called within the `with_lock_retries` block, the only allowed methods are: #{ALLOWED_MIGRATION_METHODS.join(', ')}" + + def_node_matcher :send_node?, <<~PATTERN + send + PATTERN + + def on_block(node) + block_body = node.body + + return unless in_migration?(node) + return unless block_body + return unless node.method_name == :with_lock_retries + + if send_node?(block_body) + check_node(block_body) + else + block_body.children.each { |n| check_node(n) } + end + end + + def check_node(node) + return unless send_node?(node) + + name = node.children[1] + add_offense(node, location: :expression) unless ALLOWED_MIGRATION_METHODS.include?(name) + end + end + end + end +end diff --git a/rubocop/cop/migration/with_lock_retries_without_ddl_transaction.rb b/rubocop/cop/migration/with_lock_retries_without_ddl_transaction.rb deleted file mode 100644 index ebd91dd5a6e..00000000000 --- a/rubocop/cop/migration/with_lock_retries_without_ddl_transaction.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # Cop that prevents usage of `with_lock_retries` with `disable_ddl_transaction!` - class WithLockRetriesWithoutDdlTransaction < RuboCop::Cop::Cop - include MigrationHelpers - - MSG = '`with_lock_retries` cannot be used with disabled DDL transactions (`disable_ddl_transaction!`). ' \ - 'Please remove the `disable_ddl_transaction!` call from your migration.'.freeze - - def_node_matcher :disable_ddl_transaction?, <<~PATTERN - (send _ :disable_ddl_transaction!) - PATTERN - - def_node_matcher :with_lock_retries?, <<~PATTERN - (send _ :with_lock_retries) - PATTERN - - def on_send(node) - return unless in_migration?(node) - return unless with_lock_retries?(node) - - node.each_ancestor(:begin) do |begin_node| - disable_ddl_transaction_node = begin_node.children.find { |n| disable_ddl_transaction?(n) } - - add_offense(node, location: :expression) if disable_ddl_transaction_node - end - end - end - end - end -end diff --git a/rubocop/cop/performance/ar_exists_and_present_blank.rb b/rubocop/cop/performance/ar_exists_and_present_blank.rb new file mode 100644 index 00000000000..54c2d6bf95a --- /dev/null +++ b/rubocop/cop/performance/ar_exists_and_present_blank.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + class ARExistsAndPresentBlank < RuboCop::Cop::Cop + def message_present(ivar) + "Avoid `#{ivar}.present?`, because it will generate database query 'Select TABLE.*' which is expensive. "\ + "Suggest to use `#{ivar}.any?` to replace `#{ivar}.present?`" + end + + def message_blank(ivar) + "Avoid `#{ivar}.blank?`, because it will generate database query 'Select TABLE.*' which is expensive. "\ + "Suggest to use `#{ivar}.empty?` to replace `#{ivar}.blank?`" + end + + def_node_matcher :exists_match, <<~PATTERN + (send (ivar $_) :exists?) + PATTERN + + def_node_matcher :present_match, <<~PATTERN + (send (ivar $_) :present?) + PATTERN + + def_node_matcher :blank_match, <<~PATTERN + (send (ivar $_) :blank?) + 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_present = present_match(node) + ivar_blank = blank_match(node) + return unless ivar_present || ivar_blank + + node.each_ancestor(:begin) do |begin_node| + begin_node.each_descendant do |n| + ivar_exists = exists_match(n) + next unless ivar_exists + + add_offense(node, location: :expression, message: message_present(ivar_exists)) if ivar_exists == ivar_present + add_offense(node, location: :expression, message: message_blank(ivar_exists)) if ivar_exists == ivar_blank + end + end + end + end + end + end +end diff --git a/rubocop/cop/rspec/empty_line_after_shared_example.rb b/rubocop/cop/rspec/empty_line_after_shared_example.rb new file mode 100644 index 00000000000..5d09565bd5a --- /dev/null +++ b/rubocop/cop/rspec/empty_line_after_shared_example.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'rubocop/rspec/final_end_location' +require 'rubocop/rspec/blank_line_separation' +require 'rubocop/rspec/language' + +module RuboCop + module Cop + module RSpec + # Checks if there is an empty line after shared example blocks. + # + # @example + # # bad + # RSpec.describe Foo do + # it_behaves_like 'do this first' + # it_behaves_like 'does this' do + # end + # it_behaves_like 'does that' do + # end + # it_behaves_like 'do some more' + # end + # + # # good + # RSpec.describe Foo do + # it_behaves_like 'do this first' + # it_behaves_like 'does this' do + # end + # + # it_behaves_like 'does that' do + # end + # + # it_behaves_like 'do some more' + # end + # + # # fair - it's ok to have non-separated without blocks + # RSpec.describe Foo do + # it_behaves_like 'do this first' + # it_behaves_like 'does this' + # end + # + class EmptyLineAfterSharedExample < RuboCop::Cop::Cop + include RuboCop::RSpec::BlankLineSeparation + include RuboCop::RSpec::Language + + MSG = 'Add an empty line after `%<example>s` block.' + + def_node_matcher :shared_examples, + (SharedGroups::ALL + Includes::ALL).block_pattern + + def on_block(node) + shared_examples(node) do + break if last_child?(node) + + missing_separating_line(node) do |location| + add_offense(node, + location: location, + message: format(MSG, example: node.method_name)) + end + end + end + end + end + end +end |