[ActiveStorage for All] Add a storage-neutral file API to Hyrax::UploadedFile

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -322,14 +322,16 @@ def update_existing_banner
 
       def add_new_banner(uploaded_file_ids)
         f = uploaded_files(uploaded_file_ids).first
-        banner_info = CollectionBrandingInfo.new(
-          collection_id: @collection.id,
-          filename: File.split(f.file_url).last,
-          role: "banner",
-          alt_txt: "",
-          target_url: ""
-        )
-        banner_info.save f.file_url
+        f.with_local_path do |path|
+          banner_info = CollectionBrandingInfo.new(
+            collection_id: @collection.id,
+            filename: File.basename(path),
+            role: "banner",
+            alt_txt: "",
+            target_url: ""
+          )
+          banner_info.save path
+        end
       end
 
       def remove_banner
@@ -347,15 +349,17 @@ def update_logo_info(uploaded_file_id, alttext, linkurl)
 
       def create_logo_info(uploaded_file_id, alttext, linkurl)
         file = uploaded_files(uploaded_file_id)
-        logo_info = CollectionBrandingInfo.new(
-          collection_id: @collection.id,
-          filename: File.split(file.file_url).last,
-          role: "logo",
-          alt_txt: alttext,
-          target_url: linkurl
-        )
-        logo_info.save file.file_url
-        logo_info
+        file.with_local_path do |path|
+          logo_info = CollectionBrandingInfo.new(
+            collection_id: @collection.id,
+            filename: File.basename(path),
+            role: "logo",
+            alt_txt: alttext,
+            target_url: linkurl
+          )
+          logo_info.save path
+          logo_info
+        end
       end
 
       def remove_redundant_files(public_files)

app/jobs/valkyrie_ingest_job.rb

@@ -43,17 +43,14 @@ def ingest(file:, pcdm_use:)
   #
   # @return [Hyrax::FileMetadata] the metadata representing the uploaded file
   def upload_file(file:, file_set:, pcdm_use:, user: nil)
-    carrier_wave_sanitized_file = file.uploader.file
-    # Pull file, since carrierwave files don't respond to a proper IO #read. See
-    # https://github.com/carrierwaveuploader/carrierwave/issues/1959
-    file_io = carrier_wave_sanitized_file.to_file
-
-    ::Hyrax::ValkyrieUpload.file(
-      io: file_io,
-      filename: carrier_wave_sanitized_file.original_filename,
-      file_set: file_set,
-      use: pcdm_use,
-      user: user
-    )
+    file.with_io do |io|
+      ::Hyrax::ValkyrieUpload.file(
+        io: io,
+        filename: file.filename,
+        file_set: file_set,
+        use: pcdm_use,
+        user: user
+      )
+    end
   end
 end

app/models/hyrax/uploaded_file.rb

@@ -31,6 +31,70 @@ def add_file_set!(file_set)
       update!(file_set_uri: uri)
     end
 
+    ##
+    # @!group Storage-neutral file API
+    #
+    # The methods in this group are the storage-backend-neutral interface for
+    # reading staged upload content and metadata. Hyrax internals outside the
+    # ActiveFedora-specific code paths should prefer these over the
+    # CarrierWave-specific {#uploader} interface, so the storage backing
+    # {UploadedFile} can change without touching every consumer.
+
+    ##
+    # @return [String, nil] filename of the staged file; nil when no file
+    #   content has been stored yet
+    def filename
+      uploader.file&.filename
+    end
+
+    ##
+    # @return [String, nil] MIME type of the staged file
+    def content_type
+      uploader.file&.content_type
+    end
+
+    ##
+    # @return [Integer, nil] size of the staged file in bytes
+    def byte_size
+      uploader.file&.size
+    end
+
+    ##
+    # @return [Boolean] whether file content has been stored for this record
+    def stored?
+      uploader.file.present?
+    end
+
+    ##
+    # Yields a rewound, binary mode IO for the staged content. The IO responds
+    # to +#path+ with a local filesystem path, as expected by Valkyrie storage
+    # adapters that move files by path. The IO is closed when the block
+    # returns.
+    #
+    # @yieldparam [IO] io readable IO positioned at the start of the file
+    # @return [Object] the return value of the block
+    # @see #stored? callers should ensure content is stored first
+    def with_io(&block)
+      raise ArgumentError, "expected a block" unless block_given?
+      with_local_path { |path| File.open(path, 'rb', &block) }
+    end
+
+    ##
+    # Yields a local filesystem path for the staged content.
+    #
+    # @note the path may be a temporary materialization depending on the
+    #   storage backend; consumers must not assume it remains valid after the
+    #   block returns.
+    #
+    # @yieldparam [String] path
+    # @return [Object] the return value of the block
+    # @see #stored? callers should ensure content is stored first
+    def with_local_path
+      raise ArgumentError, "expected a block" unless block_given?
+      yield uploader.path
+    end
+    # @!endgroup
+
     private
 
     def virus_scan

app/services/hyrax/work_uploads_handler.rb

