ADD Rails/HttpStatus cop (rubocop#5597)

This cop enforces consistent use of symbolic or numeric value to define HTTP status. @example EnforcedStyle: symbolic (default) ```ruby # bad render :foo, status: 200 render json: { foo: 'bar' }, status: 200 render plain: 'foo/bar', status: 304 redirect_to root_url, status: 301 # good render :foo, status: :ok render json: { foo: 'bar' }, status: :ok render plain: 'foo/bar', status: :not_modified redirect_to root_url, status: :moved_permanently ``` @example EnforcedStyle: numeric ```ruby # bad render :foo, status: :ok render json: { foo: 'bar' }, status: :not_found render plain: 'foo/bar', status: :not_modified redirect_to root_url, status: :moved_permanently # good render :foo, status: 200 render json: { foo: 'bar' }, status: 404 render plain: 'foo/bar', status: 304 redirect_to root_url, status: 301 ```
pocke · Mar 14, 2018 · 841569b · 841569b
1 parent 9da2633
commit 841569b
Show file tree

Hide file tree

Showing 9 changed files with 422 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ## master (unreleased)
 
+### New features
+* [#5597](https://github.com/bbatsov/rubocop/pull/5597): Add new `Rails/HttpStatus` cop. ([@anthony-robin][])
+
 ### Bug fixes
 
 * [#5642](https://github.com/bbatsov/rubocop/pull/5642): Fix Style/Documentation :nodoc: for compact-style nested modules/classes. ([@ojab][])
@@ -3249,3 +3252,4 @@
 [@unused]: https://github.com/unused
 [@htwroclau]: https://github.com/htwroclau
 [@hamada14]: https://github.com/hamada14
+[@anthony-robin]: https://github.com/anthony-robin
diff --git a/config/default.yml b/config/default.yml
@@ -1667,6 +1667,12 @@ Rails/HasManyOrHasOneDependent:
   Include:
     - app/models/**/*.rb
 
+Rails/HttpStatus:
+  EnforcedStyle: symbolic
+  SupportedStyles:
+    - numeric
+    - symbolic
+
 Rails/InverseOf:
   Include:
     - app/models/**/*.rb

diff --git a/config/enabled.yml b/config/enabled.yml
@@ -1169,6 +1169,10 @@ Rails/HttpPositionalArguments:
     - 'spec/**/*'
     - 'test/**/*'
 
+Rails/HttpStatus:
+  Description: 'Enforces use of symbolic or numeric value to define HTTP status.'
+  Enabled: true
+
 Rails/InverseOf:
   Description: 'Checks for associations where the inverse cannot be determined automatically.'
   Enabled: true

diff --git a/lib/rubocop.rb b/lib/rubocop.rb
@@ -550,6 +550,7 @@
 require_relative 'rubocop/cop/rails/has_and_belongs_to_many'
 require_relative 'rubocop/cop/rails/has_many_or_has_one_dependent'
 require_relative 'rubocop/cop/rails/http_positional_arguments'
+require_relative 'rubocop/cop/rails/http_status'
 require_relative 'rubocop/cop/rails/inverse_of'
 require_relative 'rubocop/cop/rails/lexically_scoped_action_filter'
 require_relative 'rubocop/cop/rails/not_null_column'

diff --git a/lib/rubocop/cop/rails/http_status.rb b/lib/rubocop/cop/rails/http_status.rb
@@ -0,0 +1,181 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Enforces use of symbolic or numeric value to define HTTP status.
+      #
+      # @example `EnforcedStyle: symbolic` (default)
+      #   # bad
+      #   render :foo, status: 200
+      #   render json: { foo: 'bar' }, status: 200
+      #   render plain: 'foo/bar', status: 304
+      #   redirect_to root_url, status: 301
+      #
+      #   # good
+      #   render :foo, status: :ok
+      #   render json: { foo: 'bar' }, status: :ok
+      #   render plain: 'foo/bar', status: :not_modified
+      #   redirect_to root_url, status: :moved_permanently
+      #
+      # @example `EnforcedStyle: numeric`
+      #   # bad
+      #   render :foo, status: :ok
+      #   render json: { foo: 'bar' }, status: :not_found
+      #   render plain: 'foo/bar', status: :not_modified
+      #   redirect_to root_url, status: :moved_permanently
+      #
+      #   # good
+      #   render :foo, status: 200
+      #   render json: { foo: 'bar' }, status: 404
+      #   render plain: 'foo/bar', status: 304
+      #   redirect_to root_url, status: 301
+      #
+      class HttpStatus < Cop
+        begin
+          require 'rack/utils'
+          RACK_LOADED = true
+        rescue LoadError
+          RACK_LOADED = false
+        end
+
+        include ConfigurableEnforcedStyle
+
+        def_node_matcher :http_status, <<-PATTERN
+          {
+            (send nil? {:render :redirect_to}
+              _
+              (hash
+                (pair
+                  (sym :status)
+                  ${int sym})))
+            (send nil? {:render}
+              (hash
+                _
+                (pair
+                  (sym :status)
+                  ${int sym})))
+          }
+        PATTERN
+
+        def on_send(node)
+          http_status(node) do |ast_node|
+            checker = checker_class.new(ast_node)
+            return unless checker.offensive?
+            add_offense(checker.node, message: checker.message)
+          end
+        end
+
+        def support_autocorrect?
+          RACK_LOADED
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            checker = checker_class.new(node)
+            corrector.replace(node.loc.expression, checker.preferred_style)
+          end
+        end
+
+        private
+
+        def checker_class
+          case style
+          when :symbolic
+            SymbolicStyleChecker
+          when :numeric
+            NumericStyleChecker
+          end
+        end
+
+        # :nodoc:
+        class SymbolicStyleChecker
+          MSG = 'Prefer `%<prefer>s` over `%<current>s` ' \
+                'to define HTTP status code.'.freeze
+          DEFAULT_MSG = 'Prefer `symbolic` over `numeric` ' \
+                        'to define HTTP status code.'.freeze
+
+          attr_reader :node
+          def initialize(node)
+            @node = node
+          end
+
+          def offensive?
+            !node.sym_type? && !custom_http_status_code?
+          end
+
+          def message
+            if RACK_LOADED
+              format(MSG, prefer: preferred_style, current: number.to_s)
+            else
+              DEFAULT_MSG
+            end
+          end
+
+          def preferred_style
+            symbol.inspect
+          end
+
+          private
+
+          def symbol
+            ::Rack::Utils::SYMBOL_TO_STATUS_CODE.key(number)
+          end
+
+          def number
+            node.children.first
+          end
+
+          def custom_http_status_code?
+            node.int_type? &&
+              !::Rack::Utils::SYMBOL_TO_STATUS_CODE.value?(number)
+          end
+        end
+
+        # :nodoc:
+        class NumericStyleChecker
+          MSG = 'Prefer `%<prefer>s` over `%<current>s` ' \
+                'to define HTTP status code.'.freeze
+          DEFAULT_MSG = 'Prefer `numeric` over `symbolic` ' \
+                        'to define HTTP status code.'.freeze
+          WHITELIST_STATUS = %i[error success missing redirect].freeze
+
+          attr_reader :node
+          def initialize(node)
+            @node = node
+          end
+
+          def offensive?
+            !node.int_type? && !whitelisted_symbol?
+          end
+
+          def message
+            if RACK_LOADED
+              format(MSG, prefer: preferred_style, current: symbol.inspect)
+            else
+              DEFAULT_MSG
+            end
+          end
+
+          def preferred_style
+            number.to_s
+          end
+
+          private
+
+          def number
+            ::Rack::Utils::SYMBOL_TO_STATUS_CODE[symbol]
+          end
+
+          def symbol
+            node.value
+          end
+
+          def whitelisted_symbol?
+            node.sym_type? && WHITELIST_STATUS.include?(node.value)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/manual/cops.md b/manual/cops.md
@@ -340,6 +340,7 @@ In the following section you find all available cops:
 * [Rails/HasAndBelongsToMany](cops_rails.md#railshasandbelongstomany)
 * [Rails/HasManyOrHasOneDependent](cops_rails.md#railshasmanyorhasonedependent)
 * [Rails/HttpPositionalArguments](cops_rails.md#railshttppositionalarguments)
+* [Rails/HttpStatus](cops_rails.md#railshttpstatus)
 * [Rails/InverseOf](cops_rails.md#railsinverseof)
 * [Rails/LexicallyScopedActionFilter](cops_rails.md#railslexicallyscopedactionfilter)
 * [Rails/NotNullColumn](cops_rails.md#railsnotnullcolumn)

diff --git a/manual/cops_rails.md b/manual/cops_rails.md
@@ -694,6 +694,53 @@ Name | Default value | Configurable values
 --- | --- | ---
 Include | `spec/**/*`, `test/**/*` | Array
 
+## Rails/HttpStatus
+
+Enabled by default | Supports autocorrection
+--- | ---
+Enabled | Yes
+
+Enforces use of symbolic or numeric value to define HTTP status.
+
+### Examples
+
+#### `EnforcedStyle: symbolic` (default)
+
+```ruby
+# bad
+render :foo, status: 200
+render json: { foo: 'bar' }, status: 200
+render plain: 'foo/bar', status: 304
+redirect_to root_url, status: 301
+
+# good
+render :foo, status: :ok
+render json: { foo: 'bar' }, status: :ok
+render plain: 'foo/bar', status: :not_modified
+redirect_to root_url, status: :moved_permanently
+```
+#### `EnforcedStyle: numeric`
+
+```ruby
+# bad
+render :foo, status: :ok
+render json: { foo: 'bar' }, status: :not_found
+render plain: 'foo/bar', status: :not_modified
+redirect_to root_url, status: :moved_permanently
+
+# good
+render :foo, status: 200
+render json: { foo: 'bar' }, status: 404
+render plain: 'foo/bar', status: 304
+redirect_to root_url, status: 301
+```
+
+### Configurable attributes
+
+Name | Default value | Configurable values
+--- | --- | ---
+EnforcedStyle | `symbolic` | `numeric`, `symbolic`
+
 ## Rails/InverseOf
 
 Enabled by default | Supports autocorrection

diff --git a/rubocop.gemspec b/rubocop.gemspec
@@ -44,5 +44,6 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency('unicode-display_width', '~> 1.0', '>= 1.0.1')
 
   s.add_development_dependency('bundler', '~> 1.3')
+  s.add_development_dependency('rack')
 end
 # rubocop:enable Metrics/BlockLength