diff --git a/changelog/new_deprecated_http_status_names_cop.md b/changelog/new_deprecated_http_status_names_cop.md new file mode 100644 index 0000000000..0a26188c4b --- /dev/null +++ b/changelog/new_deprecated_http_status_names_cop.md @@ -0,0 +1 @@ +* [#1520](https://github.com/rubocop/rubocop-rails/pull/1520): New `Rails/HttpStatusNameConsistency` cop. ([@tuxagon][]) diff --git a/config/default.yml b/config/default.yml index 74bebd3142..efbd977606 100644 --- a/config/default.yml +++ b/config/default.yml @@ -609,6 +609,14 @@ Rails/HttpStatus: - numeric - symbolic +Rails/HttpStatusNameConsistency: + Description: 'Enforces consistency by using the current HTTP status names.' + Enabled: pending + Severity: warning + VersionAdded: '<>' + Include: + - '**/app/controllers/**/*.rb' + Rails/I18nLazyLookup: Description: 'Checks for places where I18n "lazy" lookup can be used.' StyleGuide: 'https://rails.rubystyle.guide/#lazy-lookup' diff --git a/lib/rubocop/cop/rails/http_status_name_consistency.rb b/lib/rubocop/cop/rails/http_status_name_consistency.rb new file mode 100644 index 0000000000..89ac0bb3bd --- /dev/null +++ b/lib/rubocop/cop/rails/http_status_name_consistency.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Enforces consistency by using the current HTTP status names. + # + # @example + # # bad + # render json: { error: "Invalid data" }, status: :unprocessable_entity + # head :payload_too_large + # + # # good + # render json: { error: "Invalid data" }, status: :unprocessable_content + # head :content_too_large + # + class HttpStatusNameConsistency < Base + extend AutoCorrector + + requires_gem 'rack', '>= 3.1.0' + + RESTRICT_ON_SEND = %i[render redirect_to head assert_response assert_redirected_to].freeze + + PREFERRED_STATUSES = { + unprocessable_entity: :unprocessable_content, + payload_too_large: :content_too_large + }.freeze + + MSG = 'Prefer `:%s` over `:%s`.' + + def_node_matcher :status_method_call, <<~PATTERN + { + (send nil? {:render :redirect_to} _ $hash) + (send nil? {:render :redirect_to} $hash) + (send nil? {:head :assert_response} $_ ...) + (send nil? :assert_redirected_to _ $hash ...) + (send nil? :assert_redirected_to $hash ...) + } + PATTERN + + def_node_matcher :status_hash_value, <<~PATTERN + (hash <(pair (sym :status) $_) ...>) + PATTERN + + def on_send(node) + status_method_call(node) do |status_node| + if status_node.hash_type? + # Handle hash arguments like { status: :unprocessable_entity } + status_hash_value(status_node) do |status_value| + find_deprecated_status_names(status_value) + end + else + # Handle positional arguments like head :unprocessable_entity + find_deprecated_status_names(status_node) + end + end + end + + private + + def find_deprecated_status_names(node) + if node.sym_type? && PREFERRED_STATUSES.key?(node.value) + deprecated_status = node.value + preferred_status = PREFERRED_STATUSES[deprecated_status] + + message = format(MSG, deprecated: deprecated_status, preferred: preferred_status) + + add_offense(node, message: message) do |corrector| + corrector.replace(node, ":#{preferred_status}") + end + elsif node.respond_to?(:children) + # Recursively search child nodes (handles ternary expressions, etc.) + node.children.each do |child| + find_deprecated_status_names(child) if child.is_a?(Parser::AST::Node) + end + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index a8c7202dac..8132d3f1db 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -66,6 +66,7 @@ require_relative 'rails/helper_instance_variable' require_relative 'rails/http_positional_arguments' require_relative 'rails/http_status' +require_relative 'rails/http_status_name_consistency' require_relative 'rails/i18n_lazy_lookup' require_relative 'rails/i18n_locale_assignment' require_relative 'rails/i18n_locale_texts' diff --git a/spec/rubocop/cop/rails/http_status_name_consistency_spec.rb b/spec/rubocop/cop/rails/http_status_name_consistency_spec.rb new file mode 100644 index 0000000000..ae720a1d24 --- /dev/null +++ b/spec/rubocop/cop/rails/http_status_name_consistency_spec.rb @@ -0,0 +1,194 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::HttpStatusNameConsistency, :config do + context 'when Rack is older than 3.1' do + let(:gem_versions) { { 'rack' => '3.0.0' } } + + it 'does nothing' do + expect_no_offenses(<<~RUBY) + render json: { error: 'Invalid data' }, status: :unprocessable_entity + head :payload_too_large + RUBY + end + end + + context 'when Rack is 3.1 or later' do + let(:gem_versions) { { 'rack' => '3.1.0' } } + + context 'with :unprocessable_entity' do + it 'registers an offense when using :unprocessable_entity in render' do + expect_offense(<<~RUBY) + render json: { error: 'Invalid data' }, status: :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + render json: { error: 'Invalid data' }, status: :unprocessable_content + RUBY + end + + it 'registers an offense when using :unprocessable_entity in head' do + expect_offense(<<~RUBY) + head :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + head :unprocessable_content + RUBY + end + + it 'registers an offense when using :unprocessable_entity in redirect_to' do + expect_offense(<<~RUBY) + redirect_to some_path, status: :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + redirect_to some_path, status: :unprocessable_content + RUBY + end + + it 'registers an offense when using :unprocessable_entity in assert_response' do + expect_offense(<<~RUBY) + assert_response :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + assert_response :unprocessable_content + RUBY + end + + it 'registers an offense when using :unprocessable_entity in assert_redirected_to' do + expect_offense(<<~RUBY) + assert_redirected_to some_path, status: :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + assert_redirected_to some_path, status: :unprocessable_content + RUBY + end + + it 'registers an offense when using :unprocessable_entity in ternary expression' do + expect_offense(<<~RUBY) + render json: { error: 'Invalid data' }, status: some_condition ? :unprocessable_entity : :ok + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + RUBY + + expect_correction(<<~RUBY) + render json: { error: 'Invalid data' }, status: some_condition ? :unprocessable_content : :ok + RUBY + end + + it 'does not register an offense when using :unprocessable_content' do + expect_no_offenses(<<~RUBY) + render json: { error: 'Invalid data' }, status: :unprocessable_content + RUBY + end + + it 'does not register an offense when using :unprocessable_entity in hash key' do + expect_no_offenses(<<~RUBY) + { unprocessable_entity: 'Invalid data' } + RUBY + end + end + + context 'with :payload_too_large' do + it 'registers an offense when using :payload_too_large in render' do + expect_offense(<<~RUBY) + render json: { error: 'File too big' }, status: :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + render json: { error: 'File too big' }, status: :content_too_large + RUBY + end + + it 'registers an offense when using :payload_too_large in head' do + expect_offense(<<~RUBY) + head :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + head :content_too_large + RUBY + end + + it 'registers an offense when using :payload_too_large in redirect_to' do + expect_offense(<<~RUBY) + redirect_to some_path, status: :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + redirect_to some_path, status: :content_too_large + RUBY + end + + it 'registers an offense when using :payload_too_large in assert_response' do + expect_offense(<<~RUBY) + assert_response :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + assert_response :content_too_large + RUBY + end + + it 'registers an offense when using :payload_too_large in assert_redirected_to' do + expect_offense(<<~RUBY) + assert_redirected_to some_path, status: :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + assert_redirected_to some_path, status: :content_too_large + RUBY + end + + it 'registers an offense when using :payload_too_large in ternary expression' do + expect_offense(<<~RUBY) + render json: { error: 'File too big' }, status: some_condition ? :payload_too_large : :ok + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + render json: { error: 'File too big' }, status: some_condition ? :content_too_large : :ok + RUBY + end + + it 'does not register an offense when using :content_too_large' do + expect_no_offenses(<<~RUBY) + render json: { error: 'File too big' }, status: :content_too_large + RUBY + end + + it 'does not register an offense when using :payload_too_large in hash key' do + expect_no_offenses(<<~RUBY) + { payload_too_large: 'File too big' } + RUBY + end + end + + context 'with mixed deprecated statuses' do + it 'handles multiple deprecated statuses in the same code' do + expect_offense(<<~RUBY) + head :unprocessable_entity + ^^^^^^^^^^^^^^^^^^^^^ Prefer `:unprocessable_content` over `:unprocessable_entity`. + head :payload_too_large + ^^^^^^^^^^^^^^^^^^ Prefer `:content_too_large` over `:payload_too_large`. + RUBY + + expect_correction(<<~RUBY) + head :unprocessable_content + head :content_too_large + RUBY + end + end + end +end