@@ -148,8 +148,8 @@ def file_set_args(file, file_set_params = {})
         creator: Array.wrap(file.user.user_key),
         date_uploaded: file.created_at,
         date_modified: Hyrax::TimeService.time_in_utc,
-        label: file.uploader.filename,
-        title: Array.wrap(file.uploader.filename) }.merge(extra.symbolize_keys)
+        label: file.filename,
+        title: Array.wrap(file.filename) }.merge(extra.symbolize_keys)
     end
 
     ##

app/views/hyrax/uploads/create.json.jbuilder

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 json.files [@upload] do |uploaded_file|
   json.id uploaded_file.id
-  json.name uploaded_file.file&.file&.filename
-  json.size uploaded_file.file&.file&.size
+  json.name uploaded_file.filename
+  json.size uploaded_file.byte_size
   # TODO: implement these
   # json.url  "/uploads/#{uploaded_file.id}"
   # json.thumbnail_url uploaded_file.id

lib/hyrax/transactions/steps/save_collection_banner.rb

@@ -39,14 +39,16 @@ def remove_banner(collection_id:)
 
         def add_new_banner(collection_id:, uploaded_file_ids:)
           f = uploaded_files(uploaded_file_ids).first
-          banner_info = CollectionBrandingInfo.new(
-            collection_id: collection_id,
-            filename: File.split(f.file_url).last,
-            role: "banner",
-            alt_txt: "",
-            target_url: ""
-          )
-          banner_info.save f.file_url
+          f.with_local_path do |path|
+            banner_info = CollectionBrandingInfo.new(
+              collection_id: collection_id,
+              filename: File.basename(path),
+              role: "banner",
+              alt_txt: "",
+              target_url: ""
+            )
+            banner_info.save path
+          end
         end
 
         def uploaded_files(uploaded_file_ids)

lib/hyrax/transactions/steps/save_collection_logo.rb

@@ -69,15 +69,17 @@ def update_logo_info(collection_id:, uploaded_file_id:, alttext:, linkurl:)
 
         def create_logo_info(collection_id:, uploaded_file_id:, alttext:, linkurl:)
           file = uploaded_files(uploaded_file_id)
-          logo_info = CollectionBrandingInfo.new(
-            collection_id: collection_id,
-            filename: File.split(file.file_url).last,
-            role: "logo",
-            alt_txt: alttext,
-            target_url: linkurl
-          )
-          logo_info.save file.file_url
-          logo_info
+          file.with_local_path do |path|
+            logo_info = CollectionBrandingInfo.new(
+              collection_id: collection_id,
+              filename: File.basename(path),
+              role: "logo",
+              alt_txt: alttext,
+              target_url: linkurl
+            )
+            logo_info.save path
+            logo_info
+          end
         end
 
         def uploaded_files(uploaded_file_ids)

spec/models/hyrax/uploaded_file_spec.rb

@@ -13,4 +13,82 @@
     allow(Hyrax::VirusScanner).to receive(:infected?).and_return(true)
     expect(subject.errors[:file]).to include(/Virus detected/)
   end
+
+  describe 'the storage-neutral file API' do
+    describe '#filename' do
+      it 'returns the name of the stored file' do
+        expect(subject.filename).to eq 'world.png'
+      end
+
+      it 'is nil when no file is stored' do
+        expect(described_class.new.filename).to be_nil
+      end
+    end
+
+    describe '#content_type' do
+      it 'returns the MIME type of the stored file' do
+        expect(subject.content_type).to eq 'image/png'
+      end
+    end
+
+    describe '#byte_size' do
+      it 'returns the size of the stored file' do
+        expect(subject.byte_size).to eq File.size(fixture_path + '/world.png')
+      end
+    end
+
+    describe '#stored?' do
+      it 'is true when a file is stored' do
+        expect(subject).to be_stored
+      end
+
+      it 'is false when no file is stored' do
+        expect(described_class.new).not_to be_stored
+      end
+    end
+
+    describe '#with_io' do
+      it 'yields a readable IO positioned at the start of the content' do
+        content = subject.with_io(&:read)
+
+        expect(content).to eq File.binread(fixture_path + '/world.png')
+      end
+
+      it 'yields an IO with a local filesystem path' do
+        subject.with_io do |io|
+          expect(File).to exist(io.path)
+        end
+      end
+
+      it 'closes the IO after the block returns' do
+        leaked = subject.with_io { |io| io }
+
+        expect(leaked).to be_closed
+      end
+
+      it 'returns the value of the block' do
+        expect(subject.with_io { :result }).to eq :result
+      end
+
+      it 'raises ArgumentError without a block' do
+        expect { subject.with_io }.to raise_error ArgumentError
+      end
+    end
+
+    describe '#with_local_path' do
+      it 'yields a path to the stored content' do
+        subject.with_local_path do |path|
+          expect(File.binread(path)).to eq File.binread(fixture_path + '/world.png')
+        end
+      end
+
+      it 'returns the value of the block' do
+        expect(subject.with_local_path { :result }).to eq :result
+      end
+
+      it 'raises ArgumentError without a block' do
+        expect { subject.with_local_path }.to raise_error ArgumentError
+      end
+    end
+  end
 end