From 660a05afc93ad50c55c825d4f8217ce639c03021 Mon Sep 17 00:00:00 2001 From: Trevor Broaddus Date: Mon, 14 Jun 2021 12:32:56 -0500 Subject: [PATCH] Bug: Ensure HABTM associations are not incorrectly labeled n+1 Prior to this commit, unless your HABTM join table excluded the `id` primary key column, it would be erroneously marked as a potential n+1 query. Other HABTM queries that had join tables _without_ an `id` column were properly ignored. This exclusion _seems_ to be a side-effect of excluding objects that don't have primary keys/id columns in an effort to ignore non-persisted objects (stemming from [this commit](https://github.com/flyerhzm/bullet/blob/2c8a6d26f4a11bc8b3af5ff7da815401049732d4/lib/bullet/detector/n_plus_one_query.rb#L37)). This commit, instead, explicitly ignores HABTM models when checking for potential objects that could have a n+1 impact since they are join tables that typically aren't queried for specifically but instead are joined on. --- lib/bullet/detector/n_plus_one_query.rb | 1 + lib/bullet/stack_trace_filter.rb | 1 + spec/integration/active_record/association_spec.rb | 9 +++++++++ spec/models/deal.rb | 5 +++++ spec/models/post.rb | 1 + spec/support/sqlite_seed.rb | 12 ++++++++++++ test.sh | 1 + 7 files changed, 30 insertions(+) create mode 100644 spec/models/deal.rb diff --git a/lib/bullet/detector/n_plus_one_query.rb b/lib/bullet/detector/n_plus_one_query.rb index 38e01136..adce09d7 100644 --- a/lib/bullet/detector/n_plus_one_query.rb +++ b/lib/bullet/detector/n_plus_one_query.rb @@ -35,6 +35,7 @@ def add_possible_objects(object_or_objects) objects = Array(object_or_objects) return if objects.map(&:bullet_primary_key_value).compact.empty? + return if objects.all? { |obj| obj.class.name =~ /^HABTM_/ } Bullet.debug( 'Detector::NPlusOneQuery#add_possible_objects', diff --git a/lib/bullet/stack_trace_filter.rb b/lib/bullet/stack_trace_filter.rb index fa861db4..d652fd11 100644 --- a/lib/bullet/stack_trace_filter.rb +++ b/lib/bullet/stack_trace_filter.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require "bundler" module Bullet module StackTraceFilter diff --git a/spec/integration/active_record/association_spec.rb b/spec/integration/active_record/association_spec.rb index f03ea938..04fa0a3f 100644 --- a/spec/integration/active_record/association_spec.rb +++ b/spec/integration/active_record/association_spec.rb @@ -401,6 +401,15 @@ end describe Bullet::Detector::Association, 'has_and_belongs_to_many' do + context 'posts <=> deals' do + it 'should detect preload associations with join tables that have identifier' do + Post.includes(:deals).each { |post| post.deals.map(&:name) } + Bullet::Detector::UnusedEagerLoading.check_unused_preload_associations + expect(Bullet::Detector::Association).not_to be_has_unused_preload_associations + + expect(Bullet::Detector::Association).to be_completely_preloading_associations + end + end context 'students <=> teachers' do it 'should detect non preload associations' do Student.all.each { |student| student.teachers.map(&:name) } diff --git a/spec/models/deal.rb b/spec/models/deal.rb new file mode 100644 index 00000000..94b0fe57 --- /dev/null +++ b/spec/models/deal.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Deal < ActiveRecord::Base + has_and_belongs_to_many :posts +end diff --git a/spec/models/post.rb b/spec/models/post.rb index 741c8f7e..31168c08 100644 --- a/spec/models/post.rb +++ b/spec/models/post.rb @@ -4,6 +4,7 @@ class Post < ActiveRecord::Base belongs_to :category, inverse_of: :posts belongs_to :writer has_many :comments, inverse_of: :post + has_and_belongs_to_many :deals validates :category, presence: true diff --git a/spec/support/sqlite_seed.rb b/spec/support/sqlite_seed.rb index 2d285bc0..92550236 100644 --- a/spec/support/sqlite_seed.rb +++ b/spec/support/sqlite_seed.rb @@ -21,6 +21,13 @@ def seed_db post2 = category2.posts.create(name: 'second', writer: writer2) post3 = category2.posts.create(name: 'third', writer: writer2) + deal1 = Deal.new(name: 'Deal 1') + deal1.posts << post1 + deal1.posts << post2 + deal2 = Deal.new(name: 'Deal 2') + post1.deals << deal1 + post1.deals << deal2 + comment1 = post1.comments.create(name: 'first', author: writer1) comment2 = post1.comments.create(name: 'first2', author: writer1) comment3 = post1.comments.create(name: 'first3', author: writer1) @@ -156,6 +163,11 @@ def setup_db t.column :hotel_id, :integer end + create_table :deals_posts do |t| + t.column :deal_id, :integer + t.column :post_id, :integer + end + create_table :documents do |t| t.string :name t.string :type diff --git a/test.sh b/test.sh index e2299461..7c31a68e 100755 --- a/test.sh +++ b/test.sh @@ -1,5 +1,6 @@ #bundle update rails && bundle exec rspec spec #BUNDLE_GEMFILE=Gemfile.mongoid bundle update mongoid && BUNDLE_GEMFILE=Gemfile.mongoid bundle exec rspec spec +BUNDLE_GEMFILE=Gemfile.rails-6.1 bundle && BUNDLE_GEMFILE=Gemfile.rails-6.1 bundle exec rspec spec BUNDLE_GEMFILE=Gemfile.rails-6.0 bundle && BUNDLE_GEMFILE=Gemfile.rails-6.0 bundle exec rspec spec BUNDLE_GEMFILE=Gemfile.rails-5.2 bundle && BUNDLE_GEMFILE=Gemfile.rails-5.2 bundle exec rspec spec BUNDLE_GEMFILE=Gemfile.rails-5.1 bundle && BUNDLE_GEMFILE=Gemfile.rails-5.1 bundle exec rspec spec