Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw jruby] # Windows does not
gem "view_component" # View components for reusability
gem "wicked" # Multi-step form wizard for Rails

group :development, :test do
group :development, :test, :production do
gem "brakeman" # Security inspection
gem "bullet" # Detect and fix N+1 queries
gem "prosopite" # N+1 query detection via SQL pattern analysis
Comment on lines +64 to +66
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this group to include :production will also bundle/require a large set of dev/test-only gems (e.g., rspec-rails, factory_bot_rails, linters) in production, increasing boot time/memory and potentially introducing production-only dependency conflicts. Keep the group as :development, :test and, if Prosopite truly needs to be available in production, place only prosopite in its own appropriate group (or mark it require: false) instead of pulling the entire dev/test toolchain into production.

Copilot uses AI. Check for mistakes.
gem "byebug", platforms: %i[mri mingw x64_mingw] # Debugger console
gem "dotenv-rails" # Environment variable management
gem "erb_lint", require: false # ERB linter
Expand Down
7 changes: 2 additions & 5 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,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)
Expand Down Expand Up @@ -424,6 +421,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)
Expand Down Expand Up @@ -668,7 +666,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)
Expand Down Expand Up @@ -713,7 +710,6 @@ DEPENDENCIES
blueprinter
brakeman
bugsnag
bullet
bundler-audit
byebug
capybara
Expand Down Expand Up @@ -757,6 +753,7 @@ DEPENDENCIES
pg_query
pghero
pretender
prosopite
pry
pry-byebug
puma (~> 7.0)
Expand Down
145 changes: 145 additions & 0 deletions PROSOPITE_TODO.md
Original file line number Diff line number Diff line change
@@ -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 <top (required)>`
- **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 }
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "After (eager loading)" example still calls u.orders.count, which issues a separate COUNT(*) query per user even when the association is preloaded. To demonstrate fixing an N+1, this should use u.orders.size/length (or a counter cache / precomputed aggregate) instead of count.

Suggested change
users.includes(:orders).each { |u| u.orders.count }
users.includes(:orders).each { |u| u.orders.size }

Copilot uses AI. Check for mistakes.

# For intentional batch operations
Prosopite.pause do
records.each { |r| process_individually(r) }
end
```
9 changes: 3 additions & 6 deletions config/environments/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,9 @@
# 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

# Annotate rendered view with file names.
config.action_view.annotate_rendered_view_with_filenames = true
Expand Down
8 changes: 0 additions & 8 deletions config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

# Annotate rendered view with file names.
# config.action_view.annotate_rendered_view_with_filenames = true

Expand Down
23 changes: 23 additions & 0 deletions config/initializers/prosopite.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# frozen_string_literal: true

# Rack middleware for development only — in test, scanning is handled by RSpec hooks
if Rails.env.development? &&
Rails.configuration.respond_to?(:prosopite_enabled) &&
Rails.configuration.prosopite_enabled
require "prosopite/middleware/rack"
Rails.configuration.middleware.use(Prosopite::Middleware::Rack)
end

# Development configuration — test config lives in spec/support/prosopite.rb
Rails.application.config.after_initialize do
next unless Rails.env.development?

Prosopite.enabled = Rails.configuration.respond_to?(:prosopite_enabled) &&
Rails.configuration.prosopite_enabled

Prosopite.min_n_queries = Rails.configuration.respond_to?(:prosopite_min_n_queries) ?
Rails.configuration.prosopite_min_n_queries : 2

Prosopite.rails_logger = true
Prosopite.prosopite_logger = true
end
15 changes: 15 additions & 0 deletions spec/.prosopite_ignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Directories excluded from Prosopite N+1 raise (will still log).
# Remove paths as you fix the underlying N+1s.
#
# See PROSOPITE_TODO.md for the full list of known issues.
spec/models
spec/services
spec/lib
spec/system
spec/requests
spec/controllers
spec/views
spec/decorators
spec/policies
spec/datatables
spec/helpers
73 changes: 73 additions & 0 deletions spec/support/prosopite.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# frozen_string_literal: true

