Is it possible to activate the forbid_lazy_load plugin incrementally?

[brandur]

We're in a situation right now where we've got dozens of Sequel models, aren't eager loading anything, but would like to be. There's so much existing code though that there's no way feasible way to start doing it everywhere all at once, so we'll need an incremental approach.

I read the ForbidLazyLoad plugin's documentation a couple times, and I'm a bit confused by some aspects. It seems to suggest that it should be possible to forbid lazy load for specific objects:

This plugin offers multiple different ways to forbid lazy loading. You can forbid lazy loading associations for individual model instances:

But then the next paragraph seems to suggest that any use of .all might forbid lazy loading anywhere (and not just on objects where it's set):

forbid_lazy_load is automatically called on instances if the instances are loaded via a method such as Dataset#all, Dataset#each, and other methods that load multiple instances at once. These are the cases where lazily loading associations for such instances can cause N+1 issues.

I tried activating the plugin via:

Sequel::Model.plugin :forbid_lazy_load

Then trying to use it only in a couple specific places at first where I've started to put together the right eager invocations:

def call
  data_set = Cluster.dataset
  data_set = T.cast(
    T.cast(data_set, Sequel::Model::Associations::DatasetMethods).eager(GetClusterResponse.eager_options),
    Sequel::Postgres::Dataset[Cluster]
  )
  data_set = data_set.where(id: query.id) if query.id

  clusters = pagination_params.apply(data_set)
    .all # use of `all` is necessary for eager loading to work
    .map do |cluster|
      T.cast(cluster, Sequel::Plugins::ForbidLazyLoad::InstanceMethods).forbid_lazy_load if Config.test?
      GetClusterResponse.from_cluster(cluster, include_network: false)
    end

When I run the test suite for this specific route it does exactly what I expect. It passes in the state above, and if I forget an eager directive that needs to be there, I get an error telling me about it.

However, when I run other parts of the test suite that start loading one of these models through an association, I also get Sequel::Plugins::ForbidLazyLoad::Error, despite never having activated forbid_lazy_load in that part of the test suite:

  1) Resize#replace_ha_standby? is a resize of a read replica and also requires zero loss failover
     Failure/Error: T.must(source_server.postgres_server).ha_standby?

     Sequel::Plugins::ForbidLazyLoad::Error:
       lazy loading forbidden for this object (association: #<Sequel::Model::Associations::ManyToOneAssociationReflection Resize.many_to_one :source_server, class: :Server>, object: #<Resize @values={id: Eid["kpzl37zzdenmy4rpli6l4db674"], target_storage: 128, initial_storage: 128, cluster_id: Eid["kv2qajdvhnae7yhrobufufy26u"], flavor: "maintenance", initial_major_version: 14, target_major_version: 14, failover_window: nil, source_server_id: Eid["hdete6kpmrvdedkqggwwmgudqq"], target_replica_slot_id: Eid["kopt22kniu2xcopg2ub4frhqoi"], target_plan: "hobby-4", initial_plan: "hobby-4", autoresize: false, failover_id: nil, target_zone: nil, target_distro: "alma_9", target_network_id: nil}>)
     # /Users/brandur/.local/share/mise/installs/ruby/3.4.1/lib/ruby/gems/3.4.0/gems/sequel-5.90.0/lib/sequel/plugins/forbid_lazy_load.rb:167:in 'Sequel::Plugins::ForbidLazyLoad::InstanceMethods#_load_associated_objects'
     # /Users/brandur/.local/share/mise/installs/ruby/3.4.1/lib/ruby/gems/3.4.0/gems/sequel-5.90.0/lib/sequel/model/associations.rb:2730:in 'Sequel::Model::Associations::InstanceMethods#load_associated_objects'
     # /Users/brandur/.local/share/mise/installs/ruby/3.4.1/lib/ruby/gems/3.4.0/gems/sequel-5.90.0/lib/sequel/model/associations.rb:2058:in 'block in def_association_method'
     # ./app/models/resize.rb:154:in 'Resize#replace_ha_standby?'

What I'm hoping to accomplish is to be able to turn this on test by test as I add eager directives, but not have to do everything at once. Is there any way to accomplish that, or am I out of luck? Thanks.

[jeremyevans]

Thank you for providing feedback. The way the forbid_lazy_load plugin works is:

1. Dataset actions that return a single object mark that object as allowing lazy load
2. Dataset actions that can return multiple objects automatically mark those objects as forbidding lazy load

So the documentation isn't wrong. You can forbid lazy loading for objects that would allow it by default, and you can allow lazy load on objects that would forbid it by default

There currently isn't a way to configure the plugin to not automatically forbid lazy loading when retrieving multiple objects. However, can you try the following and let me know if it works the way you want?:

Sequel::Plugins::ForbidLazyLoad::DatasetMethods.class_exec do
 remove_method(:each)
 remove_method(:with_sql_each)
end

[jeremyevans]

Agreed, it is a bit hacky, I just wanted to check if it would work. I think one simple way to make it officially supported is to move those methods to a separate module, and extend datasets with that module by default, but offer a plugin option to not extend the datasets with that module. Please let me know if you are interested in working on that. The code change is fairly simple, but it does require writing some additional tests.

I did integrate ForbidLazyLoad into a number of production apps, and the largest (~150 models, 50k LoC IIRC) took a while to fix all of the issues, even though I wrote most of the code. So I can certainly understand the difficulty of adding it to a larger application.

[brandur]

Thx. I might be able to put something like that together. Just looking at the core plugin code, it kind of looks like DatasetMethods always gets pulled in regardless of plugin options:

module Sequel
  class Model
    ...

      def plugin(plugin, *args, &block)
        m = plugin.is_a?(Module) ? plugin : plugin_module(plugin)

        if !m.respond_to?(:apply) && !m.respond_to?(:configure) && (!args.empty? || block)
          Deprecation.deprecate("Plugin #{plugin} accepts no arguments or block, and passing arguments/block to it", "Remove arguments and block when loading the plugin")
        end

        unless @plugins.include?(m)
          @plugins << m
          m.apply(self, *args, &block) if m.respond_to?(:apply)
          extend(m::ClassMethods) if m.const_defined?(:ClassMethods, false)
          include(m::InstanceMethods) if m.const_defined?(:InstanceMethods, false)
          if m.const_defined?(:DatasetMethods, false)
            dataset_extend(m::DatasetMethods, :create_class_methods=>false)
          end
        end

        m.configure(self, *args, &block) if m.respond_to?(:configure)
      end

To get around that without modifying the base plugin code, would you have to rename ForbidLazyLoad's DatasetMethods to something else, and then make sure it only gets pulled in conditionally based off an option?

I did integrate ForbidLazyLoad into a number of production apps, and the largest (~150 models, 50k LoC IIRC) took a while to fix all of the issues, even though I wrote most of the code. So I can certainly understand the difficulty of adding it to a larger application.

Ah yeah, nice. For what it's worth, I totally think it's possible for us as well, but in this case I'm kind of just a secondary contributor to this particular repo and massive, multi-thousand changes from me are probably not all that welcome, especially when I'm not the one who'll be deploying them. Hope to get it done, but it'll probably be a step-by-step process.

[jeremyevans]

To get around that without modifying the base plugin code, would you have to rename ForbidLazyLoad's DatasetMethods to something else, and then make sure it only gets pulled in conditionally based off an option?

Yep, that should work.

[brandur]

@jeremyevans Just before I get too far on this, without any changes to the test suite, this diff seems to roughly be doing the right thing:

diff --git a/lib/sequel/plugins/forbid_lazy_load.rb b/lib/sequel/plugins/forbid_lazy_load.rb
index 3ac08ba26..ce588fe9e 100644
--- a/lib/sequel/plugins/forbid_lazy_load.rb
+++ b/lib/sequel/plugins/forbid_lazy_load.rb
@@ -39,6 +39,9 @@ module Sequel
     #
     #   Album.first.artist # no error
     #
+    # This behavior of enabling +forbid_lazy_load+ automatically from dataset
+    # methods can be disable using the plugin's +:skip_dataset_methods+ option:
+    #
     # You can allow lazy loading associations for an instance that it
     # was previously forbidden for:
     # 
@@ -98,7 +101,17 @@ module Sequel
     #
     #   # Make the Album class support forbidding lazy load
     #   Album.plugin :forbid_lazy_load
+    #
+    #   # Let lazy loading be forbidden by object, but not automatically for any
+    #   # object loaded via dataset.
+    #   Album.plugin :forbid_lazy_load, skip_dataset_methods: true
     module ForbidLazyLoad
+      def self.apply(model, opts=OPTS)
+        unless opts[:skip_dataset_methods]
+          model.send(:dataset_extend, OptionalDatasetMethods, :create_class_methods=>false)
+        end
+      end
+
       # Error raised when attempting to lazy load an association when
       # lazy loading has been forbidden.
       class Error < StandardError
@@ -179,7 +192,7 @@ module Sequel
         end
       end
 
-      module DatasetMethods
+      module OptionalDatasetMethods
         # Mark model instances retrieved in this call as forbidding lazy loading.
         def each
           if row_proc

Not sure if the particularly naming conventions etc. are what would be preferred though (or use of send to get at dataset_extend). Thoughts?

[jeremyevans]

That looks good, though I think we should rename:

• OptionalDatasetMethods -> ForbidByDefault
• :skip_dataset_methods -> :allow_by_default