Class: RuboCop::Cop::DevDoc::Rails::NoCollectionIdsWriterInController

Inherits:
Base
  • Object
show all
Defined in:
lib/rubocop/cop/dev_doc/rails/no_collection_ids_writer_in_controller.rb

Overview

Controllers must not call association *_ids= writers; the write belongs behind the model method or form-object PORO that owns the operation's validations.

Rationale

collection_ids= looks like plain attribute assignment but is not: on a persisted record it writes the join table IMMEDIATELY — no save call, no validations, no surrounding transaction. In a controller that produces two failure modes:

  • Validation asymmetry. The create path routes through a form object whose validations guard the association (say, "at least one tag"); a later-added update action assigns record.tag_ids = ... directly, and every one of those guards silently stops existing on the second write path. Nothing fails loudly — the update simply never consults the rules that create enforces.
  • Partial writes. The association mutates even when the surrounding operation fails afterwards: clear record.tag_ids = [] and then fail to save a status flag, and the two halves are left inconsistent with no transaction to roll the join rows back.

Both are the data-saving-orchestration placement rule (see the orchestration best-practice doc, category 1) wearing an assignment costume: which rows must land together, and under which guards, is domain knowledge. Move the write into a model method or the form-object PORO that owns the operation's validations, so updates travel the same validation path as creates.

❌ Hand-rolled update path — join rows written before save!, guards skipped
def update
@article.category = update_params[:category]
@article.tag_ids = checked_tag_ids
@article.save!
end

✔️ The model method owns the write and its guards
def update
@article.update_with_tags!(update_params, tag_ids: checked_tag_ids)
end

Relationship with DevDoc/Rails/AvoidBypassingValidation

Same bypass family, different signal: that cop matches a fixed list of ActiveRecord method names repo-wide, while *_ids= is a NAME PATTERN that is perfectly legitimate on form objects (common under app/models/). The pattern is only dependable where a direct ids write is either a validation bypass or misplaced orchestration — controllers — so it lives in this scoped cop instead.

Exception

The receiver's type is statically invisible, so this cop flags every *_ids= spelling, including ones that persist nothing: an ActiveModel form object, a not-yet-saved record (its ids apply at save time, under validations), or a non-AR object such as a presenter or OpenStruct. Those are ordinary params→object binding (orchestration category 2) — disable inline, naming why nothing persists:

# rubocop:disable DevDoc/Rails/NoCollectionIdsWriterInController
# -- ArticleImport is an ActiveModel PORO; nothing persists

# rubocop:disable DevDoc/Rails/NoCollectionIdsWriterInController
# -- @article is built in this action and unsaved; ids apply at save,
#    under validations

NOTE: Spellings with the same immediate-write behavior that this cop cannot see, for reviewers: the plain collection writer (record.tags = [...]) — indistinguishable from an attribute write — a STANDALONE assign_attributes(tag_ids: [...]) on a persisted record (mass assignment invokes the same writer, with no transaction around it), and dynamic dispatch (record.send(:tag_ids=, x)). By contrast, update/update! with an ids key is NOT in this class: Rails wraps assign_attributes + save in a transaction precisely so those join-row writes roll back when validations fail (see the comment inside ActiveRecord::Persistence#update!).

Examples:

# bad
@article.tag_ids = checked_tag_ids

# bad
article&.tag_ids = []

# bad — op-assign calls the same writer
@article.tag_ids += [tag.id]

# good — the model method owns the write and its validations
@article.update_with_tags!(update_params, tag_ids: checked_tag_ids)

Constant Summary collapse

MSG = 
'`%<method>s` in a controller — on a persisted record a collection `_ids=` writer ' \
'updates the join table immediately, skipping the owner\'s save and validations. Move the ' \
'write into the model method or form-object PORO that owns the operation\'s validations.'.freeze
IDS_WRITER =

Dynamic name pattern, so RESTRICT_ON_SEND cannot apply (it is static); the filter runs inside on_send instead.

/\A\w+_ids=\z/

Instance Method Summary collapse

Instance Method Details

#on_op_asgn(node) ⇒ Object Also known as: on_or_asgn, on_and_asgn

Op-assigns (record.tag_ids += [x], ||=, &&=) call the same writer but parse as op_asgn/or_asgn/and_asgn nodes whose inner send is the READER (tag_ids, no =), so on_send never sees a match.



114
115
116
117
118
119
120
121
122
 
# File 'lib/rubocop/cop/dev_doc/rails/no_collection_ids_writer_in_controller.rb', line 114

def on_op_asgn(node)
  lhs = node.children.first
  return unless lhs.type?(:send, :csend)

  writer = "#{lhs.method_name}="
  return unless IDS_WRITER.match?(writer)

  add_offense(lhs.loc.selector, message: format(MSG, method: writer))
end

#on_send(node) ⇒ Object Also known as: on_csend 



100
101
102
103
104
105
 
# File 'lib/rubocop/cop/dev_doc/rails/no_collection_ids_writer_in_controller.rb', line 100

def on_send(node)
  return unless node.assignment_method?
  return unless IDS_WRITER.match?(node.method_name.to_s)

  add_offense(node.loc.selector, message: format(MSG, method: node.method_name))
end