From 1d3c39ddd2117fd12db095791925454340b91c29 Mon Sep 17 00:00:00 2001 From: Melody Universe Date: Sat, 2 May 2026 21:47:14 -0700 Subject: [PATCH] [Fix #1612] Add `Rails/ActiveStorageAttachedDependent` cop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flags `has_one_attached` / `has_many_attached` declarations whose `dependent:` value silently no-ops in ActiveStorage. Rails stores the kwarg on the reflection, but only the consumer `ActiveStorage::Attachment#purge_dependent_blob_later` reads it, and only with strict equality on `:purge_later`. Anything else (`true`, `nil`, `:destroy`, arbitrary symbols) leaves orphan blobs in storage when the parent record is destroyed. The cop does not autocorrect — every value other than `:purge_later` and `false` is runtime-equivalent (all silent no-ops), so the right replacement depends on the user's intent. `dependent: :purge` is reported at warning severity, citing rails/rails#36423. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../new_active_storage_attached_dependent.md | 1 + config/default.yml | 7 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rails.adoc | 140 ++++++++++++++++-- .../active_storage_attached_dependent.rb | 71 +++++++++ lib/rubocop/cop/rails_cops.rb | 1 + .../active_storage_attached_dependent_spec.rb | 66 +++++++++ 7 files changed, 278 insertions(+), 9 deletions(-) create mode 100644 changelog/new_active_storage_attached_dependent.md create mode 100644 lib/rubocop/cop/rails/active_storage_attached_dependent.rb create mode 100644 spec/rubocop/cop/rails/active_storage_attached_dependent_spec.rb diff --git a/changelog/new_active_storage_attached_dependent.md b/changelog/new_active_storage_attached_dependent.md new file mode 100644 index 0000000000..d7e2d943b2 --- /dev/null +++ b/changelog/new_active_storage_attached_dependent.md @@ -0,0 +1 @@ +* [#1612](https://github.com/rubocop/rubocop-rails/issues/1612): Add new `Rails/ActiveStorageAttachedDependent` cop that flags `has_one_attached` / `has_many_attached` declarations whose `dependent:` value silently no-ops in ActiveStorage. ([@melody-universe][]) diff --git a/config/default.yml b/config/default.yml index d956ea1118..86fa55f546 100644 --- a/config/default.yml +++ b/config/default.yml @@ -173,6 +173,13 @@ Rails/ActiveRecordOverride: Include: - '**/app/models/**/*.rb' +Rails/ActiveStorageAttachedDependent: + Description: >- + Flags `has_one_attached` / `has_many_attached` `dependent:` + values that silently no-op in ActiveStorage. + Enabled: pending + VersionAdded: '2.35' + Rails/ActiveSupportAliases: Description: >- Avoid ActiveSupport aliases of standard ruby methods: diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 7e64ca6efa..9c7bc64ebb 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -23,6 +23,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. * xref:cops_rails.adoc#railsactiverecordaliases[Rails/ActiveRecordAliases] * xref:cops_rails.adoc#railsactiverecordcallbacksorder[Rails/ActiveRecordCallbacksOrder] * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride] +* xref:cops_rails.adoc#railsactivestorageattacheddependent[Rails/ActiveStorageAttachedDependent] * xref:cops_rails.adoc#railsactivesupportaliases[Rails/ActiveSupportAliases] * xref:cops_rails.adoc#railsactivesupportonload[Rails/ActiveSupportOnLoad] * xref:cops_rails.adoc#railsaddcolumnindex[Rails/AddColumnIndex] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 2bc1af207e..46c9f413aa 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -378,6 +378,51 @@ end | Array |=== +[#railsactivestorageattacheddependent] +== Rails/ActiveStorageAttachedDependent + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| 2.35 +| - +|=== + +Flags `has_one_attached` / `has_many_attached` declarations whose +`dependent:` value silently no-ops in ActiveStorage. + +Rails stores `dependent:` verbatim on the reflection but only +`ActiveStorage::Attachment#purge_dependent_blob_later` consumes it, +with strict equality on `:purge_later`. Anything else (`true`, `nil`, +`:destroy`, arbitrary symbols) leaves orphan blobs in storage with +no warning when the parent record is destroyed. + +`:purge` is documented but not honored by the consumer today; +see https://github.com/rails/rails/issues/36423. + +[#examples-railsactivestorageattacheddependent] +=== Examples + +[source,ruby] +---- +# bad - silent no-op +has_one_attached :avatar, dependent: true + +# bad - silent no-op +has_many_attached :photos, dependent: :destroy + +# bad - documented but not honored, see rails/rails#36423 +has_one_attached :avatar, dependent: :purge + +# good +has_one_attached :avatar +has_one_attached :avatar, dependent: :purge_later +has_one_attached :avatar, dependent: false +---- + [#railsactivesupportaliases] == Rails/ActiveSupportAliases @@ -3042,7 +3087,7 @@ get :new, **options | Name | Default value | Configurable values | Include -| `+spec/**/*+`, `+test/**/*+` +| `+**/spec/**/*+`, `+**/test/**/*+` | Array |=== @@ -3303,7 +3348,7 @@ end | Name | Default value | Configurable values | Include -| `+spec/**/*.rb+`, `+test/**/*.rb+` +| `+**/spec/**/*.rb+`, `+**/test/**/*.rb+` | Array |=== @@ -3367,6 +3412,28 @@ class PostsController < ApplicationController end end +# bad +class PostsController < ApplicationController + def update + # ... + redirect_back_or_to root_path, alert: "Failed to update!" + end +end + +# good +# config/locales/en.yml +# en: +# posts: +# update: +# failure: "Failed to update!" + +class PostsController < ApplicationController + def update + # ... + redirect_back_or_to root_path, alert: t(".failure") + end +end + # bad class UserMailer < ApplicationMailer def welcome(user) @@ -4862,7 +4929,10 @@ a.blank? ? nil : a.foo # good a.presence&.foo +---- +[source,ruby] +---- # good a.present? ? a[1] : nil @@ -4871,7 +4941,12 @@ a[:key] = value if a.present? # good a.present? ? a > 1 : nil + +# good a <= 0 if a.present? + +# good +a << "bar" if a.present? ---- [#railspresent] @@ -5480,7 +5555,7 @@ end | Name | Default value | Configurable values | Include -| `+spec/**/*.rb+`, `+test/**/*.rb+` +| `+**/spec/**/*.rb+`, `+**/test/**/*.rb+` | Array |=== @@ -5867,12 +5942,15 @@ so you still have to use `JSON.parse(response.body)` there. ---- # bad JSON.parse(response.body) - -# bad +Nokogiri::HTML(response.body) +Nokogiri::HTML4(response.body) +Nokogiri::HTML5(response.body) Nokogiri::HTML.parse(response.body) - -# bad +Nokogiri::HTML4.parse(response.body) Nokogiri::HTML5.parse(response.body) +Nokogiri::HTML::Document.parse(response.body) +Nokogiri::HTML4::Document.parse(response.body) +Nokogiri::HTML5::Document.parse(response.body) # good response.parsed_body @@ -5885,7 +5963,7 @@ response.parsed_body | Name | Default value | Configurable values | Include -| `+spec/controllers/**/*.rb+`, `+spec/requests/**/*.rb+`, `+test/controllers/**/*.rb+`, `+test/integration/**/*.rb+` +| `+**/spec/controllers/**/*.rb+`, `+**/spec/requests/**/*.rb+`, `+**/test/controllers/**/*.rb+`, `+**/test/integration/**/*.rb+` | Array |=== @@ -6947,6 +7025,15 @@ NOTE: Requires Rails version 8.0 Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling. +In the following cases, `params[:key]` is treated as a key that is expected to be passed from the HTTP client, +and the cop detects it using the `expect` method. + +- Method calls on `params[:key]` without comparison methods +- Passing `params[:key]` as an argument to finder methods that raise on missing records +- Strong parameter methods using `require` or `permit` + +Other cases are not detected, as they are cases where `params[:key]` may not be passed from the HTTP client. + [#safety-railsstrongparametersexpect] === Safety @@ -6960,6 +7047,24 @@ strong parameter conventions. [source,ruby] ---- +# bad +params[:key].do_something + +# good +params.expect(:key).do_something + +# bad +Model.find(params[:id]) + +# good +Model.find(params.expect(:id)) + +# bad +Model.find_by!(key: params[:key]) + +# good +Model.find_by!(key: params.expect(:key)) + # bad params.require(:user).permit(:name, :age) params.permit(user: [:name, :age]).require(:user) @@ -7219,7 +7324,7 @@ end | Name | Default value | Configurable values | Include -| `+spec/**/*.rb+`, `+test/**/*.rb+` +| `+**/spec/**/*.rb+`, `+**/test/**/*.rb+` | Array |=== @@ -7626,10 +7731,27 @@ More can be added to the `Environments` config parameter. # bad Rails.env.proudction? Rails.env == 'proudction' +Rails.env != 'proudction' # good Rails.env.production? Rails.env == 'production' +Rails.env != 'production' +---- + +[source,ruby] +---- +# bad +case Rails.env +when 'proudction' + do_something +end + +# good +case Rails.env +when 'production' + do_something +end ---- [#configurable-attributes-railsunknownenv] diff --git a/lib/rubocop/cop/rails/active_storage_attached_dependent.rb b/lib/rubocop/cop/rails/active_storage_attached_dependent.rb new file mode 100644 index 0000000000..3789971eae --- /dev/null +++ b/lib/rubocop/cop/rails/active_storage_attached_dependent.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Flags `has_one_attached` / `has_many_attached` declarations whose + # `dependent:` value silently no-ops in ActiveStorage. + # + # Rails stores `dependent:` verbatim on the reflection but only + # `ActiveStorage::Attachment#purge_dependent_blob_later` consumes it, + # with strict equality on `:purge_later`. Anything else (`true`, `nil`, + # `:destroy`, arbitrary symbols) leaves orphan blobs in storage with + # no warning when the parent record is destroyed. + # + # `:purge` is documented but not honored by the consumer today; + # see https://github.com/rails/rails/issues/36423. + # + # @example + # # bad - silent no-op + # has_one_attached :avatar, dependent: true + # + # # bad - silent no-op + # has_many_attached :photos, dependent: :destroy + # + # # bad - documented but not honored, see rails/rails#36423 + # has_one_attached :avatar, dependent: :purge + # + # # good + # has_one_attached :avatar + # has_one_attached :avatar, dependent: :purge_later + # has_one_attached :avatar, dependent: false + class ActiveStorageAttachedDependent < Base + MSG = '`dependent: %s` is silently a no-op in ActiveStorage and ' \ + 'leaves orphan blobs in storage. Use `:purge_later` (default) or `false`.' + MSG_PURGE = '`dependent: :purge` is documented but only `:purge_later` is honored ' \ + 'by ActiveStorage today (rails/rails#36423). Use `:purge_later` (default).' + + RESTRICT_ON_SEND = %i[has_one_attached has_many_attached].freeze + + # @!method dependent_pair(node) + def_node_matcher :dependent_pair, <<~PATTERN + (send _ {:has_one_attached :has_many_attached} _ + (hash <$(pair (sym :dependent) _) ...>)) + PATTERN + + def on_send(node) + return unless (pair = dependent_pair(node)) + return if allowed_value?(pair.value) + + register_offense(pair) + end + alias on_csend on_send + + private + + def allowed_value?(value) + (value.sym_type? && value.value == :purge_later) || value.false_type? + end + + def register_offense(pair) + value = pair.value + if value.sym_type? && value.value == :purge + add_offense(pair, message: MSG_PURGE, severity: :warning) + else + add_offense(pair, message: format(MSG, value: value.source)) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index e3cea8a378..8388a4ab6c 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -18,6 +18,7 @@ require_relative 'rails/active_record_aliases' require_relative 'rails/active_record_callbacks_order' require_relative 'rails/active_record_override' +require_relative 'rails/active_storage_attached_dependent' require_relative 'rails/active_support_aliases' require_relative 'rails/active_support_on_load' require_relative 'rails/add_column_index' diff --git a/spec/rubocop/cop/rails/active_storage_attached_dependent_spec.rb b/spec/rubocop/cop/rails/active_storage_attached_dependent_spec.rb new file mode 100644 index 0000000000..b8f5de980b --- /dev/null +++ b/spec/rubocop/cop/rails/active_storage_attached_dependent_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::ActiveStorageAttachedDependent, :config do + it 'flags has_one_attached dependent: true' do + expect_offense(<<~RUBY) + class Foo + has_one_attached :bar, dependent: true + ^^^^^^^^^^^^^^^ `dependent: true` is silently a no-op in ActiveStorage and leaves orphan blobs in storage. Use `:purge_later` (default) or `false`. + end + RUBY + end + + it 'flags has_one_attached when dependent: nil' do + expect_offense(<<~RUBY) + class Foo + has_one_attached :bar, dependent: nil + ^^^^^^^^^^^^^^ `dependent: nil` is silently a no-op in ActiveStorage and leaves orphan blobs in storage. Use `:purge_later` (default) or `false`. + end + RUBY + end + + it 'flags has_one_attached when dependent: :purge at warning severity (cites rails/rails#36423)' do + expect_offense(<<~RUBY, severity: :warning) + class Foo + has_one_attached :bar, dependent: :purge + ^^^^^^^^^^^^^^^^^ `dependent: :purge` is documented but only `:purge_later` is honored by ActiveStorage today (rails/rails#36423). Use `:purge_later` (default). + end + RUBY + end + + it 'flags has_one_attached when dependent: is an arbitrary unrecognized symbol' do + expect_offense(<<~RUBY) + class Foo + has_one_attached :bar, dependent: :nope + ^^^^^^^^^^^^^^^^ `dependent: :nope` is silently a no-op in ActiveStorage and leaves orphan blobs in storage. Use `:purge_later` (default) or `false`. + end + RUBY + end + + it 'flags has_many_attached dependent: true' do + expect_offense(<<~RUBY) + class Foo + has_many_attached :bars, dependent: true + ^^^^^^^^^^^^^^^ `dependent: true` is silently a no-op in ActiveStorage and leaves orphan blobs in storage. Use `:purge_later` (default) or `false`. + end + RUBY + end + + it 'allows documented dependent values' do + [':purge_later', 'false'].each do |value| + expect_no_offenses(<<~RUBY) + class Foo + has_one_attached :bar, dependent: #{value} + end + RUBY + end + end + + it 'allows has_one_attached without a dependent: argument' do + expect_no_offenses(<<~RUBY) + class Foo + has_one_attached :bar + end + RUBY + end +end