Merge pull request #1131 from MITLibraries/bullet

Detect N+1 errors in development

Author: JPrevost

Gemfile

@@ -41,6 +41,7 @@ group :production do
 end
 
 group :development, :test do
+  gem 'bullet'
   gem 'byebug'
   gem 'capybara'
   gem 'selenium-webdriver'

Gemfile.lock

@@ -136,6 +136,9 @@ GEM
     bootsnap (1.16.0)
       msgpack (~> 1.2)
     builder (3.2.4)
+    bullet (7.0.7)
+      activesupport (>= 3.0.0)
+      uniform_notifier (~> 1.11)
     byebug (11.1.3)
     cancancan (3.5.0)
     capybara (3.38.0)
@@ -385,6 +388,7 @@ GEM
       unf_ext
     unf_ext (0.0.8.2)
     unicode-display_width (2.4.2)
+    uniform_notifier (1.16.0)
     warden (1.2.9)
       rack (>= 2.0.9)
     web-console (4.2.0)
@@ -410,6 +414,7 @@ DEPENDENCIES
   aws-sdk-rails
   aws-sdk-s3
   bootsnap
+  bullet
   byebug
   cancancan
   capybara

app/controllers/registrar_controller.rb

@@ -27,7 +27,7 @@ def show
   end
 
   def list_registrar
-    @registrars = Registrar.all
+    @registrars = Registrar.with_attached_graduation_list.includes([:user]).all
     @jobs = Delayed::Job.where(queue: 'default')
   end
 

app/controllers/report_controller.rb

@@ -31,12 +31,14 @@ def empty_theses
   end
 
   def expired_holds
-    @list = Hold.active_or_expired.ends_today_or_before.order(:date_end)
+    @list = Hold.active_or_expired.ends_today_or_before.order(:date_end).includes([:thesis, {
+                                                                                    thesis: %i[authors departments]
+                                                                                  }])
   end
 
   def files
     report = Report.new
-    theses = Thesis.all
+    theses = Thesis.all.with_attached_files.includes(%i[authors departments])
     @terms = report.extract_terms theses
     subset = filter_theses_by_term theses
     @list = report.list_unattached_files subset
@@ -69,7 +71,7 @@ def student_submitted_theses
     @this_term = 'all terms'
     @this_term = term.in_time_zone('Eastern Time (US & Canada)').strftime('%b %Y') if term != 'all'
     report = Report.new
-    theses = Thesis.all
+    theses = Thesis.all.includes([:versions])
     @terms = report.extract_terms theses
     subset = filter_theses_by_term theses
     @list = report.list_student_submitted_metadata subset
@@ -79,7 +81,7 @@ def holds_by_source
     term = params[:graduation] ? params[:graduation].to_s : 'all'
     @this_term = 'all terms'
     @this_term = term.in_time_zone('Eastern Time (US & Canada)').strftime('%b %Y') if term != 'all'
-    holds = Hold.all.includes(:thesis).includes(:hold_source).includes(thesis: :users).includes(thesis: :authors)
+    holds = Hold.all.includes([:thesis, :hold_source, { thesis: [:authors, { authors: :user }] }])
     @terms = Report.new.extract_terms holds
     @hold_sources = HoldSource.pluck(:source).uniq.sort
     term_filtered = filter_holds_by_term holds

app/controllers/thesis_controller.rb

@@ -40,19 +40,20 @@ def deduplicate
     # Get array of defined terms where theses have coauthors
     @terms = defined_terms Thesis.where.not('coauthors = ?', '')
     # Filter relevant theses by selected term from querystring
-    @thesis = filter_theses_by_term Thesis.where.not('coauthors = ?', '')
+    @thesis = filter_theses_by_term Thesis.where.not('coauthors = ?', '').includes(%i[departments users])
   end
 
   def publication_statuses
     @terms = defined_terms Thesis.all
     @publication_statuses = Thesis.all.pluck(:publication_status).uniq.sort
     # Filter relevant theses by selected term from querystring
