In Rails >= 5, use optional: true, not required: false (#6596)

The `required` option on belongs_to associations has been deprecated, instead the recommended (and default) approach is to set this to true by default and disable it using `optional: true`. The developer can override this default globally, however, so we can't say that things like `optional: false` are superfluous, or indeed that `required: true` isn't necessary. This change therefore only makes the known-to-be-valid correction from `required: false` to `optional: true` and registers an offence for any instances of `required` See: https://guides.rubyonrails.org/5_0_release_notes.html
rubocop · Jan 14, 2019 · a5105d8 · a5105d8
1 parent 3a32310
commit a5105d8
Show file tree

Hide file tree

Showing 6 changed files with 189 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### New features
 
+* [#6596](https://github.com/rubocop-hq/rubocop/pull/6596): Add offence when using `required` option for `belongs_to` associations in Rails >= 5. ([@petehamilton][])
 * [#6604](https://github.com/rubocop-hq/rubocop/pull/6604): Add auto-correct support to `Rails/LinkToBlank`. ([@Intrepidd][])
 * [#6660](https://github.com/rubocop-hq/rubocop/pull/6660): Add new `Rails/IgnoredSkipActionFilterOption` cop. ([@wata727][])
 * [#6363](https://github.com/rubocop-hq/rubocop/issues/6363): Allow `Style/YodaCondition` cop to be configured to enforce yoda conditions. ([@tejasbubane][])

diff --git a/lib/rubocop.rb b/lib/rubocop.rb
@@ -562,6 +562,7 @@
 require_relative 'rubocop/cop/rails/application_job'
 require_relative 'rubocop/cop/rails/application_record'
 require_relative 'rubocop/cop/rails/assert_not'
+require_relative 'rubocop/cop/rails/belongs_to'
 require_relative 'rubocop/cop/rails/blank'
 require_relative 'rubocop/cop/rails/bulk_change_table'
 require_relative 'rubocop/cop/rails/create_table_with_timestamps'

diff --git a/lib/rubocop/cop/rails/belongs_to.rb b/lib/rubocop/cop/rails/belongs_to.rb
@@ -0,0 +1,101 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop looks for belongs_to associations where we control whether the
+      # association is required via the deprecated `required` option instead.
+      #
+      # Since Rails 5, belongs_to associations are required by default and this
+      # can be controlled through the use of `optional: true`.
+      #
+      # From the release notes:
+      #
+      #     belongs_to will now trigger a validation error by default if the
+      #     association is not present. You can turn this off on a
+      #     per-association basis with optional: true. Also deprecate required
+      #     option in favor of optional for belongs_to. (Pull Request)
+      #
+      # In the case that the developer is doing `required: false`, we
+      # definitely want to autocorrect to `optional: true`.
+      #
+      # However, without knowing whether they've set overriden the default
+      # value of `config.active_record.belongs_to_required_by_default`, we
+      # can't say whether it's safe to remove `required: true` or replace it
+      # with `optional: false` (or, similarly, remove a superfluous `optional:
+      # false`). Therefore, in the cases we're using `required: true`, we'll
+      # highlight that `required` is deprecated but otherwise do nothing.
+      #
+      # @example
+      #   # bad
+      #   class Post < ApplicationRecord
+      #     belongs_to :blog, required: false
+      #   end
+      #
+      #   # good
+      #   class Post < ApplicationRecord
+      #     belongs_to :blog, optional: true
+      #   end
+      #
+      # @see https://guides.rubyonrails.org/5_0_release_notes.html
+      # @see https://github.com/rails/rails/pull/18937
+      class BelongsTo < Cop
+        extend TargetRailsVersion
+
+        minimum_target_rails_version 5.0
+
+        DEPRECATED_REQUIRE =
+          'The use of `required` on belongs_to associations was deprecated ' \
+          'in Rails 5. Please use the `optional` flag instead'.freeze
+
+        SUPERFLOUS_REQUIRE_MSG =
+          'You specified `required: false`, in Rails > 5.0 the requires ' \
+          'option is deprecated and you want to use `optional: true`.'.freeze
+
+        def_node_matcher :match_belongs_to_with_options, <<-PATTERN
+          (send $_ :belongs_to _ (hash $...))
+        PATTERN
+
+        def_node_matcher :match_requires_false?, <<-PATTERN
+          (pair (sym :required) false)
+        PATTERN
+
+        def_node_matcher :match_requires_any?, <<-PATTERN
+          (pair (sym :required) $_)
+        PATTERN
+
+        def on_send(node)
+          _, opts = match_belongs_to_with_options(node)
+          if opts && opts.any? { |opt| match_requires_false?(opt) }
+            add_offense(
+              node,
+              message: SUPERFLOUS_REQUIRE_MSG,
+              location: :selector
+            )
+          elsif opts && opts.any? { |opt| match_requires_any?(opt) }
+            add_offense(
+              node,
+              message: DEPRECATED_REQUIRE,
+              location: :selector
+            )
+          end
+        end
+
+        def autocorrect(node)
+          _, opts = match_belongs_to_with_options(node)
+          return nil if opts && opts.none? { |opt| match_requires_false?(opt) }
+
+          lambda do |corrector|
+            requires_expression =
+              node.children[3].children.find { |c| match_requires_false?(c) }
+
+            corrector.replace(
+              requires_expression.loc.expression,
+              'optional: true'
+            )
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/manual/cops.md b/manual/cops.md
@@ -338,6 +338,7 @@ In the following section you find all available cops:
 * [Rails/ApplicationJob](cops_rails.md#railsapplicationjob)
 * [Rails/ApplicationRecord](cops_rails.md#railsapplicationrecord)
 * [Rails/AssertNot](cops_rails.md#railsassertnot)
+* [Rails/BelongsTo](cops_rails.md#railsbelongsto)
 * [Rails/Blank](cops_rails.md#railsblank)
 * [Rails/BulkChangeTable](cops_rails.md#railsbulkchangetable)
 * [Rails/CreateTableWithTimestamps](cops_rails.md#railscreatetablewithtimestamps)

diff --git a/manual/cops_rails.md b/manual/cops_rails.md
@@ -162,6 +162,49 @@ Name | Default value | Configurable values
 --- | --- | ---
 Include | `**/test/**/*` | Array
 
+## Rails/BelongsTo
+
+Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+--- | --- | --- | --- | ---
+Enabled | Yes | Yes  | - | -
+
+This cop looks for belongs_to associations where we control whether the
+association is required via the deprecated `required` option instead.
+
+Since Rails 5, belongs_to associations are required by default and this
+can be controlled through the use of `optional: true`.
+
+From the release notes:
+
+    belongs_to will now trigger a validation error by default if the
+    association is not present. You can turn this off on a
+    per-association basis with optional: true. Also deprecate required
+    option in favor of optional for belongs_to. (Pull Request)
+
+In the case that the developer is doing `required: false`, we
+definitely want to autocorrect to `optional: true`.
+
+However, without knowing whether they've set overriden the default
+value of `config.active_record.belongs_to_required_by_default`, we
+can't say whether it's safe to remove `required: true` or replace it
+with `optional: false` (or, similarly, remove a superfluous `optional:
+false`). Therefore, in the cases we're using `required: true`, we'll
+highlight that `required` is deprecated but otherwise do nothing.
+
+### Examples
+
+```ruby
+# bad
+class Post < ApplicationRecord
+  belongs_to :blog, required: false
+end
+
+# good
+class Post < ApplicationRecord
+  belongs_to :blog, optional: true
+end
+```
+
 ## Rails/Blank
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

diff --git a/spec/rubocop/cop/rails/belongs_to_spec.rb b/spec/rubocop/cop/rails/belongs_to_spec.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::BelongsTo do
+  subject(:cop) { described_class.new(config) }
+
+  let(:config) { RuboCop::Config.new }
+
+  it 'registers an offense when specifying `required: false`' do
+    expect_offense(<<-RUBY.strip_indent)
+      belongs_to :foo, required: false
+      ^^^^^^^^^^ You specified `required: false`, in Rails > 5.0 the requires option is deprecated and you want to use `optional: true`.
+    RUBY
+  end
+
+  it 'registers an offense when specifying `required: true`' do
+    expect_offense(<<-RUBY.strip_indent)
+      belongs_to :foo, required: true
+      ^^^^^^^^^^ The use of `required` on belongs_to associations was deprecated in Rails 5. Please use the `optional` flag instead
+    RUBY
+  end
+
+  it 'auto-corrects `required: false` to `optional: true`' do
+    expect(autocorrect_source('belongs_to :foo, required: false'))
+      .to eq('belongs_to :foo, optional: true')
+    expect(cop.offenses.last.status).to eq(:corrected)
+  end
+
+  it 'does not auto-correct `required: true`' do
+    code = 'belongs_to :foo, required: true'
+    expect(autocorrect_source(code)).to eq(code)
+    expect(cop.offenses.last.status).to eq(:uncorrected)
+  end
+
+  it 'registers no offense when setting `optional: true`' do
+    expect_no_offenses('belongs_to :foo, optional: true')
+  end
+
+  it 'registers no offense when requires: false is not set' do
+    expect_no_offenses('belongs_to :foo')
+    expect_no_offenses('belongs_to :foo, polymorphic: true')
+  end
+end