Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split the with_deferred_parent_expiration and with_deferred_parent_expiration #578

Merged
merged 3 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ test/version_tmp
tmp
.rubocop-http*
.byebug_history
gemfiles/*.lock
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@

## Unreleased

## 1.6.3

- Split the `with_deferred_parent_expiration` and `with_deferred_parent_expiration`. (#578)
drinkbeer marked this conversation as resolved.
Show resolved Hide resolved

## 1.6.2

- Support deferred expiry of associations and attributes. Add a rake task to create test database.
- Support deferred expiry of associations and attributes. Add a rake task to create test database. (#577)
drinkbeer marked this conversation as resolved.
Show resolved Hide resolved

## 1.6.1

Expand Down
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ GIT
PATH
remote: .
specs:
identity_cache (1.6.2)
identity_cache (1.6.3)
activerecord (>= 7.0)
ar_transaction_changes (~> 1.1)

Expand Down
80 changes: 74 additions & 6 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,82 @@ require "rdoc/task"
desc("Default: run tests and style checks.")
task(default: [:test, :rubocop])

desc("Test the identity_cache plugin.")
Rake::TestTask.new(:test) do |t|
t.libs << "lib"
t.libs << "test"
t.pattern = "test/**/*_test.rb"
t.verbose = true
namespace :test do
desc "Test the identity_cache plugin with default Gemfile"
Rake::TestTask.new(:default) do |t|
t.libs << "lib"
t.libs << "test"
t.pattern = "test/**/*_test.rb"
t.verbose = true
end

desc "Test the identity_cache plugin with minimum supported dependencies"
task :min_supported do
gemfile = File.expand_path("gemfiles/Gemfile.min-supported", __dir__)
ENV["BUNDLE_GEMFILE"] = gemfile

puts "\nInstalling dependencies for #{gemfile}..."
Bundler.with_unbundled_env do
system("bundle install --gemfile #{gemfile}") || abort("Bundle install failed")
end

puts "Running tests with #{gemfile}..."
Rake::TestTask.new(:run_min_supported) do |t|
t.libs << "lib"
t.libs << "test"
t.pattern = "test/**/*_test.rb"
t.verbose = true
end
Rake::Task["run_min_supported"].invoke
end

desc "Test the identity_cache plugin with latest released dependencies"
task :latest_release do
gemfile = File.expand_path("gemfiles/Gemfile.latest-release", __dir__)
ENV["BUNDLE_GEMFILE"] = gemfile

puts "\nInstalling dependencies for #{gemfile}..."
Bundler.with_unbundled_env do
system("bundle install --gemfile #{gemfile}") || abort("Bundle install failed")
end

puts "Running tests with #{gemfile}..."
Rake::TestTask.new(:run_latest_release) do |t|
t.libs << "lib"
t.libs << "test"
t.pattern = "test/**/*_test.rb"
t.verbose = true
end
Rake::Task["run_latest_release"].invoke
end

desc "Test the identity_cache plugin with rails edge dependencies"
task :rails_edge do
gemfile = File.expand_path("gemfiles/Gemfile.rails-edge", __dir__)
ENV["BUNDLE_GEMFILE"] = gemfile

puts "\nInstalling dependencies for #{gemfile}..."
Bundler.with_unbundled_env do
system("bundle install --gemfile #{gemfile}") || abort("Bundle install failed")
end

puts "Running tests with #{gemfile}..."
Rake::TestTask.new(:run_rails_edge) do |t|
t.libs << "lib"
t.libs << "test"
t.pattern = "test/**/*_test.rb"
t.verbose = true
end
Rake::Task["run_rails_edge"].invoke
end
end

desc "Run default tests"
task test: ["test:default"]

desc "Run all tests (default, min_supported, latest_release, rails_edge)"
task test_all: ["test:default", "test:min_supported", "test:latest_release", "test:rails_edge"]

task :rubocop do
require "rubocop/rake_task"
RuboCop::RakeTask.new
Expand Down
4 changes: 4 additions & 0 deletions dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ commands:
bundle exec ruby -I test "$@"
fi
test-all:
desc: "Run tests for all gemfiles (default, min_supported, latest_release, rails_edge)"
run: bundle exec rake test_all

style:
desc: "Run rubocop checks"
run: bundle exec rubocop "$@"
Expand Down
52 changes: 52 additions & 0 deletions lib/identity_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ class AssociationError < StandardError; end

class InverseAssociationError < StandardError; end

class NestedDeferredParentBlockError < StandardError; end

class NestedDeferredCacheExpirationBlockError < StandardError; end

class UnsupportedScopeError < StandardError; end
Expand Down Expand Up @@ -202,6 +204,48 @@ def fetch_multi(*keys)
result
end

