From 0dbddad8acb132e40b8c72405c69ef74bed0bb8e Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 20 Dec 2023 14:40:45 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20Re-arrange=20CleanupAccountJob?= =?UTF-8?q?=20specs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `CleanupAccountJob` was stubbing very nosily; needing to know too much about implementation details of the end-points. Instead this preserves the over-view spec (e.g. what all the cleanup spec actually cleans up) while moving that nosy logic to the constituent endpoint. Most of these specs are testing that the method chains work; which is perhaps adequate as the other option is far more expensive tests (e.g. make a new Fedora node only to then immediately destroy it) I'm also leveraging the new `Redis::Namespace#clear` method. Related to: - https://github.com/resque/redis-namespace/pull/202 --- Gemfile | 1 + Gemfile.lock | 1 + app/models/fcrepo_endpoint.rb | 2 +- app/models/redis_endpoint.rb | 10 ++-------- app/models/solr_endpoint.rb | 3 +++ spec/jobs/cleanup_account_job_spec.rb | 22 ++++++---------------- spec/models/fcrepo_endpoint_spec.rb | 21 +++++++++++++++++++-- spec/models/redis_endpoint_spec.rb | 20 +++++++++++++++----- spec/models/solr_endpoint_spec.rb | 9 +++++++++ 9 files changed, 57 insertions(+), 32 deletions(-) diff --git a/Gemfile b/Gemfile index f3c4dda27..02a857b32 100644 --- a/Gemfile +++ b/Gemfile @@ -71,6 +71,7 @@ gem 'rack-test', '0.7.0', group: %i[test] # rack-test >= 0.71 does not work with gem 'rails-controller-testing', group: %i[test] gem 'rdf', '~> 3.2' gem 'redlock', '>= 0.1.2', '< 2.0' # lock redlock per https://github.com/samvera/hyrax/pull/5961 +gem 'redis-namespace', '~> 1.10' # Hyrax v5 relies on 1.5; but we'd like to have the #clear method so we need 1.10 or greater. gem 'riiif', '~> 2.0' gem 'rolify' gem 'rsolr', '~> 2.0' diff --git a/Gemfile.lock b/Gemfile.lock index 01f9501af..9e330d35d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1437,6 +1437,7 @@ DEPENDENCIES rails (~> 6.0) rails-controller-testing rdf (~> 3.2) + redis-namespace (~> 1.10) redlock (>= 0.1.2, < 2.0) riiif (~> 2.0) rolify diff --git a/app/models/fcrepo_endpoint.rb b/app/models/fcrepo_endpoint.rb index 7b02ff281..cd5a6e88b 100644 --- a/app/models/fcrepo_endpoint.rb +++ b/app/models/fcrepo_endpoint.rb @@ -27,7 +27,7 @@ def ping def remove! switch! # Preceding slash must be removed from base_path when calling delete() - path = base_path.sub!(%r{^/}, '') + path = base_path.sub(%r{^/}, '') ActiveFedora.fedora.connection.delete(path) destroy end diff --git a/app/models/redis_endpoint.rb b/app/models/redis_endpoint.rb index 8003b9e6c..3cd68f707 100644 --- a/app/models/redis_endpoint.rb +++ b/app/models/redis_endpoint.rb @@ -21,14 +21,8 @@ def ping # Remove all the keys in Redis in this namespace, then destroy the record def remove! switch! - # Redis::Namespace currently doesn't support flushall or flushdb. - # See https://github.com/resque/redis-namespace/issues/56 - # So, instead we select all keys in current namespace and delete - keys = redis_instance.keys '*' - return if keys.empty? - # Delete in slices to avoid "stack level too deep" errors for large numbers of keys - # See https://github.com/redis/redis-rb/issues/122 - keys.each_slice(1000) { |key_slice| redis_instance.del(*key_slice) } + # redis-namespace v1.10.0 introduced clear https://github.com/resque/redis-namespace/pull/202 + redis_instance.clear destroy end diff --git a/app/models/solr_endpoint.rb b/app/models/solr_endpoint.rb index be217ab81..3e90a6812 100644 --- a/app/models/solr_endpoint.rb +++ b/app/models/solr_endpoint.rb @@ -29,6 +29,9 @@ def switch! # Remove the solr collection then destroy this record def remove! + # NOTE: Other end points first call switch!; is that an oversight? Perhaps not as we're relying + # on a scheduled job to do the destructive work. + # Spin off as a job, so that it can fail and be retried separately from the other logic. if account.search_only? RemoveSolrCollectionJob.perform_later(collection, connection_options, 'cross_search_tenant') diff --git a/spec/jobs/cleanup_account_job_spec.rb b/spec/jobs/cleanup_account_job_spec.rb index 77e4a76f9..3823ccc4b 100644 --- a/spec/jobs/cleanup_account_job_spec.rb +++ b/spec/jobs/cleanup_account_job_spec.rb @@ -10,39 +10,29 @@ end before do - allow(RemoveSolrCollectionJob).to receive(:perform_later) - allow(account.fcrepo_endpoint).to receive(:switch!) - allow(ActiveFedora.fedora.connection).to receive(:delete) + allow(account.solr_endpoint).to receive(:remove!) + allow(account.fcrepo_endpoint).to receive(:remove!) + allow(account.redis_endpoint).to receive(:remove!) allow(Apartment::Tenant).to receive(:drop).with(account.tenant) end it 'destroys the solr collection' do - expect(RemoveSolrCollectionJob).to receive(:perform_later).with('x', hash_including('url')) - expect(account.solr_endpoint).to receive(:destroy) + expect(account.solr_endpoint).to receive(:remove!) described_class.perform_now(account) end it 'destroys the fcrepo collection' do - expect(ActiveFedora.fedora.connection).to receive(:delete).with('x') - expect(account.fcrepo_endpoint).to receive(:destroy) + expect(account.fcrepo_endpoint).to receive(:remove!) described_class.perform_now(account) end it 'deletes all entries in the redis namespace' do - allow(Redis.current).to receive(:keys).and_return(["x:events:x1", "x:events:x2"]) - allow(Hyrax::RedisEventStore).to receive(:instance).and_return( - Redis::Namespace.new(account.redis_endpoint.namespace, redis: Redis.current) - ) - expect(Hyrax::RedisEventStore.instance.namespace).to eq('x') - expect(Hyrax::RedisEventStore.instance.keys).to eq(["events:x1", "events:x2"]) - expect(Hyrax::RedisEventStore.instance).to receive(:del).with('events:x1', 'events:x2') - expect(account.redis_endpoint).to receive(:destroy) + expect(account.redis_endpoint).to receive(:remove!) described_class.perform_now(account) end it 'destroys the tenant database' do expect(Apartment::Tenant).to receive(:drop).with(account.tenant) - described_class.perform_now(account) end diff --git a/spec/models/fcrepo_endpoint_spec.rb b/spec/models/fcrepo_endpoint_spec.rb index 90ca23032..bce230d69 100644 --- a/spec/models/fcrepo_endpoint_spec.rb +++ b/spec/models/fcrepo_endpoint_spec.rb @@ -2,10 +2,9 @@ RSpec.describe FcrepoEndpoint do let(:base_path) { 'foobar' } + subject { described_class.new base_path: } describe '.options' do - subject { described_class.new base_path: } - it 'uses the configured application settings' do expect(subject.options[:base_path]).to eq base_path end @@ -28,4 +27,22 @@ expect(subject.ping).to be_falsey end end + + describe '#remove!' do + it 'removes the base node in fedora and deletes this endpoint' do + subject.save! + # All of this stubbing doesn't tell us much; except that the method chain is valid. Which is perhaps better than the two options: + # + # 1. Creating the Fedora node then tearing it down. + # 2. Not testing this at all. + # + # What I found is that I could not stub the last receiver in the chain; as it would still + # attempt a HEAD request. So here is the "test". + connection = double(ActiveFedora::CachingConnection, delete: true) + fedora = double(ActiveFedora::Fedora, connection: connection) + expect(ActiveFedora).to receive(:fedora).and_return(fedora) + expect(connection).to receive(:delete).with(base_path) + expect { subject.remove! }.to change(described_class, :count).by(-1) + end + end end diff --git a/spec/models/redis_endpoint_spec.rb b/spec/models/redis_endpoint_spec.rb index def3074c9..bb45aaa5f 100644 --- a/spec/models/redis_endpoint_spec.rb +++ b/spec/models/redis_endpoint_spec.rb @@ -2,26 +2,36 @@ RSpec.describe RedisEndpoint do let(:namespace) { 'foobar' } + let(:faux_redis_instance) { double(Hyrax::RedisEventStore, ping: 'PONG', clear: true) } + before { allow(subject).to receive(:redis_instance).and_return(faux_redis_instance) } + subject { described_class.new(namespace:) } describe '.options' do - subject { described_class.new(namespace:) } - it 'uses the configured application settings' do expect(subject.options[:namespace]).to eq namespace end end describe '#ping' do - let(:faux_redis_instance) { double(Hyrax::RedisEventStore, ping: 'PONG') } it 'checks if the service is up' do - allow(subject).to receive(:redis_instance).and_return(faux_redis_instance) + allow(faux_redis_instance).to receive(:ping).and_return("PONG") expect(subject.ping).to be_truthy end it 'is false if the service is down' do allow(faux_redis_instance).to receive(:ping).and_raise(RuntimeError) - allow(subject).to receive(:redis_instance).and_return(faux_redis_instance) expect(subject.ping).to eq false end end + + describe '#remove!' do + subject { described_class.create! } + + it 'clears the namespace and deletes itself' do + expect(faux_redis_instance).to receive(:clear) + expect do + subject.remove! + end.to change(described_class, :count).by(-1) + end + end end diff --git a/spec/models/solr_endpoint_spec.rb b/spec/models/solr_endpoint_spec.rb index e93d6e8c9..7a6f00c68 100644 --- a/spec/models/solr_endpoint_spec.rb +++ b/spec/models/solr_endpoint_spec.rb @@ -61,4 +61,13 @@ expect(subject.ping).to eq false end end + + describe '#remove!' do + it 'schedules the removal and deletes the end point' do + instance = described_class.create! + allow(instance).to receive(:account).and_return(double(Account, search_only?: true)) + expect(RemoveSolrCollectionJob).to receive(:perform_later) + expect { instance.remove! }.to change(described_class, :count).by(-1) + end + end end