return unless defined?(Prosopite)

# Test configuration — this file owns all Prosopite settings for the test env
Prosopite.enabled = true
Prosopite.raise = true
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"
]

# Load ignore list from file for gradual rollout — directories listed in
# .prosopite_ignore are scanned but won't raise, only log.
PROSOPITE_IGNORE = if File.exist?("spec/.prosopite_ignore")
File.read("spec/.prosopite_ignore")
.lines
.map(&:chomp)
.reject { |line| line.empty? || line.start_with?("#") }
else
[]
end

RSpec.configure do |config|
# Pause Prosopite during factory creation to prevent false positives
# from factory callbacks and associations
config.before(:suite) do
if defined?(FactoryBot)
FactoryBot::SyntaxRunner.class_eval do
alias_method :original_create, :create

def create(*args, **kwargs, &block)
if defined?(Prosopite) && Prosopite.enabled?
Prosopite.pause { original_create(*args, **kwargs, &block) }
else
original_create(*args, **kwargs, &block)
Comment on lines +36 to +42
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This FactoryBot monkey-patch can break under Spring: each spec run re-enters before(:suite) and re-aliases original_create to the already-wrapped create, which can lead to recursion/stack overflow. Guard the aliasing so it only happens once per process (e.g., check method_defined?) or prefer a Module#prepend wrapper that is idempotent.

Suggested change
alias_method :original_create, :create
def create(*args, **kwargs, &block)
if defined?(Prosopite) && Prosopite.enabled?
Prosopite.pause { original_create(*args, **kwargs, &block) }
else
original_create(*args, **kwargs, &block)
unless method_defined?(:original_create)
alias_method :original_create, :create
def create(*args, **kwargs, &block)
if defined?(Prosopite) && Prosopite.enabled?
Prosopite.pause { original_create(*args, **kwargs, &block) }
else
original_create(*args, **kwargs, &block)
end

Copilot uses AI. Check for mistakes.
end
end
end
end
end

config.around do |example|
if use_prosopite?(example)
Prosopite.scan { example.run }
else
original_enabled = Prosopite.enabled?
Prosopite.enabled = false
begin
example.run
ensure
Prosopite.enabled = original_enabled
Comment thread
compwron marked this conversation as resolved.
end
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)
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ignore matching here appears to use a nonstandard RSpec metadata key (:rerun_file_path), which is likely to be nil for most examples. That would prevent the ignore list from ever matching and cause Prosopite to run/raise in specs that are supposed to be ignored. Use the actual spec file path metadata key (e.g., :file_path/:absolute_file_path) when comparing against .prosopite_ignore.

Suggested change
PROSOPITE_IGNORE.none? do |path|
File.fnmatch?("./#{path}/*", example.metadata[:rerun_file_path].to_s)
spec_path = example.metadata[:absolute_file_path] ||
example.metadata[:file_path] ||
example.metadata[:rerun_file_path]
PROSOPITE_IGNORE.none? do |path|
File.fnmatch?("./#{path}/*", spec_path.to_s)

Copilot uses AI. Check for mistakes.
Comment thread
compwron marked this conversation as resolved.
end
Comment on lines +64 to +72
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_prosopite? is defined at the top level, which adds a method to Object and can leak into other specs/helpers. Define it in a module (e.g., Support::Prosopite) and call it from the RSpec hook, or define it as a local lambda inside the RSpec.configure block to avoid global namespace pollution.

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +72
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore matching is currently very brittle: it relies on example.metadata[:rerun_file_path], which may be unset for many runs, and it matches against a pattern prefixed with ./ (while RSpec file paths are typically spec/... or absolute). If :rerun_file_path is nil, the ignore list will never match and Prosopite will scan everything. Consider using example.metadata[:file_path] (or example.location) and normalizing paths before matching (e.g., via File.expand_path).

Copilot uses AI. Check for mistakes.
end
Loading