Merge pull request #655 from quintinm-dev/detect-bad-counts

Detect n+1 count queries from ActiveRecord::Associations::CollectionProxy#count

CHANGELOG.md

@@ -1,6 +1,8 @@
 ## Next Release
 
 * Added `always_append_html_body` option, so the html snippet is always included even if there are no notifications
+* Added detection of n+1 count queries from `count` method
+* Changed the counter cache notification title to recommend using `size`
 
 ## 7.0.7 (03/01/2023)
 

lib/bullet/active_record4.rb

@@ -176,6 +176,15 @@ def has_cached_counter?(reflection = reflection())
           result
         end
       end
+
+      ::ActiveRecord::Associations::CollectionProxy.class_eval do
+        def count
+          if Bullet.start?
+            Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+          end
+          super
+        end
+      end
     end
   end
 end

lib/bullet/active_record41.rb

@@ -168,6 +168,15 @@ def count_records
           origin_count_records
         end
       end
+
+      ::ActiveRecord::Associations::CollectionProxy.class_eval do
+        def count
+          if Bullet.start?
+            Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+          end
+          super
+        end
+      end
     end
   end
 end

lib/bullet/active_record42.rb

@@ -233,6 +233,15 @@ def count_records
           origin_count_records
         end
       end
+
+      ::ActiveRecord::Associations::CollectionProxy.class_eval do
+        def count
+          if Bullet.start?
+            Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+          end
+          super
+        end
+      end
     end
   end
 end

lib/bullet/active_record5.rb

@@ -260,6 +260,17 @@ def count_records
           end
         end
       )
+
+      ::ActiveRecord::Associations::CollectionProxy.prepend(
+        Module.new do
+          def count
+            if Bullet.start? && !proxy_association.is_a?(::ActiveRecord::Associations::ThroughAssociation)
+              Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+            end
+            super
+          end
+        end
+      )
     end
   end
 end

lib/bullet/active_record52.rb

@@ -242,6 +242,17 @@ def count_records
           end
         end
       )
+
+      ::ActiveRecord::Associations::CollectionProxy.prepend(
+        Module.new do
+          def count
+            if Bullet.start? && !proxy_association.is_a?(::ActiveRecord::Associations::ThroughAssociation)
+              Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+            end
+            super
+          end
+        end
+      )
     end
   end
 end

lib/bullet/active_record60.rb

@@ -269,6 +269,17 @@ def count_records
           end
         end
       )
+
+      ::ActiveRecord::Associations::CollectionProxy.prepend(
+        Module.new do
+          def count
+            if Bullet.start? && !proxy_association.is_a?(::ActiveRecord::Associations::ThroughAssociation)
+              Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+            end
+            super
+          end
+        end
+      )
     end
   end
 end

lib/bullet/active_record61.rb

@@ -269,6 +269,17 @@ def count_records
           end
         end
       )
+
+      ::ActiveRecord::Associations::CollectionProxy.prepend(
+        Module.new do
+          def count
+            if Bullet.start? && !proxy_association.is_a?(::ActiveRecord::Associations::ThroughAssociation)
+              Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+            end
+            super
+          end
+        end
+      )
     end
   end
 end

lib/bullet/active_record70.rb

@@ -279,6 +279,17 @@ def count_records
           end
         end
       )
+
+      ::ActiveRecord::Associations::CollectionProxy.prepend(
+        Module.new do
+          def count
+            if Bullet.start? && !proxy_association.is_a?(::ActiveRecord::Associations::ThroughAssociation)
+              Bullet::Detector::CounterCache.add_counter_cache(proxy_association.owner, proxy_association.reflection.name)
+            end
+            super
+          end
+        end
+      )
     end
   end
 end

lib/bullet/notification/counter_cache.rb

@@ -8,7 +8,7 @@ def body
       end
 
       def title
-        'Need Counter Cache'
+        'Need Counter Cache with Active Record size'
       end
     end
   end

spec/bullet/notification/counter_cache_spec.rb

@@ -8,7 +8,7 @@ module Notification
       subject { CounterCache.new(Post, %i[comments votes]) }
 
       it { expect(subject.body).to eq('  Post => [:comments, :votes]') }
-      it { expect(subject.title).to eq('Need Counter Cache') }
+      it { expect(subject.title).to eq('Need Counter Cache with Active Record size') }
     end
   end
 end

spec/integration/counter_cache_spec.rb

@@ -64,5 +64,29 @@
         expect(Bullet.collected_counter_cache_notifications).to be_empty
       end
     end
+
+    describe 'with count' do
+      it 'should need counter cache' do
+        Country.all.each { |country| country.cities.count }
+        expect(Bullet.collected_counter_cache_notifications).not_to be_empty
+      end
+
+      it 'should notify even with counter cache' do
+        Person.all.each { |person| person.pets.count }
+        expect(Bullet.collected_counter_cache_notifications).not_to be_empty
+      end
+
+      if ActiveRecord::VERSION::MAJOR > 4
+        it 'should not need counter cache for has_many through' do
+          Client.all.each { |client| client.firms.count }
+          expect(Bullet.collected_counter_cache_notifications).to be_empty
+        end
+      else
+        it 'should need counter cache for has_many through' do
+          Client.all.each { |client| client.firms.count }
+          expect(Bullet.collected_counter_cache_notifications).not_to be_empty
+        end
+      end
+    end
   end
 end