Pull Request

Properly handle case when master file has been deleted avalonmediasystem#4932

app/models/master_file.rb

@@ -1,11 +1,11 @@
 # Copyright 2011-2022, The Trustees of Indiana University and Northwestern
 #   University.  Licensed under the Apache License, Version 2.0 (the "License");
 #   you may not use this file except in compliance with the License.
-#
+# 
 # You may obtain a copy of the License at
-#
+# 
 # http://www.apache.org/licenses/LICENSE-2.0
-#
+# 
 # Unless required by applicable law or agreed to in writing, software distributed
 #   under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
 #   CONDITIONS OF ANY KIND, either express or implied. See the License for the
@@ -470,9 +470,9 @@ def rdf_type
   def self.post_processing_move_filename(oldpath, options = {})
     prefix = options[:id].tr(':', '_')
     if File.basename(oldpath).start_with?(prefix)
-      File.basename(oldpath)
+      Avalon::Configuration.sanitize_filename.call(File.basename(oldpath))
     else
-      "#{prefix}-#{File.basename(oldpath)}"
+      Avalon::Configuration.sanitize_filename.call("#{prefix}-#{File.basename(oldpath)}")
     end
   end
 
@@ -512,7 +512,7 @@ def has_structuralMetadata?
 
   def to_solr *args
     super.tap do |solr_doc|
-      solr_doc['file_size_ltsi'] = file_size
+      solr_doc['file_size_ltsi'] = file_size if file_size.present?
       solr_doc['has_captions?_bs'] = has_captions?
       solr_doc['has_waveform?_bs'] = has_waveform?
       solr_doc['has_poster?_bs'] = has_poster?
@@ -559,32 +559,35 @@ def find_frame_source(options={})
     options[:offset] ||= 2000
 
     source = FileLocator.new(working_file_path&.first || file_location)
-    options[:master] = true
-    if source.source.nil? or (source.uri.scheme == 's3' and not source.exist?)
+    options[:non_temp_file] = true
+    if source.source.blank? or (source.uri.scheme == 's3' and not source.exist?)
       source = FileLocator.new(self.derivatives.where(quality_ssi: 'high').first.absolute_location)
