Add bullet gem to catch n+1 queries #5961

squanto · 2025-04-25T16:50:46Z

Link to pivotal/JIRA issue

https://codeforamerica.atlassian.net/browse/FYST-1825

Is PM acceptance required? (delete one)

No - merge after code review approval

Reminder: merge main into this branch and get green tests before merging to main

What was done?

introduces the bullet gem
Fix various errors present, then disabling the gem for future usage
(This is a smaller change set from [draft]Add bullet gem to dev and test envs #5603 , to hopefully have no side effects)

How to test?

verify test coverage
ensure existing changes don't change behavior
write an n+1 query to double check that it fails the build with bullet enabled

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

github-actions · 2025-04-25T16:55:01Z

Heroku app: https://gyr-review-app-5961-6480d40149ad.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5961 (optionally add --tail)

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

Copilot

Pull Request Overview

This PR adds the bullet gem to catch n+1 queries, addresses various errors in the code, and then disables the gem for future use. Key changes include adding eager loading to various ActiveRecord queries to optimize database access and modifying or removing unnecessary includes.

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
app/views/shared/_tax_return_list.html.erb	Added eager loading for assigned_user to tax_returns
app/services/search_indexer.rb	Removed extra eager loads, now only eager loading tax_returns
app/presenters/message_presenter.rb	Added includes for documents on incoming messages
app/models/client.rb	Updated eager loading within scopes
app/lib/client_sorter.rb	Eager loads parent organization on vita partner queries
app/helpers/role_helper.rb	Adjusted user role loading with eager-loading queries
app/controllers/*	Adjusted several controller queries to include necessary associations and avoid n+1 queries
Gemfile	Added bullet gem to the development and test groups

Comments suppressed due to low confidence (2)

app/services/search_indexer.rb:34

The removal of includes for documents and intake: [:dependents] may lead to additional queries if the corresponding associations are accessed later. Consider re-adding these associations if they are used downstream in the mapping.

attributes = Client.includes(:tax_returns).where(id: client_ids).map do |client|

app/controllers/portal/tax_returns_controller.rb:45

Removing the includes for client and intake may result in additional queries if these associations are accessed later. Verify that this change does not cause unintended N+1 query issues in related views or logic.

@tax_return = current_client.tax_returns.find_by(id: params[:tax_return_id])

arinchoi03 · 2025-05-05T22:20:57Z

app/controllers/hub/assigned_clients_controller.rb

@@ -25,9 +25,5 @@ def filter_cookie_name
    def ensure_always_current_user_assigned
      @always_current_user_assigned = true
    end
-
-    def load_vita_partners
-      @vita_partners = VitaPartner.accessible_by(current_ability)


does this utilize the application_controller method?

arinchoi03

some q's -- also possibly a boulder on the test.rb set up

arinchoi03 · 2025-05-06T15:27:24Z

app/controllers/hub/state_routings_controller.rb

    end

    def load_independent_organizations
-      @independent_organizations = Organization.where(coalition: nil).where.not(id: @independent_org_srts.pluck(:target_id))
+      @independent_organizations = Organization.where(coalition: nil).where.not(id: @independent_org_srts.map(&:target_id))


can you explain why this change from pluck to map? Is pluck making a new sql query whereas we already have the @independent_org_srts at this point (already fetched from db) so we want to avoid using pluck and instead iterate over the objects already in-memory?

arinchoi03 · 2025-05-06T15:28:11Z

app/controllers/portal/tax_returns_controller.rb

@@ -42,7 +42,7 @@ def show; end
    private

    def load_client_tax_return
-      @tax_return = current_client.tax_returns.includes(client: [:intake]).find_by(id: params[:tax_return_id])


why was this removed in this case? Is it b/c this success page (which is the only page that utilizes load_client_tax_return method does not need client/intake records

arinchoi03 · 2025-05-06T15:31:16Z

app/helpers/role_helper.rb

+      sites.map(&:name).join(", ")
+    elsif user.role_type == TeamMemberRole::TYPE
+      sites = TeamMemberRole.find(user.role_id).sites
+      sites.map(&:name).join(", ")


can you explain why these sets of changes were necessary?

arinchoi03 · 2025-05-06T15:32:51Z

app/models/client.rb

@@ -115,7 +115,7 @@ def self.sortable_intake_attributes
  delegate *delegated_intake_attributes, to: :intake
  scope :after_consent, -> { where.not(consented_to_service_at: nil) }
  scope :assigned_to, ->(user) { joins(:tax_returns).where({ tax_returns: { assigned_user_id: user } }).distinct }
-  scope :with_eager_loaded_associations, -> { includes(:vita_partner, :tax_returns, :documents, tax_returns: [:assigned_user]) }
+  scope :with_eager_loaded_associations, -> { includes(:vita_partner, :documents, intake: [:dependents], tax_returns: [:assigned_user]) }


where are the dependents records used?

arinchoi03 · 2025-05-06T15:35:26Z

app/services/search_indexer.rb

@@ -31,7 +31,7 @@ def self.refresh_filterable_properties(client_ids = nil, limit = 1000)
          Client.where(id: client_ids)
        end

-      attributes = Client.where(id: client_ids).includes(:tax_returns, :documents, intake: :dependents).map do |client|
+      attributes = Client.includes(:tax_returns).where(id: client_ids).map do |client|


dont we look up documents & intake for each client, which is why we include(:tax_returns, :documents, :intake), not sure about the dependents piece?

arinchoi03 · 2025-05-06T15:37:04Z

config/environments/test.rb

@@ -6,6 +6,15 @@
 # and recreated between test runs. Don't rely on the data there!

 Rails.application.configure do
+  config.after_initialize do
+    Bullet.enable        = false


doesn't this completely just turn off the bullet gem & therefore it won't have a chance to raise?

squanto added 4 commits April 25, 2025 12:47

Add bullet gem to dev and test envs

db2c7e1

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

update documents upload question controller

4938239

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

update app/controllers/documents/overview_controller.rb

b99bd20

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

extract load_vita_partners to app controller

e3bac0c

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

squanto added 5 commits April 25, 2025 12:57

update app/controllers/hub/documents_controller.rb

5843f59

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

update partial

268cb01

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

remove unused includes in search indexer

dc09ab5

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

Add message presenter includes

707d9d9

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

add includes to role helper

de14292

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

squanto changed the title ~~[draft] Add bullet gem~~ [draft] Add bullet gem2 Apr 25, 2025

squanto added 3 commits April 25, 2025 14:25

update change_assignee_and_status_controller hub bulk action

70690d9

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

Add various includes to fix n+1 queries

940f295

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

exclude hub + ctc from query scan

7d3730c

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

squanto force-pushed the add-bullet-gem-2 branch from 37973a1 to 7d3730c Compare April 25, 2025 20:13

squanto added 3 commits April 25, 2025 16:23

Fix homepage n+1 query

40b5ead

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

turn off unused include feature for now

6eda3e3

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

disable bullet for after these fixes

3c1532a

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

squanto changed the title ~~[draft] Add bullet gem2~~ Add bullet gem to catch n+1 queries Apr 28, 2025

squanto marked this pull request as ready for review April 28, 2025 14:45

fix remaining spec from added includes

d4231d2

Co-authored-by: Hugo Melo <hmelo@codeforamerica.org>

squanto requested a review from Copilot May 5, 2025 17:12

Copilot AI reviewed May 5, 2025

View reviewed changes

arinchoi03 reviewed May 5, 2025

View reviewed changes

arinchoi03 reviewed May 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add bullet gem to catch n+1 queries #5961

Add bullet gem to catch n+1 queries #5961

Uh oh!

squanto commented Apr 25, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

arinchoi03 May 5, 2025

Uh oh!

arinchoi03 left a comment

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

arinchoi03 May 6, 2025

Uh oh!

Uh oh!

Add bullet gem to catch n+1 queries #5961

Are you sure you want to change the base?

Add bullet gem to catch n+1 queries #5961

Uh oh!

Conversation

squanto commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Uh oh!

github-actions bot commented Apr 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

arinchoi03 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 left a comment

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

arinchoi03 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

squanto commented Apr 25, 2025 •

edited

Loading