Merge pull request #2089 from samvera/hyrax-5-upgrade-spec-fixing

Hyrax 5 upgrade spec fixing

Gemfile

@@ -70,6 +70,7 @@ gem 'puma', '~> 5.6' # Use Puma as the app server
 gem 'rack-test', '0.7.0', group: %i[test] # rack-test >= 0.71 does not work with older Capybara versions (< 2.17). See #214 for more details
 gem 'rails-controller-testing', group: %i[test]
 gem 'rdf', '~> 3.2'
+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 'redlock', '>= 0.1.2', '< 2.0' # lock redlock per https://github.com/samvera/hyrax/pull/5961
 gem 'riiif', '~> 2.0'
 gem 'rolify'

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

app/models/account.rb

@@ -111,14 +111,16 @@ def switch_host!(cname)
     Hyrax::Engine.routes.default_url_options[:host] = cname
   end
 
+  DEFAULT_FILE_CACHE_STORE = ENV.fetch('HYKU_CACHE_ROOT', '/app/samvera/file_cache')
+
   def setup_tenant_cache(is_enabled)
     Rails.application.config.action_controller.perform_caching = is_enabled
     ActionController::Base.perform_caching = is_enabled
     # rubocop:disable Style/ConditionalAssignment
     if is_enabled
       Rails.application.config.cache_store = :redis_cache_store, { url: Redis.current.id }
     else
-      Rails.application.config.cache_store = :file_store, ENV.fetch('HYKU_CACHE_ROOT', '/app/samvera/file_cache')
+      Rails.application.config.cache_store = :file_store, DEFAULT_FILE_CACHE_STORE
     end
     # rubocop:enable Style/ConditionalAssignment
     Rails.cache = ActiveSupport::Cache.lookup_store(Rails.application.config.cache_store)

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

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
 

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')

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
 

spec/models/account_spec.rb

@@ -130,7 +130,7 @@
       it "reverts to using file store when cache is off" do
         account.settings[:cache_api] = false
         account.switch!
-        expect(Rails.application.config.cache_store).to eq([:file_store, kind_of(String)])
+        expect(Rails.application.config.cache_store).to eq([:file_store, described_class::DEFAULT_FILE_CACHE_STORE])
       end
     end
 
@@ -140,7 +140,7 @@
       it "uses the file store" do
         expect(Rails.application.config.action_controller.perform_caching).to be_falsey
         expect(ActionController::Base.perform_caching).to be_falsey
-        expect(Rails.application.config.cache_store).to eq([:file_store, kind_of(String)])
+        expect(Rails.application.config.cache_store).to eq([:file_store, described_class::DEFAULT_FILE_CACHE_STORE])
       end
     end
 

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:)
+      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

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

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