Add RuboCop cop to enforce no strict loading model or association con…

…fig (#69)

### Summary

- [x] [Fix GitHub Action to quote ruby 3.0 version so it's not evaluated
as
3](98706ef)
- [x] [Fix rubocop/cop/betterment require list to be
sorted](e2784ab)
- [x] [Add RuboCop::Cop::Betterment::UseGlobalStrictLoading
cops](ce87479)
- [x] [Upgrade betterlint to
1.17.0](ffeb167)

### Description

This branch adds two cops to ensure models and associations don't have
strict loading configuration.

.github/workflows/tests.yml

@@ -8,7 +8,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        ruby: [3.0, 3.1, 3.2]
+        ruby: ['3.0', 3.1, 3.2]
         gemfile:
           - Gemfile
 

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [1.17.0] - 2025-01-24
+### Added
+  - Betterment/UseGlobalStrictLoading/ByDefaultForModels cop
+  - Betterment/UseGlobalStrictLoading/ForAssociations cop
+
 ## [1.1.0] - 2022-02-16
 ### Added
  - Betterment/NonStandardActions cop

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    betterlint (1.16.0)
+    betterlint (1.17.0)
       rubocop (~> 1.62.0)
       rubocop-graphql (~> 1.5.0)
       rubocop-performance (~> 1.21.0)

README.md

@@ -280,3 +280,20 @@ actions often indicates error handling (e.g. `422 Unprocessable Entity`).
 This cop requires you to explicitly provide an HTTP status code when rendering a response in the
 create, update, and destroy actions. When autocorrecting, this will automatically add
 `status: :unprocessable_entity` or `status: :ok` depending on what you're rendering.
+
+### Betterment/UseGlobalStrictLoading/ByDefaultForModels
+
+This cop identifies models where `self.strict_loading_by_default` is assigned to explicitly, and prefers that it be removed in favor of using the global strict loading settings.
+
+We use this to create a `.rubocop_todo.yml` file showing all the models that have been explicitly configured, so that we can "burn down" and lean more heavily on using global settings. We prefer to enable strict loading by default globally, but transioning a large Rails application to work in that state is difficult and must be broken down into smaller steps. Our workflow is to change all models to turn off strict loading, regenerate our rubocop todo file, then enable the flag globally. Then on a model-by-model basis we'll enable them one at a time, and fix all spec failures per model.
+
+This allows us two benefits:
+
+1. We can gradually change code used by older models to use strict loading without having to fix the world all at once.
+2. All new models will strict load by default, forcing us to specify the objcts to load up-front in our controllers and other call sites.
+
+### Betterment/UseGlobalStrictLoading/ForAssociations
+
+This cop identifies associations where `:strict_loading` is set explicitly, and prefers that it be removed in favor of using the global strict loading settings.
+
+This is related to the [Betterment/UseGlobalStrictLoading/ByDefaultForModels](#bettermentuseglobalstrictloadingbydefaultformodels) cop, but allows for more granular enablement and disablement of associations within a model. The intention is similar, in that we are using this cop to help "burn down" code to strict load, but it allows us to focus on the per-association level. Some models may have quite a lot of usage, so enabling it for a model might cause thousands of failures in the specs. In those cases we will disable all the associations, and then work through them one at a time until all code that uses the model strict loads.

betterlint.gemspec

@@ -5,7 +5,7 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
 
 Gem::Specification.new do |s|
   s.name = "betterlint"
-  s.version = "1.16.0"
+  s.version = "1.17.0"
   s.authors = ["Development"]
   s.email = ["[email protected]"]
   s.summary = "Betterment rubocop configuration"

config/default.yml

@@ -102,6 +102,14 @@ Betterment/UnsafeJob:
 Betterment/UnscopedFind:
   StyleGuide: '#bettermentunscopedfind'
 
+Betterment/UseGlobalStrictLoading/ByDefaultForModels:
+  Enabled: true
+  SafeAutoCorrect: false
+
+Betterment/UseGlobalStrictLoading/ForAssociations:
+  Enabled: true
+  SafeAutoCorrect: false
+
 FactoryBot/AssociationStyle:
   Enabled: false
 

lib/rubocop/cop/betterment.rb

@@ -1,27 +1,28 @@
 # frozen_string_literal: true
 
 require 'rubocop'
-require 'rubocop/cop/betterment/utils/parser'
-require 'rubocop/cop/betterment/utils/method_return_table'
-require 'rubocop/cop/betterment/utils/hardcoded_attribute'
+require 'rubocop/cop/betterment/active_job_performable'
+require 'rubocop/cop/betterment/allowlist_blocklist'
 require 'rubocop/cop/betterment/authorization_in_controller'
 require 'rubocop/cop/betterment/direct_delayed_enqueue'
 require 'rubocop/cop/betterment/dynamic_params'
-require 'rubocop/cop/betterment/unscoped_find'
-require 'rubocop/cop/betterment/unsafe_job'
-require 'rubocop/cop/betterment/timeout'
+require 'rubocop/cop/betterment/fetch_boolean'
+require 'rubocop/cop/betterment/hardcoded_id'
+require 'rubocop/cop/betterment/implicit_redirect_type'
 require 'rubocop/cop/betterment/internals_protection'
 require 'rubocop/cop/betterment/memoization_with_arguments'
 require 'rubocop/cop/betterment/non_standard_actions'
 require 'rubocop/cop/betterment/non_standard_controller'
+require 'rubocop/cop/betterment/redirect_status'
+require 'rubocop/cop/betterment/render_status'
+require 'rubocop/cop/betterment/server_error_assertion'
 require 'rubocop/cop/betterment/site_prism_loaded'
 require 'rubocop/cop/betterment/spec_helper_required_outside_spec_dir'
-require 'rubocop/cop/betterment/implicit_redirect_type'
-require 'rubocop/cop/betterment/active_job_performable'
-require 'rubocop/cop/betterment/allowlist_blocklist'
-require 'rubocop/cop/betterment/server_error_assertion'
-require 'rubocop/cop/betterment/hardcoded_id'
+require 'rubocop/cop/betterment/timeout'
+require 'rubocop/cop/betterment/unsafe_job'
+require 'rubocop/cop/betterment/unscoped_find'
+require 'rubocop/cop/betterment/use_global_strict_loading'
+require 'rubocop/cop/betterment/utils/hardcoded_attribute'
+require 'rubocop/cop/betterment/utils/method_return_table'
+require 'rubocop/cop/betterment/utils/parser'
 require 'rubocop/cop/betterment/vague_serialize'
-require 'rubocop/cop/betterment/fetch_boolean'
-require 'rubocop/cop/betterment/render_status'
-require 'rubocop/cop/betterment/redirect_status'

lib/rubocop/cop/betterment/use_global_strict_loading.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Betterment
+      module UseGlobalStrictLoading
+        # This cop ensures that `self.strict_loading_by_default = <any value>` is not set in ActiveRecord models.
+        class ByDefaultForModels < Base
+          extend AutoCorrector
+          include RangeHelp
+
+          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)
+            strict_loading_by_default_set?(node) do |method_call|
+              add_offense(method_call) do |corrector|
+                corrector.remove(method_call)
+              end
+            end
+          end
+        end
+
+        # This cop ensures that `strict_loading: <any value>` is not set in ActiveRecord associations.
+        class ForAssociations < Base
+          extend AutoCorrector
+          include RangeHelp
+
+          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)
+            association_with_strict_loading?(node) do |pair|
+              add_offense(node) do |corrector|
+                corrector.remove(range_with_surrounding_comma(range_with_surrounding_space(range: pair.source_range, side: :left), :left))
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/betterment/use_global_strict_loading/by_default_for_models_spec.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Betterment::UseGlobalStrictLoading::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
+
+        # Add explicit spaces to prevent editor from stripping them on-save
+        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
+             # Set to true to enable strict_loading_by_default
+          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
+
+        # Add explicit spaces to prevent editor from stripping them on-save
+        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
+
+        # Add explicit spaces to prevent editor from stripping them on-save
+        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

spec/rubocop/cop/betterment/use_global_strict_loading/for_associations_spec.rb

@@ -0,0 +1,113 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Betterment::UseGlobalStrictLoading::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
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations.
+            strict_loading: true, # preserve this comment
+            dependent: :destroy   # preserve this comment
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class MyModel < ApplicationRecord
+          has_many :posts,        # preserve this comment, # 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