False positive when the receiver of all is not an ActiveRecord object

[masato-bkn]

As mentioned in this comment, Rails/RedundantActiveRecordAllMethod produces a false positive when the receiver of all is not an ActiveRecord object.

Expected behavior

The cop should ignore cases where the receiver of all is not an ActiveRecord object.

Actual behavior

ActiveSupport::TimeZone.all.first
                        ^^^
ActionMailer::Preview.all.first
                      ^^^

Steps to reproduce the problem

Run rubocop on the following code:

ActiveSupport::TimeZone.all.first
ActionMailer::Preview.all.first

$ bundle exec rubocop --only Rails/RedundantActiveRecordAllMethod ./app/false-positive.rb
Inspecting 1 file
C

Offenses:

app/false-positive.rb:2:25: C: [Correctable] Rails/RedundantActiveRecordAllMethod: Redundant all detected.
ActiveSupport::TimeZone.all.first
                        ^^^
app/false-positive.rb:3:23: C: [Correctable] Rails/RedundantActiveRecordAllMethod: Redundant all detected.
ActionMailer::Preview.all.first
                      ^^^

RuboCop version

$ bundle ex rubocop -V
1.56.0 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 3.0.0) [x86_64-darwin18]
  - rubocop-rails 2.21.1

[fatkodima]

Rubocop performs static analysis of the code (does not run the code), so there is no way to know if the receiver is a ActiveRecord model or not. And better the rubocop:disable used in this case.

[jpcody]

Isn't this code, which was introduced here determining whether it's an AR model?

[jorg-vr]

We encountered the same issue using Docker::Container.all from https://github.com/upserve/docker-api
If it is not possible to limit the rule to active record models, it should be removed. As there are too many potential false positives. (.all is a quite generic function name)

[koic]

@jorg-vr Since this cop is already marked as unsafe, there will be limited ways to prevent false positives for static analysis. Can you provide a specific code example where false positives occurs with Docker::Container.all?

[koic]

Ah, I saw the backlink:
dodona-edu/dodona#4987

That false positive could be resolved.

[jpcody]

On my end, I have two instances: one where I'm iterating over sidekiq queues and another where I'm calling an activehash-backed model. Neither of these inherit from ApplicationRecord or ActiveRecord::Base, so I was surprised to find them called out as positive based on the above linked code and PR.

def queue_too_latent?(queue_name, threshold_seconds)
  queue = Sidekiq::Queue.all.find { |q| q.name == queue_name }
  latency = queue&.latency || 0

  latency > threshold_seconds
end

Hazmat::SystemClassification.all.each do |sc|
  …
end

[koic]

I haven't reviewed all cases for ActiveRecord::Querying::QUERYING_METHODS, but such scenarios will likely be resolved in the following PR:
#1126