charkost / prosopite

:mag: Rails N+1 queries auto-detection with zero false positives / false negatives
Apache License 2.0
1.53k stars 46 forks source link

False negative when using ActiveRecord::Associations::Preloader #62

Closed tientt-holistics closed 1 year ago

tientt-holistics commented 1 year ago

I found that in the source code if that Prosopite will automatic allow caller with ActiveRecord::Associations::Preloader

DEFAULT_ALLOW_LIST = %w(active_record/associations/preloader active_record/validations/uniqueness)

This could lead in false positive if a user manually uses ActiveRecord::Associations::Preloader to eager loading association.

datbth commented 1 year ago

Example false negative case:

preloader = ActiveRecord::Associations::Preloader.new
all_legs = Chair.last(20).map do |chair|
  preloader.preload(chair, :legs)
  chair.legs
end

Current prosopite behavior: Ignore the N+1 issue, according to https://github.com/charkost/prosopite/commit/b7bf180dce18a51807185759fa6957fcb1413a05

charkost commented 1 year ago

Nice catch, thanks for reporting @tientt-holistics @datbth

datbth commented 1 year ago

FYI, we (@tientt-holistics and I) decided that the original issue https://github.com/charkost/prosopite/issues/28 can be considered true positive.
Instead of querying Account 2 times, we could implement better preloading logic (instead of using default Rails Preloader):

account_ids = account_transfers.map(&:source_id) + account_transfers.map(&:target_id)
accounts = Accounts.where(id: account_ids).to_h { |acc| [acc.id, acc] }
account_transfer.each do |af|
  af.association(:source).target = accounts[af.source_id]
  af.association(:target).target = accounts[af.target_id]
end

And hence currently we are implementing our own preloader, and are applying this monkey-patch:

Prosopite.const_set(:DEFAULT_ALLOW_LIST, [])