diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f02093ef5..2a6756710e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ## master (unreleased) +* [#1035](https://github.com/rubocop/rubocop-rails/pull/1035): Add cop Rails/AcceptsNestedAttributesForUpdateOnly. ([@olivier-thatch][]) + ## 2.20.2 (2023-06-20) ### Bug fixes @@ -894,3 +896,4 @@ [@masato-bkn]: https://github.com/masato-bkn [@splattael]: https://github.com/splattael [@samrjenkins]: https://github.com/samrjenkins +[@olivier-thatch]: https://github.com/olivier-thatch diff --git a/config/default.yml b/config/default.yml index 0fef0c30d5..a489d15867 100644 --- a/config/default.yml +++ b/config/default.yml @@ -65,6 +65,14 @@ Rails: Enabled: true DocumentationBaseURL: https://docs.rubocop.org/rubocop-rails +Rails/AcceptsNestedAttributesForUpdateOnly: + Description: 'Define the update_only option to the accepts_nested_attributes_for attributes writers.' + StyleGuide: 'https://rails.rubystyle.guide#accepts_nested_attributes_for-update_only-option' + Enabled: false + VersionAdded: '2.21' + Include: + - app/models/**/*.rb + Rails/ActionControllerFlashBeforeRender: Description: 'Use `flash.now` instead of `flash` before `render`.' Enabled: 'pending' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index c56351643e..10bac2464b 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -16,6 +16,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. === Department xref:cops_rails.adoc[Rails] +* xref:cops_rails.adoc#railsacceptsnestedattributesforupdateonly[Rails/AcceptsNestedAttributesForUpdateOnly] * xref:cops_rails.adoc#railsactioncontrollerflashbeforerender[Rails/ActionControllerFlashBeforeRender] * xref:cops_rails.adoc#railsactioncontrollertestcase[Rails/ActionControllerTestCase] * xref:cops_rails.adoc#railsactionfilter[Rails/ActionFilter] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 3041223985..60b0f3d721 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -1,5 +1,51 @@ = Rails +== Rails/AcceptsNestedAttributesForUpdateOnly + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Disabled +| Yes +| No +| 2.21 +| - +|=== + +Looks for `accepts_nested_attributes_for` attributes writers that don't +specify an `:update_only` option. + +=== Examples + +[source,ruby] +---- +# bad +class Member < ActiveRecord::Base + has_one :avatar + accepts_nested_attributes_for :avatar +end + +# good +class Member < ActiveRecord::Base + has_one :avatar + accepts_nested_attributes_for :avatar, update_only: true +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| Include +| `+app/models/**/*.rb+` +| Array +|=== + +=== References + +* https://rails.rubystyle.guide#accepts_nested_attributes_for-update_only-option + == Rails/ActionControllerFlashBeforeRender |=== diff --git a/lib/rubocop/cop/rails/accepts_nested_attributes_for_update_only.rb b/lib/rubocop/cop/rails/accepts_nested_attributes_for_update_only.rb new file mode 100644 index 0000000000..2b1b791ed5 --- /dev/null +++ b/lib/rubocop/cop/rails/accepts_nested_attributes_for_update_only.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Looks for `accepts_nested_attributes_for` attributes writers that don't + # specify an `:update_only` option. + # + # @example + # # bad + # class Member < ActiveRecord::Base + # has_one :avatar + # accepts_nested_attributes_for :avatar + # end + # + # # good + # class Member < ActiveRecord::Base + # has_one :avatar + # accepts_nested_attributes_for :avatar, update_only: true + # end + class AcceptsNestedAttributesForUpdateOnly < Base + MSG = 'Specify a `:update_only` option.' + RESTRICT_ON_SEND = %i[accepts_nested_attributes_for].freeze + + def_node_search :active_resource_class?, <<~PATTERN + (const (const {nil? cbase} :ActiveResource) :Base) + PATTERN + + def_node_matcher :accepts_nested_attributes_for_without_options?, <<~PATTERN + (send _ {:accepts_nested_attributes_for} _) + PATTERN + + def_node_matcher :accepts_nested_attributes_for_with_options?, <<~PATTERN + (send _ {:accepts_nested_attributes_for} ... (hash $...)) + PATTERN + + def_node_matcher :update_only_option?, <<~PATTERN + (pair (sym :update_only) {!nil (nil)}) + PATTERN + + def_node_matcher :with_options_block, <<~PATTERN + (block + (send nil? :with_options + (hash $...)) + (args) ...) + PATTERN + + def_node_matcher :accepts_nested_attributes_for_extension_block?, <<~PATTERN + (block + (send nil? :accepts_nested_attributes_for _) + (args) ...) + PATTERN + + def on_send(node) + return if active_resource?(node.parent) + return if !accepts_nested_attributes_for_without_options?(node) && \ + valid_options?(accepts_nested_attributes_for_with_options?(node)) + return if valid_options_in_with_options_block?(node) + + add_offense(node.loc.selector) + end + + private + + def valid_options_in_with_options_block?(node) + return true unless node.parent + + n = node.parent.begin_type? + n ||= accepts_nested_attributes_for_extension_block?(node.parent) ? node.parent.parent : node.parent + + contain_valid_options_in_with_options_block?(n) + end + + def contain_valid_options_in_with_options_block?(node) + if (options = with_options_block(node)) + return true if valid_options?(options) + + return false unless node.parent + + return true if contain_valid_options_in_with_options_block?(node.parent.parent) + end + + false + end + + def valid_options?(options) + return false if options.nil? + + options = extract_option_if_kwsplat(options) + + return true unless options + return true if options.any? do |o| + update_only_option?(o) + end + + false + end + + def extract_option_if_kwsplat(options) + if options.first.kwsplat_type? && options.first.children.first.hash_type? + return options.first.children.first.pairs + end + + options + end + + def active_resource?(node) + return false if node.nil? + + active_resource_class?(node) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 5e0db983d6..8e7340415f 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -8,6 +8,7 @@ require_relative 'mixin/migrations_helper' require_relative 'mixin/target_rails_version' +require_relative 'rails/accepts_nested_attributes_for_update_only' require_relative 'rails/action_controller_flash_before_render' require_relative 'rails/action_controller_test_case' require_relative 'rails/action_filter' diff --git a/spec/rubocop/cop/rails/accepts_nested_attributes_for_update_only_spec.rb b/spec/rubocop/cop/rails/accepts_nested_attributes_for_update_only_spec.rb new file mode 100644 index 0000000000..b520a00189 --- /dev/null +++ b/spec/rubocop/cop/rails/accepts_nested_attributes_for_update_only_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::AcceptsNestedAttributesForUpdateOnly, :config do + context 'accepts_nested_attributes_for' do + it 'registers an offense when not specifying any options' do + expect_offense(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + accepts_nested_attributes_for :avatar + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specify a `:update_only` option. + end + RUBY + end + + it 'registers an offense when missing an explicit `:update_only` flag' do + expect_offense(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + accepts_nested_attributes_for :avatar, reject_if: :all_blank + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specify a `:update_only` option. + end + RUBY + end + + it 'does not register an offense when specifying `:update_only` flag' do + expect_no_offenses(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + accepts_nested_attributes_for :avatar, update_only: true + end + RUBY + end + + it 'does not register an offense when specifying `:update_only` flag with double splat' do + expect_no_offenses(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + accepts_nested_attributes_for :avatar, **{update_only: true} + end + RUBY + end + + it 'registers an offense when a variable passed with double splat' do + expect_offense(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + accepts_nested_attributes_for :avatar, **bar + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specify a `:update_only` option. + end + RUBY + end + + context 'with_options update_only: true' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + with_options update_only: true do + accepts_nested_attributes_for :avatar + end + end + RUBY + end + + it 'does not register an offense for using `reject_if` option' do + expect_no_offenses(<<~RUBY) + class Member < ApplicationRecord + has_one :avatar + with_options update_only: true do + accepts_nested_attributes_for :avatar, reject_if: :all_blank + end + end + RUBY + end + end + end + + context 'when an Active Record model does not have any associations' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class Member < ApplicationRecord + end + RUBY + end + end +end