From a9506f5a3c9416a3f3206e0d6008abaa7f077680 Mon Sep 17 00:00:00 2001 From: Linda Goldstein Date: Mon, 2 Mar 2026 19:04:32 -0800 Subject: [PATCH 1/3] add prosopite --- Gemfile | 1 + Gemfile.lock | 2 + config/environments/development.rb | 4 ++ config/environments/production.rb | 4 ++ config/environments/test.rb | 4 ++ config/initializers/prosopite.rb | 21 +++++++++ spec/.prosopite_ignore | 7 +++ spec/support/prosopite.rb | 69 ++++++++++++++++++++++++++++++ 8 files changed, 112 insertions(+) create mode 100644 config/initializers/prosopite.rb create mode 100644 spec/.prosopite_ignore create mode 100644 spec/support/prosopite.rb diff --git a/Gemfile b/Gemfile index 406ad17e52..bf747d8bb9 100644 --- a/Gemfile +++ b/Gemfile @@ -64,6 +64,7 @@ gem "wicked" # Multi-step form wizard for Rails group :development, :test do gem "brakeman" # Security inspection gem "bullet" # Detect and fix N+1 queries + gem "prosopite" # N+1 query detection via SQL pattern analysis gem "byebug", platforms: %i[mri mingw x64_mingw] # Debugger console gem "dotenv-rails" # Environment variable management gem "erb_lint", require: false # ERB linter diff --git a/Gemfile.lock b/Gemfile.lock index ba52d3cc9d..5ebc5cd3ae 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -423,6 +423,7 @@ GEM actionpack (>= 7.1) prettyprint (0.2.0) prism (1.9.0) + prosopite (2.1.2) pry (0.16.0) coderay (~> 1.1) method_source (~> 1.0) @@ -754,6 +755,7 @@ DEPENDENCIES pg_query pghero pretender + prosopite pry pry-byebug puma (~> 7.0) diff --git a/config/environments/development.rb b/config/environments/development.rb index 0ffa5ba8ac..ff764f7e0a 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -75,6 +75,10 @@ Bullet.bullet_logger = true end + # Prosopite N+1 query detection + config.prosopite_enabled = true + config.prosopite_min_n_queries = 5 # More lenient for development + # Annotate rendered view with file names. config.action_view.annotate_rendered_view_with_filenames = true diff --git a/config/environments/production.rb b/config/environments/production.rb index 050d32d48b..cb660bbab2 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -108,4 +108,8 @@ # ] # Skip DNS rebinding protection for the default health check endpoint. # config.host_authorization = { exclude: ->(request) { request.path == "/up" } } + + # Prosopite N+1 query detection (disabled by default in production) + config.prosopite_enabled = ENV.fetch("PROSOPITE_ENABLED", "false") == "true" + config.prosopite_min_n_queries = ENV.fetch("PROSOPITE_MIN_N_QUERIES", "10").to_i end diff --git a/config/environments/test.rb b/config/environments/test.rb index 3111e9657b..979ac48f72 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -71,6 +71,10 @@ # Bullet.raise = true # TODO https://github.com/rubyforgood/casa/issues/2441 end + # Prosopite N+1 query detection + config.prosopite_enabled = true + config.prosopite_min_n_queries = 2 # Stricter for tests + # Annotate rendered view with file names. # config.action_view.annotate_rendered_view_with_filenames = true diff --git a/config/initializers/prosopite.rb b/config/initializers/prosopite.rb new file mode 100644 index 0000000000..cd16b948d2 --- /dev/null +++ b/config/initializers/prosopite.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# Only enable Rack middleware if Prosopite is configured on +if Rails.configuration.respond_to?(:prosopite_enabled) && Rails.configuration.prosopite_enabled + require "prosopite/middleware/rack" + Rails.configuration.middleware.use(Prosopite::Middleware::Rack) +end + +Rails.application.config.after_initialize do + # Core settings + Prosopite.enabled = Rails.configuration.respond_to?(:prosopite_enabled) && + Rails.configuration.prosopite_enabled + + # Minimum repeated queries to trigger detection (default: 2) + Prosopite.min_n_queries = Rails.configuration.respond_to?(:prosopite_min_n_queries) ? + Rails.configuration.prosopite_min_n_queries : 2 + + # Logging options + Prosopite.rails_logger = true # Log to Rails.logger + Prosopite.prosopite_logger = Rails.env.development? # Log to log/prosopite.log +end diff --git a/spec/.prosopite_ignore b/spec/.prosopite_ignore new file mode 100644 index 0000000000..e88405f8a0 --- /dev/null +++ b/spec/.prosopite_ignore @@ -0,0 +1,7 @@ +# Directories to exclude from Prosopite N+1 scanning +# Remove paths as you fix N+1s in each area +# +# Example entries: +# spec/features +# spec/system +# spec/requests/legacy diff --git a/spec/support/prosopite.rb b/spec/support/prosopite.rb new file mode 100644 index 0000000000..c3ccb54d3f --- /dev/null +++ b/spec/support/prosopite.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +return unless defined?(Prosopite) + +# Test configuration +Prosopite.enabled = true +Prosopite.raise = true # Fail specs on N+1 detection +Prosopite.rails_logger = true +Prosopite.prosopite_logger = true + +# Allowlist for known acceptable N+1 patterns (e.g., test matchers) +Prosopite.allow_stack_paths = [ + "shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb", + "shoulda/matchers/active_model/validate_presence_of_matcher.rb", + "shoulda/matchers/active_model/validate_inclusion_of_matcher.rb", + "shoulda/matchers/active_model/allow_value_matcher.rb" +] + +# Optional: Load ignore list from file for gradual rollout +PROSOPITE_IGNORE = if File.exist?("spec/.prosopite_ignore") + File.read("spec/.prosopite_ignore").lines.map(&:chomp).reject(&:empty?) +else + [] +end + +# Monkey-patch FactoryBot to pause during factory creation +# Prevents false positives from factory callbacks +if defined?(FactoryBot) + module FactoryBot + module Strategy + class Create + alias_method :original_result, :result + + def result(evaluation) + if defined?(Prosopite) && Prosopite.enabled? + Prosopite.pause { original_result(evaluation) } + else + original_result(evaluation) + end + end + end + end + end +end + +# RSpec integration +RSpec.configure do |config| + config.around do |example| + if use_prosopite?(example) + Prosopite.scan { example.run } + else + original_enabled = Prosopite.enabled? + Prosopite.enabled = false + example.run + Prosopite.enabled = original_enabled + end + end +end + +def use_prosopite?(example) + # Explicit metadata takes precedence + return false if example.metadata[:disable_prosopite] + return true if example.metadata[:enable_prosopite] + + # Check against ignore list + PROSOPITE_IGNORE.none? do |path| + File.fnmatch?("./#{path}/*", example.metadata[:rerun_file_path].to_s) + end +end From 8eeb090b3d0f0c006a823c26e6453d732866abcc Mon Sep 17 00:00:00 2001 From: Linda Goldstein Date: Mon, 2 Mar 2026 19:06:35 -0800 Subject: [PATCH 2/3] remove bullet now that we have prosopite --- Gemfile | 1 - Gemfile.lock | 5 ----- config/environments/development.rb | 7 ------- config/environments/test.rb | 8 -------- 4 files changed, 21 deletions(-) diff --git a/Gemfile b/Gemfile index bf747d8bb9..76e324dc02 100644 --- a/Gemfile +++ b/Gemfile @@ -63,7 +63,6 @@ gem "wicked" # Multi-step form wizard for Rails group :development, :test do gem "brakeman" # Security inspection - gem "bullet" # Detect and fix N+1 queries gem "prosopite" # N+1 query detection via SQL pattern analysis gem "byebug", platforms: %i[mri mingw x64_mingw] # Debugger console gem "dotenv-rails" # Environment variable management diff --git a/Gemfile.lock b/Gemfile.lock index 5ebc5cd3ae..da6b99209a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -115,9 +115,6 @@ GEM bugsnag (6.28.0) concurrent-ruby (~> 1.0) builder (3.3.0) - bullet (8.1.0) - activesupport (>= 3.0.0) - uniform_notifier (~> 1.11) bundler-audit (0.9.3) bundler (>= 1.2.0) thor (~> 1.0) @@ -666,7 +663,6 @@ GEM unicode-display_width (3.2.0) unicode-emoji (~> 4.1) unicode-emoji (4.2.0) - uniform_notifier (1.18.0) useragent (0.16.11) view_component (4.2.0) actionview (>= 7.1.0) @@ -711,7 +707,6 @@ DEPENDENCIES blueprinter brakeman bugsnag - bullet bundler-audit byebug capybara diff --git a/config/environments/development.rb b/config/environments/development.rb index ff764f7e0a..800c6ce9fb 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -68,13 +68,6 @@ # Raises error for missing translations. # config.i18n.raise_on_missing_translations = true - config.after_initialize do - Bullet.enable = true - Bullet.console = true - Bullet.rails_logger = true - Bullet.bullet_logger = true - end - # Prosopite N+1 query detection config.prosopite_enabled = true config.prosopite_min_n_queries = 5 # More lenient for development diff --git a/config/environments/test.rb b/config/environments/test.rb index 979ac48f72..602fdc65bf 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -63,14 +63,6 @@ # Raises error for missing translations. config.i18n.raise_on_missing_translations = true - config.after_initialize do - Bullet.enable = true - Bullet.console = true - Bullet.bullet_logger = true - Bullet.rails_logger = true - # Bullet.raise = true # TODO https://github.com/rubyforgood/casa/issues/2441 - end - # Prosopite N+1 query detection config.prosopite_enabled = true config.prosopite_min_n_queries = 2 # Stricter for tests From 8d1b97e18d6d7bde4d850d96dbee084c9c97a811 Mon Sep 17 00:00:00 2001 From: Linda Goldstein Date: Mon, 2 Mar 2026 19:11:32 -0800 Subject: [PATCH 3/3] prosopite todo list --- PROSOPITE_TODO.md | 145 ++++++++++++++++++++++++++++++++++++++ spec/support/prosopite.rb | 2 +- 2 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 PROSOPITE_TODO.md diff --git a/PROSOPITE_TODO.md b/PROSOPITE_TODO.md new file mode 100644 index 0000000000..cad0163ce4 --- /dev/null +++ b/PROSOPITE_TODO.md @@ -0,0 +1,145 @@ +# Prosopite N+1 Query Issues + +Issues detected by Prosopite during test suite run. Fix by adding eager loading (`includes`, `preload`) or restructuring queries. + +## High Priority (20+ occurrences) + +### app/decorators/casa_case_decorator.rb:34 +- **Method:** `map` in decorator +- **Likely fix:** Add `includes` for associated records being accessed in iteration + +### app/models/user.rb:84 +- **Method:** `create_preference_set` +- **Query:** `SELECT "preference_sets".* FROM "preference_sets" WHERE "preference_sets"."user_id" = $1` +- **Likely fix:** Check if preference_set already loaded before querying + +### app/models/concerns/api.rb:11 +- **Method:** `initialize_api_credentials` +- **Query:** `SELECT "api_credentials".* FROM "api_credentials" WHERE "api_credentials"."user_id" = $1` +- **Likely fix:** Check if api_credentials already loaded before querying + +### app/lib/importers/file_importer.rb:50 +- **Method:** `create_user_record` +- **Queries:** Multiple user lookups during import +- **Likely fix:** Batch lookups or use `Prosopite.pause` for intentional import operations + +## Medium Priority (10-19 occurrences) + +### app/validators/user_validator.rb:56 +- **Method:** `validate_uniqueness` +- **Query:** `SELECT "users".* FROM "users" WHERE "users"."type" = $1 AND "users"."email" = $2` +- **Likely fix:** Consider if validation is necessary during bulk operations + +### app/lib/importers/supervisor_importer.rb:47 +- **Method:** `block in assign_volunteers` +- **Query:** `SELECT "users".* FROM "users" INNER JOIN "supervisor_volunteers"...` +- **Likely fix:** Preload volunteers before assignment loop + +### app/lib/importers/supervisor_importer.rb:51 +- **Method:** `block in assign_volunteers` +- **Query:** `SELECT "users".* FROM "users" WHERE "users"."id" = $1` +- **Likely fix:** Batch load users by ID before iteration + +### app/controllers/case_contacts/form_controller.rb:156 +- **Method:** `block in create_additional_case_contacts` +- **Likely fix:** Eager load case contact associations + +## Lower Priority (5-9 occurrences) + +### app/models/court_date.rb:32 +- **Method:** `associated_reports` +- **Likely fix:** Add `includes` for court report associations + +### app/lib/importers/supervisor_importer.rb:23 +- **Method:** `block in import_supervisors` +- **Query:** `SELECT "users".* FROM "users" WHERE "users"."type" = $1 AND "users"."email" = $2` +- **Likely fix:** Batch check existing supervisors before import loop + +## Lower Priority (2-4 occurrences) + +### app/lib/importers/file_importer.rb:57 +- **Method:** `email_addresses_to_users` +- **Likely fix:** Batch load users by email + +### app/lib/importers/case_importer.rb:41 +- **Method:** `create_casa_case` +- **Likely fix:** Preload or batch casa_case lookups + +### app/decorators/contact_type_decorator.rb:14 +- **Method:** `last_time_used_with_cases` +- **Likely fix:** Eager load case associations + +### app/datatables/reimbursement_datatable.rb:25 +- **Method:** `block in data` +- **Query:** `SELECT "addresses".* FROM "addresses" WHERE "addresses"."user_id" = $1` +- **Likely fix:** Add `includes(:address)` to reimbursement query + +### app/services/volunteer_birthday_reminder_service.rb:7 +- **Method:** `block in send_reminders` +- **Likely fix:** Eager load volunteer associations + +### app/models/contact_topic.rb:27 +- **Method:** `block in generate_for_org!` +- **Likely fix:** Batch operations or use `Prosopite.pause` for setup + +### app/models/casa_org.rb:82 +- **Method:** `user_count` +- **Likely fix:** Use counter cache or single count query + +### app/models/casa_org.rb:62 +- **Method:** `case_contacts_count` +- **Likely fix:** Use counter cache or single count query + +### app/lib/importers/volunteer_importer.rb:23 +- **Method:** `block in import_volunteers` +- **Likely fix:** Batch check existing volunteers before import loop + +### app/lib/importers/case_importer.rb:20 +- **Method:** `block in import_cases` +- **Likely fix:** Batch check existing cases before import loop + +### app/controllers/case_contacts/form_controller.rb:26 +- **Method:** `block (2 levels) in update` +- **Likely fix:** Eager load contact associations + +## Single Occurrence + +### app/services/missing_data_export_csv_service.rb:40 +- **Method:** `full_data` + +### app/policies/contact_topic_answer_policy.rb:18 +- **Method:** `create?` + +### app/models/casa_case.rb:152 +- **Method:** `next_court_date` + +### app/models/all_casa_admins/casa_org_metrics.rb:16 +- **Method:** `map` + +### config/initializers/sent_email_event.rb:7 +- **Method:** `block in ` +- **Query:** `SELECT "casa_orgs".* FROM "casa_orgs" WHERE "casa_orgs"."id" = $1` +- **Note:** Initializer callback - consider caching org lookup + +## Notes + +- **Importers:** Many N+1s occur in import code. Consider wrapping entire import operations in `Prosopite.pause { }` if the N+1 pattern is intentional for per-record processing, or batch-load records before iteration. + +- **Decorators:** Add `includes` at the controller/query level before passing to decorators. + +- **Callbacks:** User model callbacks (`create_preference_set`, `initialize_api_credentials`) fire on each create. Consider if these can be optimized or if the N+1 is acceptable for single-record operations. + +## How to Fix + +```ruby +# Before (N+1) +users.each { |u| u.orders.count } + +# After (eager loading) +users.includes(:orders).each { |u| u.orders.count } + +# For intentional batch operations +Prosopite.pause do + records.each { |r| process_individually(r) } +end +``` diff --git a/spec/support/prosopite.rb b/spec/support/prosopite.rb index c3ccb54d3f..8395891864 100644 --- a/spec/support/prosopite.rb +++ b/spec/support/prosopite.rb @@ -4,7 +4,7 @@ # Test configuration Prosopite.enabled = true -Prosopite.raise = true # Fail specs on N+1 detection +Prosopite.raise = false # Log only, don't fail specs Prosopite.rails_logger = true Prosopite.prosopite_logger = true