-    term_filtered = filter_theses_by_term Thesis.all.includes(:degrees, :departments, :users)
+    term_filtered = filter_theses_by_term Thesis.all.includes(:degrees, :departments,
+                                                              :users).includes(degrees: :degree_type)
     @thesis = filter_theses_by_publication_status term_filtered
   end
 
   def edit
-    @thesis = Thesis.find(params[:id])
+    @thesis = Thesis.includes([degrees: :degree_type]).find(params[:id])
     @thesis.association(:advisors).add_to_target(Advisor.new) if @thesis.advisors.count.zero?
   end
 
@@ -79,8 +80,9 @@ def select
     # Get array of defined terms where unpublished theses have files attached
     @terms = defined_terms Thesis.joins(:files_attachments).group(:id).where('publication_status != ?', 'Published')
     # Filter relevant theses by selected term from querystring
-    @thesis = filter_theses_by_term Thesis.joins(:files_attachments).group(:id).where('publication_status != ?',
-                                                                                      'Published')
+    @thesis = filter_theses_by_term Thesis.joins(:files_attachments).includes(%i[departments users]).group(:id).where(
+      'publication_status != ?', 'Published'
+    )
   end
 
   def show
@@ -98,7 +100,7 @@ def start
   end
 
   def update
-    @thesis = Thesis.find(params[:id])
+    @thesis = Thesis.includes([authors: :user]).find(params[:id])
     if @thesis.update(thesis_params)
       flash[:success] = "#{@thesis.title} has been updated."
       ReceiptMailer.receipt_email(@thesis, current_user).deliver_later
@@ -109,7 +111,9 @@ def update
   end
 
   def process_theses
-    @thesis = Thesis.find(params[:id])
+    @thesis = Thesis.with_attached_files.includes([:departments, {
+                                                    authors: [:user], degrees: [:degree_type]
+                                                  }]).find(params[:id])
   end
 
   def process_theses_update
@@ -135,7 +139,7 @@ def process_theses_update
   end
 
   def proquest_export_preview
-    @theses = Thesis.ready_for_proquest_export
+    @theses = Thesis.includes([authors: :user]).ready_for_proquest_export
   end
 
   # TODO: we need to generate and send a budget report CSV for partially harvested theses (spec TBD).

app/controllers/transfer_controller.rb

@@ -51,18 +51,19 @@ def files
   end
 
   def select
-    @transfers = Transfer.all.with_attached_files.includes(:user).includes(:department)
+    @transfers = Transfer.all.includes(:user).includes(:department)
   end
 
   def show
     # Load the details of the requested Transfer record
-    @transfer = Transfer.find(params[:id])
+    @transfer = Transfer.with_attached_files.find(params[:id])
 
     # Load the Thesis records for the period covered by this Transfer (the
     # graduation month/year, and the department)
     @theses = Thesis.where('grad_date = ?', @transfer.grad_date)
     @theses = @theses.includes(:departments).where('departments.name_dw = ?',
                                                    @transfer.department.name_dw).references(:departments)
+    @theses.with_attached_files
   end
 
   private

app/models/report.rb

@@ -15,11 +15,11 @@ def card_files(collection, term)
       'value' => subset.pluck(:id).uniq.count,
       'verb' => 'has',
       'label' => 'files attached',
-      'note' => 'Only theses with a status of "Not ready for publication" and "Publication review" will be visible '\
+      'note' => 'Only theses with a status of "Not ready for publication" and "Publication review" will be visible ' \
                 'in the processing queue.',
       'link' => {
         'url' => url_helpers.thesis_select_path(graduation: term),
-        'text' => "See #{subset.where('publication_status != ?', 'Published').pluck(:id).uniq.count} unpublished "\
+        'text' => "See #{subset.where('publication_status != ?', 'Published').pluck(:id).uniq.count} unpublished " \
                   'theses in processing queue'
       }
     }
@@ -235,8 +235,8 @@ def data_authors_not_graduated
     row_data = {}
     terms = Thesis.all.pluck(:grad_date).uniq.sort
     terms.each do |term|