# Executes a block with deferred parent expiration, ensuring that the parent
# records' cache expiration is deferred until the block completes. When the block
# completes, it triggers expiration of the primary index for the parent records.
# Raises a NestedDeferredParentBlockError if a deferred parent expiration block
# is already active on the current thread.
#
# == Parameters:
# No parameters.
#
# == Raises:
# NestedDeferredParentBlockError if a deferred parent expiration block is already active.
#
# == Yield:
# Runs the provided block with deferred parent expiration.
#
# == Returns:
# The result of executing the provided block.
#
# == Ensures:
# Cleans up thread-local variables related to deferred parent expiration regardless
# of whether the block raises an exception.
def with_deferred_parent_expiration
raise NestedDeferredParentBlockError if Thread.current[:idc_deferred_parent_expiration]

if Thread.current[:idc_deferred_expiration]
deprecator.deprecation_warning("`with_deferred_parent_expiration`")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should raise, not just emit a deprecation. The two are not compatible right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should just always emit this deprecation. It shouldn't be blocked by the deferred expiration flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a warning, so will not block. We don't want to raise errors here, because we want the two methods are compatible.

The with_deferred_expiration has already covered with_deferred_parent_expiration.

  • parent deferred expiration: supported by both.
  • child deferred expiration: only supported by with_deferred_parent_expiration.
  • attribute deferred expiration: only supported by with_deferred_parent_expiration.

Copy link
Contributor Author

@drinkbeer drinkbeer Oct 21, 2024

Choose a reason for hiding this comment

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

The two methods can be nested and used together as they are compatible. We don't need to raise and fail if they are used together. The warning here is just to cover the case in this unit test: https://github.com/Shopify/identity_cache/pull/578/files#diff-a5e1bbc9f7c7519a1e9bd0e04064c349f39bf6a6a4d029651461b87ef81dde1eR258-R290

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are two separate things here.

Regarding the incompatibility, I had the same concern, @grcooper. @drinkbeer and I paired on it and the nesting should be compatible, as also evidenced by the tests.

If there is a scenario we are concerned about we should write a test for it.

However, I think we should just be deprecating the use of this method in all cases (rather than the gated warning) as we want to progress to a V2 where we remove this method and just have people use .with_deferred_expiration.

So:

  • Always fire the deprecation notice for .with_deferred_parent_expiration
  • Maintain compatibility between the two APIs for now
  • Seek to remove this method as a breaking change at the earliest convenience

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I was thinking that they were not compatible. It looks like that test proves that they are compatible though and are not duplicating work 👍

end

Thread.current[:idc_deferred_parent_expiration] = true
Thread.current[:idc_parent_records_for_cache_expiry] = Set.new

result = yield

Thread.current[:idc_deferred_parent_expiration] = nil
Thread.current[:idc_parent_records_for_cache_expiry].each(&:expire_primary_index)

result
ensure
Thread.current[:idc_deferred_parent_expiration] = nil
Thread.current[:idc_parent_records_for_cache_expiry]&.clear
end

# Executes a block with deferred cache expiration, ensuring that the records' (parent,
# children and attributes) cache expiration is deferred until the block completes. When
# the block completes, it issues delete_multi calls for all the records and attributes
Expand All @@ -225,6 +269,10 @@ def fetch_multi(*keys)
def with_deferred_expiration
raise NestedDeferredCacheExpirationBlockError if Thread.current[:idc_deferred_expiration]

if Thread.current[:idc_deferred_parent_expiration]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we deprecating here? Shouldn't we raise if they are both used? Ie with a nested deffered exception?

deprecator.deprecation_warning("`with_deferred_parent_expiration`")
end

Thread.current[:idc_deferred_expiration] = true
Thread.current[:idc_records_to_expire] = Set.new
Thread.current[:idc_attributes_to_expire] = Set.new
Expand Down Expand Up @@ -268,6 +316,10 @@ def eager_load!
ParentModelExpiration.install_all_pending_parent_expiry_hooks
end

def deprecator
@deprecator ||= ActiveSupport::Deprecation.new("1.7.0", "IdentityCache")
end

private

def fetch_in_batches(keys, &block)
Expand Down
4 changes: 4 additions & 0 deletions lib/identity_cache/parent_model_expiration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ def expire_parent_caches
add_parents_to_cache_expiry_set(parents_to_expire)
parents_to_expire.select! { |parent| parent.class.primary_cache_index_enabled }
parents_to_expire.reduce(true) do |all_expired, parent|
if Thread.current[:idc_deferred_parent_expiration]
Thread.current[:idc_parent_records_for_cache_expiry] << parent
next parent
end
parent.expire_primary_index && all_expired
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/identity_cache/version.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

module IdentityCache
VERSION = "1.6.2"
VERSION = "1.6.3"
CACHE_VERSION = 8
end
125 changes: 125 additions & 0 deletions test/index_cache_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,137 @@ def test_unique_cache_index_with_non_id_primary_key
assert_equal(123, KeyedRecord.fetch_by_value("a").id)
end

def test_with_deferred_parent_expiration_expires_parent_index_once
Item.send(:cache_has_many, :associated_records, embed: true)

