From 766f0db1f16a9419498752900765a0aaf4b5b002 Mon Sep 17 00:00:00 2001 From: Dan Kubb Date: Fri, 17 Jan 2025 14:53:38 -0800 Subject: [PATCH] Add RuboCop::Cop::Betterment::NoStrictLoading cops * Add ByDefaultForModels to ensure models do not set self.strict_loading_by_default. * Add ForAssociations to ensure model associations do not set :strict_loading option. The purpose of this cop is to make sure to highlight any places where we explicitly set strict loading, as opposed to relyong on global settings. The idea will be to capture all the offenses in a .rubocop_todo.yml so we can burn down and fix all the cases over time. --- lib/rubocop/cop/betterment.rb | 1 + .../cop/betterment/no_strict_loading.rb | 73 +++++++++++ .../by_default_for_models_spec.rb | 83 +++++++++++++ .../for_associations_spec.rb | 113 ++++++++++++++++++ 4 files changed, 270 insertions(+) create mode 100644 lib/rubocop/cop/betterment/no_strict_loading.rb create mode 100644 spec/rubocop/cop/betterment/no_strict_loading/by_default_for_models_spec.rb create mode 100644 spec/rubocop/cop/betterment/no_strict_loading/for_associations_spec.rb diff --git a/lib/rubocop/cop/betterment.rb b/lib/rubocop/cop/betterment.rb index 6c0b502..9d5c313 100644 --- a/lib/rubocop/cop/betterment.rb +++ b/lib/rubocop/cop/betterment.rb @@ -11,6 +11,7 @@ require 'rubocop/cop/betterment/implicit_redirect_type' require 'rubocop/cop/betterment/internals_protection' require 'rubocop/cop/betterment/memoization_with_arguments' +require 'rubocop/cop/betterment/no_strict_loading' require 'rubocop/cop/betterment/non_standard_actions' require 'rubocop/cop/betterment/non_standard_controller' require 'rubocop/cop/betterment/redirect_status' diff --git a/lib/rubocop/cop/betterment/no_strict_loading.rb b/lib/rubocop/cop/betterment/no_strict_loading.rb new file mode 100644 index 0000000..1c5855b --- /dev/null +++ b/lib/rubocop/cop/betterment/no_strict_loading.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Betterment + module NoStrictLoading + # This cop ensures that `self.strict_loading_by_default = ` is not set in ActiveRecord models. + class ByDefaultForModels < Base + extend AutoCorrector + + MSG = 'Do not set `self.strict_loading_by_default` in ActiveRecord models.' + + # @!method strict_loading_by_default_set?(node) + def_node_matcher :strict_loading_by_default_set?, <<~PATTERN + (send self :strict_loading_by_default= _) + PATTERN + + def on_send(node) + return unless strict_loading_by_default_set?(node) + + add_offense(node.source_range) do |corrector| + buffer = node.source_range.source_buffer + start_pos = node.source_range.begin_pos - node.source_range.column + + # Find end of line including any extra blank line + end_pos = start_pos + end_pos += 1 while end_pos < buffer.source.length && (buffer.source[end_pos] != "\n" || buffer.source[end_pos + 1] == "\n") + + corrector.remove(Parser::Source::Range.new(buffer, start_pos, end_pos + 1)) + end + end + end + + # This cop ensures that `strict_loading: ` is not set in ActiveRecord associations. + class ForAssociations < Base + extend AutoCorrector + + MSG = 'Do not set `:strict_loading` in ActiveRecord associations.' + + ASSOCIATION_METHODS = %i(belongs_to has_and_belongs_to_many has_many has_one).freeze + + # @!method association_with_strict_loading?(node) + def_node_matcher :association_with_strict_loading?, <<~PATTERN + (send nil? {#{ASSOCIATION_METHODS.map(&:inspect).join(' ')}} _ (hash <(pair (sym :strict_loading) _) ...>)) + PATTERN + + def on_send(node) # rubocop:disable Metrics/PerceivedComplexity + return unless association_with_strict_loading?(node) + + hash_node = node.last_argument + return unless hash_node.hash_type? + + hash_node.pairs.each do |pair| + next unless pair.key.sym_type? && pair.key.value.equal?(:strict_loading) + + add_offense(pair.source_range) do |corrector| + if hash_node.pairs.one? + # Remove from comma after symbol to end of hash + range = hash_node.source_range.adjust(begin_pos: -2) + corrector.remove(range) + else + # Create new hash with remaining pairs + new_source = (hash_node.pairs - [pair]).map(&:source).join(', ') + corrector.replace(hash_node, new_source) + end + end + end + end + end + end + end + end +end diff --git a/spec/rubocop/cop/betterment/no_strict_loading/by_default_for_models_spec.rb b/spec/rubocop/cop/betterment/no_strict_loading/by_default_for_models_spec.rb new file mode 100644 index 0000000..6602089 --- /dev/null +++ b/spec/rubocop/cop/betterment/no_strict_loading/by_default_for_models_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RuboCop::Cop::Betterment::NoStrictLoading::ByDefaultForModels, :config do + context 'when `self.strict_loading_by_default` is set' do + context 'when the class has no code before or after self.strict_loading_by_default' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + self.strict_loading_by_default = true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `self.strict_loading_by_default` in ActiveRecord models. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + end + RUBY + end + end + + context 'when the class has a comment after self.strict_loading_by_default' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + self.strict_loading_by_default = true # Set to true to enable strict_loading_by_default + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `self.strict_loading_by_default` in ActiveRecord models. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + end + RUBY + end + end + + context 'when the class has code before self.strict_loading_by_default' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + self.table_name = :my_models + self.strict_loading_by_default = true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `self.strict_loading_by_default` in ActiveRecord models. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + self.table_name = :my_models + end + RUBY + end + end + + context 'when the class has code after self.strict_loading_by_default' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + self.strict_loading_by_default = true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `self.strict_loading_by_default` in ActiveRecord models. + + belongs_to :user + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + belongs_to :user + end + RUBY + end + end + end + + it 'does not register an offense when `self.strict_loading_by_default` is not set' do + expect_no_offenses(<<~RUBY) + class MyModel < ApplicationRecord + end + RUBY + end +end diff --git a/spec/rubocop/cop/betterment/no_strict_loading/for_associations_spec.rb b/spec/rubocop/cop/betterment/no_strict_loading/for_associations_spec.rb new file mode 100644 index 0000000..c82b8ff --- /dev/null +++ b/spec/rubocop/cop/betterment/no_strict_loading/for_associations_spec.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RuboCop::Cop::Betterment::NoStrictLoading::ForAssociations, :config do + context 'when `strict_loading` is set in an association' do + it 'registers an offense for belongs_to' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + belongs_to :user, strict_loading: false # preserve this comment + ^^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + belongs_to :user # preserve this comment + end + RUBY + end + + it 'registers an offense for has_and_belongs_to_many' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + has_and_belongs_to_many :tags, strict_loading: true # preserve this comment + ^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + has_and_belongs_to_many :tags # preserve this comment + end + RUBY + end + + it 'registers an offense for has_many' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts, strict_loading: true # preserve this comment + ^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts # preserve this comment + end + RUBY + end + + it 'registers an offense for has_one' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + has_one :account, strict_loading: true # preserve this comment + ^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + has_one :account # preserve this comment + end + RUBY + end + + it 'preserves other options' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts, strict_loading: true, dependent: :destroy # preserve this comment + ^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts, dependent: :destroy # preserve this comment + end + RUBY + end + + it 'preserves multiline options' do + expect_offense(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts, # preserve this comment + strict_loading: true, # remove this comment + ^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations. + dependent: :destroy # preserve this comment + end + RUBY + + expect_correction(<<~RUBY) + class MyModel < ApplicationRecord + has_many :posts, # preserve this comment + dependent: :destroy # preserve this comment + end + RUBY + end + end + + context 'when `strict_loading` is not set in an association' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class MyModel < ApplicationRecord + belongs_to :user # comment + has_many :posts # comment + has_and_belongs_to_many :tags # comment + has_one :account # comment + end + RUBY + end + end +end