-      row_data[term] = Thesis.with_files.where('grad_date = ?', term).includes(authors: :user).includes(:departments)
-                             .reject(&:authors_graduated?).uniq.count
+      row_data[term] =
+        Thesis.with_files.where('grad_date = ?', term).includes([:authors]).reject(&:authors_graduated?).uniq.count
     end
     {
       label: 'Authors not graduated',
@@ -373,7 +373,7 @@ def table_copyright(collection)
     table_populate_defaults result, Copyright.pluck(:holder)
     {
       'title' => 'Thesis counts by copyright',
-      'summary' => 'This table presents a summary of thesis records by their copyright status. The second column '\
+      'summary' => 'This table presents a summary of thesis records by their copyright status. The second column ' \
                    'names the copyright holder, while the first column shows how many records have that copyright.',
       'column' => 'Copyright holder',
       'data' => result
@@ -385,10 +385,10 @@ def table_department(collection)
     table_populate_defaults result, Department.pluck(:name_dw)
     {
       'title' => 'Thesis counts by department',
-      'summary' => 'This table presents a summary of which departments have how many theses for the selected term. '\
-                   'The second column shows the programs at MIT which grant degrees, while the first column shows how '\
-                   'many theses have come from that program during this period.',
-      'note' => 'Please note: total theses indicated by this table may be greater than the overall number of theses '\
+      'summary' => 'This table presents a summary of which departments have how many theses for the selected term. ' \
+                   'The second column shows the programs at MIT which grant degrees, while the first column shows ' \
+                   'how many theses have come from that program during this period.',
+      'note' => 'Please note: total theses indicated by this table may be greater than the overall number of theses ' \
                 'because some theses have multiple departments.',
       'column' => 'Department',
       'data' => result
@@ -407,10 +407,10 @@ def table_license(collection)
     table_populate_defaults result, License.pluck(:display_description)
     {
       'title' => 'Thesis counts by Creative Commons license',
-      'summary' => 'This table presents a summary of which Creative Commons license has been selected by the author, '\
-                   'for those theses for which the author retains copyright. The second column gives the specific CC '\
+      'summary' => 'This table presents a summary of which Creative Commons license has been selected by the author, ' \
+                   'for those theses for which the author retains copyright. The second column gives the specific CC ' \
                    'license selected, while the first column shows how many theses have selected it.',
-      'note' => 'Please note: theses for which the author does not claim copyright will have "Undefined" in this '\
+      'note' => 'Please note: theses for which the author does not claim copyright will have "Undefined" in this ' \
                 'field.',
       'column' => 'License',
       'data' => result
@@ -438,7 +438,7 @@ def table_status(collection)
     table_populate_defaults result, Thesis.publication_statuses
     {
       'title' => 'Thesis counts by publication status',
-      'summary' => 'This table presents a summary of thesis records by their publication status. The second column '\
+      'summary' => 'This table presents a summary of thesis records by their publication status. The second column ' \
                    'gives the status, while the first column gives how many records have that status.',
       'column' => 'Publication status',
       'data' => result

config/environments/development.rb

@@ -1,6 +1,15 @@
 require "active_support/core_ext/integer/time"
 
 Rails.application.configure do
+  config.after_initialize do
+    Bullet.enable        = true
+    Bullet.alert         = false
+    Bullet.bullet_logger = false
+    Bullet.console       = true
+    Bullet.rails_logger  = true
+    Bullet.add_footer    = true
+  end
+
   # Settings specified here will take precedence over those in config/application.rb.
 
   # In the development environment your application's code is reloaded any time

config/environments/test.rb

@@ -6,6 +6,18 @@
 # and recreated between test runs. Don't rely on the data there!
 
 Rails.application.configure do
+
+  # Bullet configuration: currently disabled because we can't currently fix all issues
+  # This configuration block is still useful to allow manually enabling detection locally
+  # to investigate problems.
+  config.after_initialize do
+    Bullet.enable        = true
+    Bullet.bullet_logger = true
+    Bullet.raise         = false # raise an error if n+1 query occurs
+    Bullet.unused_eager_loading_enable = false
+    Bullet.counter_cache_enable        = false
+  end
+
   # Settings specified here will take precedence over those in config/application.rb.
 
   ENV['SP_PRIVATE_KEY'] = ''