@parent = Item.create!(title: "bob")
@records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }])

@memcached_spy = Spy.on(backend, :write).and_call_through

expected_item_expiration_count = Array(@parent).count
expected_associated_record_expiration_count = @records.count

expected_return_value = "Some text that we expect to see returned from the block"

result = IdentityCache.with_deferred_parent_expiration do
@parent.transaction do
@parent.associated_records.destroy_all
end
assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count)
expected_return_value
end

expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first)
item_expiration_count = expired_cache_keys.count { _1.include?("Item") }
associated_record_expiration_count = expired_cache_keys.count { _1.include?("AssociatedRecord") }

assert_operator(@memcached_spy.calls.count, :>, 0)
assert_equal(expected_item_expiration_count, item_expiration_count)
assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count)
assert_equal(expected_return_value, result)
end

def test_double_nested_deferred_parent_expiration_will_raise_error
Item.send(:cache_has_many, :associated_records, embed: true)

@parent = Item.create!(title: "bob")
@records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }])

@memcached_spy = Spy.on(backend, :write).and_call_through

assert_raises(IdentityCache::NestedDeferredParentBlockError) do
IdentityCache.with_deferred_parent_expiration do
IdentityCache.with_deferred_parent_expiration do
@parent.transaction do
@parent.associated_records.destroy_all
end
end
end
end

assert_equal(0, @memcached_spy.calls.count)
end

def test_deep_association_with_deferred_parent_expiration_expires_parent_once
AssociatedRecord.send(:has_many, :deeply_associated_records, dependent: :destroy)
Item.send(:cache_has_many, :associated_records, embed: true)

@parent = Item.create!(title: "bob")
@records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }])
@records.each do
_1.deeply_associated_records.create!([
{ name: "a", item: @parent },
{ name: "b", item: @parent },
{ name: "c", item: @parent },
])
end

@memcached_spy = Spy.on(backend, :write).and_call_through

expected_item_expiration_count = Array(@parent).count
expected_associated_record_expiration_count = @records.count
expected_deeply_associated_record_expiration_count = @records.flat_map(&:deeply_associated_records).count

IdentityCache.with_deferred_parent_expiration do
@parent.transaction do
@parent.associated_records.destroy_all
end
end

expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first)
item_expiration_count = expired_cache_keys.count { _1.include?("Item") }
associated_record_expiration_count = expired_cache_keys.count { _1.include?(":AssociatedRecord:") }
deeply_associated_record_expiration_count = expired_cache_keys.count { _1.include?("DeeplyAssociatedRecord") }

assert_operator(@memcached_spy.calls.count, :>, 0)
assert_equal(expected_item_expiration_count, item_expiration_count)
assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count)
assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_expiration_count)
end

def test_with_deferred_expiration_and_deferred_parent_expiration_is_compatible
Item.send(:cache_has_many, :associated_records, embed: true)

@parent = Item.create!(title: "bob")
@records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }])

@memcached_spy_write_multi = Spy.on(backend, :write_multi).and_call_through
@memcached_spy_write = Spy.on(backend, :write).and_call_through
expected_item_expiration_count = Array(@parent).count
expected_associated_record_expiration_count = @records.count

expected_return_value = "Some text that we expect to see returned from the block"

result =
IdentityCache.with_deferred_parent_expiration do
IdentityCache.with_deferred_expiration do
@parent.transaction do
@parent.associated_records.destroy_all
end
expected_return_value
end
end

all_keys_write = @memcached_spy_write.calls.map(&:args).map(&:first)
all_keys_write_multi = @memcached_spy_write_multi.calls.flat_map { |call| call.args.first.keys }
item_expiration_count = all_keys_write.count { _1.include?(":blob:Item:") }
associated_record_expiration_count = all_keys_write_multi.count { _1.include?(":blob:AssociatedRecord:") }

assert_equal(1, @memcached_spy_write_multi.calls.count)
assert_equal(expected_item_expiration_count, item_expiration_count)
assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count)
assert_equal(expected_return_value, result)
end

def test_with_deferred_expiration_for_parent_records_expires_parent_index_once
Item.send(:cache_has_many, :associated_records, embed: true)

@parent = Item.create!(title: "bob")
@records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }])

@memcached_spy_write_multi = Spy.on(backend, :write_multi).and_call_through
@memcached_spy_write = Spy.on(backend, :write).and_call_through
expected_item_expiration_count = Array(@parent).count
expected_associated_record_expiration_count = @records.count

Expand All @@ -193,6 +317,7 @@ def test_with_deferred_expiration_for_parent_records_expires_parent_index_once
assert_equal(expected_item_expiration_count, item_expiration_count)
assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count)
assert_equal(expected_return_value, result)
assert(@memcached_spy_write.calls.empty?)
end

def test_double_nested_deferred_expiration_for_parent_records_will_raise_error
Expand Down
Loading