-      options[:master] = false
+      options[:non_temp_file] = true
     end
     response = { source: source&.location }.merge(options)
     return response if response[:source].to_s =~ %r(^https?://)
 
     unless File.exists?(response[:source])
       Rails.logger.warn("Masterfile `#{file_location}` not found. Extracting via HLS.")
-      begin
-        playlist_url = self.stream_details[:stream_hls].find { |d| d[:quality] == 'high' }[:url]
-        secure_url = SecurityHandler.secure_url(playlist_url, target: self.id)
-        playlist = Avalon::M3U8Reader.read(secure_url)
-        details = playlist.at(options[:offset])
-
-        # Fixes https://github.com/avalonmediasystem/avalon/issues/3474
-        target_location = File.basename(details[:location]).split('?')[0]
-        target = File.join(Dir.tmpdir, target_location)
-        File.open(target,'wb') { |f| open(details[:location]) { |io| f.write(io.read) } }
-        response = { source: target, offset: details[:offset], master: false }
-      end
+      hls_temp_file, new_offset = create_frame_source_hls_temp_file
+      response = { source: hls_temp_file, offset: new_offset, non_temp_file: false }
     end
     return response
   end
 
+  def create_frame_source_hls_temp_file
+    playlist_url = self.stream_details[:stream_hls].find { |d| d[:quality] == 'high' }[:url]
+    secure_url = SecurityHandler.secure_url(playlist_url, target: self.id)
+    playlist = Avalon::M3U8Reader.read(secure_url)
+    details = playlist.at(options[:offset])
+
+    # Fixes https://github.com/avalonmediasystem/avalon/issues/3474
+    target_location = File.basename(details[:location]).split('?')[0]
+    target = File.join(Dir.tmpdir, target_location)
+    File.open(target,'wb') { |f| open(details[:location]) { |io| f.write(io.read) } }
+    return target, details[:offset]
+  end
+
   def extract_frame(options={})
     return unless is_video?
 
@@ -616,12 +619,12 @@ def get_ffmpeg_frame_data(frame_source, new_width, new_height, headers)
         File.symlink(frame_source[:source],file_source)
       end
       begin
-        options = ffmpeg_frame_options(file_source, jpeg.path, frame_source[:offset], new_width, new_height, frame_source[:master], headers)
+        options = ffmpeg_frame_options(file_source, jpeg.path, frame_source[:offset], new_width, new_height, frame_source[:non_temp_file], headers)
         Kernel.system(ffmpeg, *options)
         jpeg.rewind
         data = jpeg.read
         Rails.logger.debug("Generated #{data.length} bytes of data")
-        if (!frame_source[:master]) and data.length == 0
+        if (!frame_source[:non_temp_file]) and data.length == 0
           # -ss before -i is faster, but fails on some files.
           Rails.logger.warn("No data received. Swapping -ss and -i options")
           options[0..3] = options.values_at(2,3,0,1)
@@ -633,7 +636,7 @@ def get_ffmpeg_frame_data(frame_source, new_width, new_height, headers)
         data
       ensure
         File.unlink file_source unless file_source.match? %r{https?://}
-        File.unlink frame_source[:source] unless frame_source[:master] or frame_source[:source].match? %r{https?://}
+        File.unlink frame_source[:source] unless frame_source[:non_temp_file] or frame_source[:source].match? %r{https?://}
         File.unlink jpeg
       end
     end

spec/models/master_file_spec.rb

@@ -1,11 +1,11 @@
 # Copyright 2011-2022, The Trustees of Indiana University and Northwestern
 #   University.  Licensed under the Apache License, Version 2.0 (the "License");
 #   you may not use this file except in compliance with the License.
-#
+# 
 # You may obtain a copy of the License at
-#
+# 
 # http://www.apache.org/licenses/LICENSE-2.0
-#
+# 
 # Unless required by applicable law or agreed to in writing, software distributed
 #   under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
 #   CONDITIONS OF ANY KIND, either express or implied. See the License for the
@@ -559,6 +559,12 @@
       path = '/path/to/avalon_12345-video.mp4'
       expect(MasterFile.post_processing_move_filename(path, id: id).include?(id_prefix + '-' + id_prefix)).to be_falsey
     end
+    context 'path contains spaces' do
+      path = '/path/to/video file.mp4'
+      it 'removes spaces' do
+        expect(MasterFile.post_processing_move_filename(path, id: id)).not_to include(' ')
+      end
+    end
   end
 
   context 'with a working directory' do
@@ -609,6 +615,35 @@
     end
   end
 
+  describe 'find_frame_source' do
+    context 'when master_file has been deleted' do
+      subject(:video_master_file) { FactoryBot.create(:master_file, :with_media_object, :with_derivative, display_aspect_ratio: '1', file_location: '') }
+      let(:source) { video_master_file.send(:find_frame_source) }
+
+      context 'when derivatives are accessible' do
+        let(:high_derivative_locator) { FileLocator.new(video_master_file.derivatives.where(quality_ssi: 'high').first.absolute_location) }
+
+        it 'uses high derivative' do
+          expect(File).to receive(:exists?).with(high_derivative_locator.location).and_return(true)
+          expect(source[:source]).to eq high_derivative_locator.location
+          expect(source[:non_temp_file]).to eq true
+        end
+      end
+
+      context 'when derivatives are not accessible' do
+        let(:high_derivative_locator) { FileLocator.new(video_master_file.derivatives.where(quality_ssi: 'high').first.absolute_location) }
+        let(:hls_temp_file) { "/tmp/temp_segment.ts" }
+
+        it 'falls back to HLS' do
+          expect(video_master_file).to receive(:create_frame_source_hls_temp_file).and_return(hls_temp_file)
+          expect(File).to receive(:exists?).with(high_derivative_locator.location).and_return(false)
+          expect(source[:source]).to eq '/tmp/temp_segment.ts'
+          expect(source[:non_temp_file]).to eq false
+        end
+      end
+    end
+  end
+
   describe '#ffmpeg_frame_options' do
     subject { FactoryBot.create(:master_file, :with_media_object, :with_derivative, display_aspect_ratio: '1') }
 
@@ -775,7 +810,7 @@
 
     context 'without derivative' do
       let(:master_file) { FactoryBot.build(:master_file) }
-    
+
       it 'returns false' do
         expect(master_file.has_audio?).to eq false
       end
@@ -786,19 +821,19 @@
 
       context 'with audio track' do
         let(:derivative) { FactoryBot.build(:derivative, audio_codec: 'aac') }
-        
+
         it 'returns true' do
           expect(master_file.has_audio?).to eq true
         end
       end
 
       context 'without audio track' do
         let(:derivative) { FactoryBot.build(:derivative, audio_codec: nil) }
-        
+
         it 'returns false' do
           expect(master_file.has_audio?).to eq false
         end
       end
     end
   end
-end
+end