Proposal: Extension to strict loading

rubyonrails-core
,
1

I love the strict loading feature, for a couple of reasons:

  • The obvious one - it reduces the likelihood of N+1 queries, and in general prioritises performance
  • It highlights where you are probably loading way too many records from the database (e.g. an absurdly large includes statement)

We’ve switched it on by default across our codebase, raising violation errors in test / dev, and just logging strict loading violations in production.

That said, after carefully setting up all your strict loading, there’s nothing stopping someone triggering additional queries elsewhere in the codebase (e.g. loading another model from an instance method of a Rails model). I think it’d be valuable to have the option to prevent unexpected queries if desired, so that, in general, any “website operation” (e.g. any POST / PUT / PATCH / DELETE), would operate something like:

  • Load all the data you need at the start
  • Do some stuff (perhaps some calculations etc.)
  • Save the changes to your model(s)

I have managed to get something working through ActiveSupport::Notifications, Thread.current and a module that instruments a class’ instance methods. Obviously this is an attempt to make this work outside of Rails, so this isn’t an implementation suggestion, but at least shows it’s possible.

In addition, I’ve excluded it from the code below, but it’d be possible to opt out for specific methods (e.g. a Rails model could have a self.skip_query_interception = %i[thing]). In addition, FactoryBot doesn’t work well with strict loading, and you’d need similar escape hatches to turn it off completely.

There’s also no reason this couldn’t work outside of Rails models

app/models/concerns/query_interceptable.rb

module QueryInterceptable
  extend ActiveSupport::Concern

  METHOD_INFORMATION_KEY = :model_instance_method

  class QueryInterceptor < Module
    RAILS_METHOD_REGEXPS = [
      /\A(validate|autosave)_associated_records_for/,
      /\Areload\Z/
    ].freeze

    def initialize(model)
      model.public_methods(false).each do |method|
        # Maybe there's an easier way to exclude Rails generated methods?
        next if RAILS_METHOD_REGEXPS.any? { |regexp| method.to_s.match?(regexp) }

        define_method(method) do |*args, **kwargs, &block|
          Thread.current[METHOD_INFORMATION_KEY] = { klass: self.class.name, method: }

          value = super(*args, **kwargs, &block)
          value
        ensure
          Thread.current[METHOD_INFORMATION_KEY] = nil
        end
      end
    end
  end

  UnexpectedQueryError = Class.new(StandardError)

  def self.active_record_notification_handler(event)
    return if %w[TRANSACTION SCHEMA].include?(event.payload.fetch(:name))

    model_instance_method = Thread.current[METHOD_INFORMATION_KEY] || return

    raise UnexpectedQueryError.new(model_instance_method, event.payload.fetch(:sql))
  end

  included do
    after_initialize :instrument_instance_methods

    private

    def instrument_instance_methods
      return if self.class.included_modules.any? { |module| module.is_a?(QueryInterceptor) }

      self.class.prepend QueryInterceptor.new(self)
    end
  end
end

config/initializers/sql_active_record_subscriber.rb

ActiveSupport::Notifications.subscribe('sql.active_record') do |event|
  QueryInterceptable.active_record_notification_handler(event)
end

Then for any class

class ApplicationRecord < ActiveRecord::Base
  include QueryInterceptable
end

class SomeNonRailsClass
  include QueryInterceptable
end

You might also want to configure this globally too (e.g. apply to all classes outside of app/services or app/jobs etc.). This way you wouldn’t need to include in potentially hundreds / thousands of files.

Would this be a feature that people would be interested in? Could it be rolled into strict loading / be its own thing?

Thanks, Alex