diff options
Diffstat (limited to 'rubocop')
| -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/rspec/modify_sidekiq_middleware.rb | 50 | ||||
| -rw-r--r-- | rubocop/cop/static_translation_definition.rb | 43 | ||||
| -rw-r--r-- | rubocop/migration_helpers.rb | 7 |
6 files changed, 273 insertions, 59 deletions
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/rspec/modify_sidekiq_middleware.rb b/rubocop/cop/rspec/modify_sidekiq_middleware.rb new file mode 100644 index 00000000000..c38f074eb3a --- /dev/null +++ b/rubocop/cop/rspec/modify_sidekiq_middleware.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # This cop checks for `Sidekiq::Testing.server_middleware` + # usage in specs. + # + # @example + # + # # bad + # Sidekiq::Testing.server_middleware do |chain| + # chain.add(MyMiddlewareUnderTest) + # end + # + # + # # good + # with_custom_sidekiq_middleware do |chain| + # chain.add(MyMiddlewareUnderTest) + # end + # + # + class ModifySidekiqMiddleware < RuboCop::Cop::Cop + MSG = <<~MSG + Don't modify global sidekiq middleware, use the `#with_sidekiq_server_middleware` + helper instead + MSG + + def_node_search :modifies_sidekiq_middleware?, <<~PATTERN + (send + (const + (const nil? :Sidekiq) :Testing) :server_middleware) + PATTERN + + def on_send(node) + return unless modifies_sidekiq_middleware?(node) + + add_offense(node, location: :expression) + end + + def autocorrect(node) + -> (corrector) do + corrector.replace(node.loc.expression, + 'with_sidekiq_server_middleware') + end + end + end + end + end +end diff --git a/rubocop/cop/static_translation_definition.rb b/rubocop/cop/static_translation_definition.rb new file mode 100644 index 00000000000..736d8767342 --- /dev/null +++ b/rubocop/cop/static_translation_definition.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + class StaticTranslationDefinition < RuboCop::Cop::Cop + MSG = "The text you're translating will be already in the translated form when it's assigned to the constant. When a users changes the locale, these texts won't be translated again. Consider moving the translation logic to a method.".freeze + + TRANSLATION_METHODS = %i[_ s_ n_].freeze + + def_node_matcher :translation_method?, <<~PATTERN + (send _ _ str*) + PATTERN + + def_node_matcher :lambda_node?, <<~PATTERN + (send _ :lambda) + PATTERN + + def on_send(node) + return unless translation_method?(node) + + method_name = node.children[1] + return unless TRANSLATION_METHODS.include?(method_name) + + node.each_ancestor do |ancestor| + receiver, _ = *ancestor + break if lambda_node?(receiver) # translations defined in lambda nodes should be allowed + + if constant_assignment?(ancestor) + add_offense(node, location: :expression) + + break + end + end + end + + private + + def constant_assignment?(node) + node.type == :casgn + end + end + end +end diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb index 5fa6f8c2a2c..de377b15cc8 100644 --- a/rubocop/migration_helpers.rb +++ b/rubocop/migration_helpers.rb @@ -32,6 +32,7 @@ module RuboCop project_authorizations projects project_ci_cd_settings + project_features push_event_payloads resource_label_events routes @@ -53,6 +54,12 @@ module RuboCop ci_builds ].freeze + # List of helpers that add new columns, either directly (ADD_COLUMN_METHODS) + # or through a create/alter table (TABLE_METHODS) + ADD_COLUMN_METHODS = %i(add_column add_column_with_default change_column_type_concurrently).freeze + + TABLE_METHODS = %i(create_table create_table_if_not_exists change_table).freeze + # Returns true if the given node originated from the db/migrate directory. def in_migration?(node) dirname(node).end_with?('db/migrate', 'db/geo/migrate') || in_post_deployment_migration?(node) |