Skip to content

Commit

Permalink
Make static_cache plugin better handle cases where forbid_lazy_load p…
Browse files Browse the repository at this point in the history
…lugin is already loaded

If the static_cache and forbid_lazy_load plugins were used
together, code such as:

  static_cache_model[id].example_one_to_many_associations

would fail with a lazy load error.  This is because the
static_cache cached instances forbid lazy loading because
they were retrieved in a single query.  You generally want
that so that:

  static_cache_model.each{|o| o.example_one_to_many_associations}

raises a lazy load error, since that is a type of N+1 (really
N+0) case.

Have the static cache plugin make it so that if forbid_lazy_load
plugin is already loaded for the class, attempts to retrieve a
single object via .[], .cache_get_pk, and .first return a frozen
object copy that does not forbid lazy loading.

This approach is not ncessary if the :frozen option is given to
the static_cache plugin, since a copy is already created in that
case.
  • Loading branch information
jeremyevans committed Jun 26, 2023
1 parent af953ca commit f473b7b
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 20 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
=== master

* Make static_cache plugin better handle cases where forbid_lazy_load plugin is already loaded (jeremyevans)

* Fix ShardedThreadedConnectionPool#remove_server to disconnect all connections if removing multiple servers (jeremyevans)

* Support SEQUEL_DEFAULT_CONNECTION_POOL environment variable for choosing connection pool when :pool_class Database option is not set (jeremyevans)
Expand Down
45 changes: 44 additions & 1 deletion lib/sequel/plugins/static_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ module StaticCache
def self.configure(model, opts=OPTS)
model.instance_exec do
@static_cache_frozen = opts.fetch(:frozen, true)
if @static_cache_frozen && defined?(::Sequel::Plugins::ForbidLazyLoad::ClassMethods) && is_a?(::Sequel::Plugins::ForbidLazyLoad::ClassMethods)
extend ForbidLazyLoadClassMethods
end
load_cache
end
end
Expand All @@ -90,7 +93,7 @@ def first(*args)
if defined?(yield) || args.length > 1 || (args.length == 1 && !args[0].is_a?(Integer))
super
else
@all.first(*args)
_static_cached_first(*args)
end
end

Expand Down Expand Up @@ -222,6 +225,11 @@ def load_cache

private

# Use static cache to return first arguments.
def _static_cached_first(*args)
@all.first(*args)
end

# Load the static cache rows from the database.
def load_static_cache_rows
ret = super if defined?(super)
Expand All @@ -246,6 +254,41 @@ def static_cache_object(o)
end
end

module ForbidLazyLoadClassMethods
# Do not forbid lazy loading for single object retrieval.
def cache_get_pk(pk)
primary_key_lookup(pk)
end

private

# Use static cache to return first arguments.
def _static_cached_first(*args)
if args.empty?
if o = @all.first(*args)
_static_cache_frozen_copy(o)
end
else
@all.first(*args).map!{|o| _static_cache_frozen_copy(o)}
end
end

# Return a frozen copy of the object that does not have lazy loading
# forbidden.
def _static_cache_frozen_copy(o)
o = call(Hash[o.values])
o.errors.freeze
o.freeze
end

# Do not forbid lazy loading for single object retrieval.
def primary_key_lookup(pk)
if o = cache[pk]
_static_cache_frozen_copy(o)
end
end
end

module InstanceMethods
# Disallowing destroying the object unless the frozen: false option was used.
def before_destroy
Expand Down
106 changes: 87 additions & 19 deletions spec/extensions/static_cache_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -294,15 +294,8 @@
end
end

describe "without options" do
before do
@c.plugin :static_cache
@c1 = @c.cache[1]
@c2 = @c.cache[2]
@db.sqls
end

include static_cache_specs
unfrozen_static_cache_specs = Module.new do
extend Minitest::Spec::DSL

it "should work correctly with composite keys" do
@db.fetch = [{:id=>1, :id2=>1}, {:id=>2, :id2=>1}]
Expand All @@ -322,16 +315,6 @@
@c.all.all?{|o| o.frozen?}.must_equal true
end

it "should make .[] method with primary key return cached instances" do
@c[1].must_be_same_as(@c1)
@c[2].must_be_same_as(@c2)
end

it "should have cache_get_pk return cached instances" do
@c.cache_get_pk(1).must_be_same_as(@c1)
@c.cache_get_pk(2).must_be_same_as(@c2)
end

it "should have each yield cached objects" do
a = []
@c.each{|o| a << o}
Expand Down Expand Up @@ -393,6 +376,91 @@
end
end

describe "without options" do
before do
@c.plugin :static_cache
@c1 = @c.cache[1]
@c2 = @c.cache[2]
@db.sqls
end

include static_cache_specs
include unfrozen_static_cache_specs

it "should make .[] method with primary key return cached instances" do
@c[1].must_be_same_as(@c1)
@c[2].must_be_same_as(@c2)
end

it "should have cache_get_pk return cached instances" do
@c.cache_get_pk(1).must_be_same_as(@c1)
@c.cache_get_pk(2).must_be_same_as(@c2)
end
end

describe "with forbid_lazy_load plugin" do
before do
@c.plugin :forbid_lazy_load
@c.plugin :static_cache
@c.many_to_one :c, :class=>Class.new(Sequel::Model(@db[:t])), :key=>:id
@c1 = @c.cache[1]
@c2 = @c.cache[2]
@db.sqls
end

include static_cache_specs
include unfrozen_static_cache_specs

it "should return nil for unmatched pk" do
@c[3].must_be_nil
end

it "should make .[] method with primary key return frozen copies of cached instances" do
@c[1].wont_be_same_as(@c1)
@c[2].wont_be_same_as(@c2)
@c[1].must_be :frozen?
@c[2].must_be :frozen?
end

it "should make allow lazy loading of associations after []" do
@c[1].c.values.must_equal(:id=>1)
end

it "should have cache_get_pk return frozen copies of cached instances" do
@c.cache_get_pk(1).wont_be_same_as(@c1)
@c.cache_get_pk(2).wont_be_same_as(@c2)
@c.cache_get_pk(1).must_be :frozen?
@c.cache_get_pk(2).must_be :frozen?
end

it "should make allow lazy loading of associations after cache_get_pk" do
@c.cache_get_pk(1).c.values.must_equal(:id=>1)
end

it "should return nil for empty cache" do
@c.instance_variable_set(:@all, [])
@c.first.must_be_nil
end

it "should have first without arguments return frozen copies of cached instances" do
@c.first.wont_be_same_as(@c1)
@c.first.must_be :frozen?
end

it "should make allow lazy loading of associations after first without arguments" do
@c.first.c.values.must_equal(:id=>1)
end

it "should have first with single Integer argument return frozen copies of cached instances" do
@c.first(1)[0].wont_be_same_as(@c1)
@c.first(1)[0].must_be :frozen?
end

it "should make allow lazy loading of associations after first with single Integer argument" do
@c.first(1)[0].c.values.must_equal(:id=>1)
end
end

describe "with :frozen=>false option" do
before do
@c.plugin :static_cache, :frozen=>false
Expand Down

0 comments on commit f473b7b

Please sign in to comment.