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