Merge pull request #91 from insales/fix_styles_tmp

Delete intermediary tempfiles

.gitignore

@@ -4,3 +4,4 @@ gemfiles/*.lock
 gemfiles/.bundle/
 tmp/
 .rubocop-*
+/coverage/

.pryrc

@@ -0,0 +1,7 @@
+# Alieases for debugger.
+if defined?(PryByebug) || defined?(PryDebugger)
+  Pry.commands.alias_command 'c', 'continue'
+  Pry.commands.alias_command 's', 'step'
+  Pry.commands.alias_command 'n', 'next'
+  Pry.commands.alias_command 'f', 'finish'
+end

Gemfile

@@ -15,6 +15,7 @@ gem 'rails'
 gem 'sidekiq'
 
 gem 'test-unit'
+gem 'simplecov', require: false
 gem 'mocha'
 gem 'thoughtbot-shoulda', '>= 2.9.0'
 

Rakefile

@@ -3,7 +3,6 @@ require 'rake/testtask'
 require 'rdoc/task'
 
 $LOAD_PATH << File.join(File.dirname(__FILE__), 'lib')
-require 'paperclip'
 
 import 'lib/tasks/paperclip_tasks.rake'
 
@@ -19,7 +18,7 @@ Rake::TestTask.new(:test) do |t|
 end
 
 desc 'Start an IRB session with all necessary files required.'
-task :shell do
+task :shell => :load_paperclip do
   chdir File.dirname(__FILE__)
   exec 'irb -I lib/ -I lib/paperclip -r rubygems -r active_record -r tempfile -r init'
 end
@@ -62,23 +61,28 @@ exclude_file_globs = ["test/s3.yml",
                       "test/pkg/*",
                       "test/tmp",
                       "test/tmp/*"]
-spec = Gem::Specification.new do |s|
-  s.name              = "paperclip"
-  s.version           = Paperclip::VERSION
-  s.author            = "Jon Yurek"
-  s.email             = "jyurek@thoughtbot.com"
-  s.homepage          = "http://www.thoughtbot.com/projects/paperclip"
-  s.platform          = Gem::Platform::RUBY
-  s.summary           = "File attachments as attributes for ActiveRecord"
-  s.files             = FileList[include_file_globs].to_a - FileList[exclude_file_globs].to_a
-  s.require_path      = "lib"
-  s.test_files        = FileList["test/**/test_*.rb"].to_a
-  s.rubyforge_project = "paperclip"
-  s.extra_rdoc_files  = FileList["README*"].to_a
-  s.rdoc_options << '--line-numbers' << '--inline-source'
-  s.requirements << "ImageMagick"
-  s.add_development_dependency 'thoughtbot-shoulda'
-  s.add_development_dependency 'mocha'
+
+def spec
+  # TODO: require 'paperclip/version'
+  require 'paperclip'
+  Gem::Specification.new do |s|
+    s.name              = "paperclip"
+    s.version           = Paperclip::VERSION
+    s.author            = "Jon Yurek"
+    s.email             = "jyurek@thoughtbot.com"
+    s.homepage          = "http://www.thoughtbot.com/projects/paperclip"
+    s.platform          = Gem::Platform::RUBY
+    s.summary           = "File attachments as attributes for ActiveRecord"
+    s.files             = FileList[include_file_globs].to_a - FileList[exclude_file_globs].to_a
+    s.require_path      = "lib"
+    s.test_files        = FileList["test/**/test_*.rb"].to_a
+    s.rubyforge_project = "paperclip"
+    s.extra_rdoc_files  = FileList["README*"].to_a
+    s.rdoc_options << '--line-numbers' << '--inline-source'
+    s.requirements << "ImageMagick"
+    s.add_development_dependency 'thoughtbot-shoulda'
+    s.add_development_dependency 'mocha'
+  end
 end
 
 desc "Print a list of the files to be put into the gem"

lib/paperclip/attachment.rb

@@ -314,7 +314,7 @@ module Paperclip
     # thumbnails forcefully, by reobtaining the original file and going through
     # the post-process again.
     def reprocess!
-      new_original = Tempfile.new("paperclip-reprocess-#{instance.id}-")
+      new_original = Tempfile.new("paperclip-reprocess-#{instance.class.table_name}-#{instance.id}-")
       new_original.binmode
       old_original = to_file(:original)
       new_original.write( old_original.read )
@@ -431,7 +431,11 @@ module Paperclip
         begin
           raise RuntimeError.new("Style #{name} has no processors defined.") if args[:processors].blank?
           queued_for_write[name] = args[:processors].inject(queued_for_write[:original]) do |file, processor|
-            Paperclip.processor(processor).make(file, args, self)
+            Paperclip.processor(processor).make(file, args, self).tap do |new_file|
+              # closing intermediary tempfiles
+              file.close! if new_file != file && file.is_a?(Tempfile) &&
+                             (name == :original || !queued_for_write.value?(file))
+            end
           end
         rescue PaperclipError => e
           log("An error was received while processing: #{e.inspect}")

lib/paperclip/optimizer.rb

@@ -3,12 +3,10 @@ require 'open3'
 module Paperclip
   class Optimizer < Processor
     def make
-      optimized_file_path  = optimize(@file)
-      if optimized_file_path && File.exist?(optimized_file_path)
-        return File.open(optimized_file_path)
-      else
-        return @file
-      end
+      optimized_file = optimize(@file)
+      return @file unless optimized_file && optimized_file.size > 0 # rubocop:disable Style/ZeroLengthPredicate
+
+      optimized_file
     end
 
     def real_content_type
@@ -18,9 +16,9 @@ module Paperclip
     def optimize(_file)
       # TODO: use the arg?
       src = @file.path
-      dst = "#{src}-#{SecureRandom.hex}"
+      dst_file = Tempfile.new(["#{File.basename(src)}-optim", File.extname(src)])
       src_shell = src.shellescape
-      dst_shell = dst.shellescape
+      dst_shell = dst_file.path.shellescape
       cmd = case real_content_type
             when 'image/jpeg', 'image/jpg', 'image/pjpeg'
               "cp #{src_shell} #{dst_shell} && jpegoptim --all-progressive -q --strip-com --strip-exif --strip-iptc -- #{dst_shell}"
@@ -32,7 +30,11 @@ module Paperclip
               return
             end
       run_and_verify!(cmd)
-      dst
+      dst_file
+    rescue StandardError => e
+      dst_file.close!
+      Paperclip.log("Error: cannot optimize: #{e}")
+      nil
     end
 
     private

lib/paperclip/recursive_thumbnail.rb

@@ -5,8 +5,12 @@ module Paperclip
       # если по каким-то причинам не сформировался файл
       # для прыдущего размера не кидаем ексепшен и
       # генерим файл из оригинального
-      f = attachment.to_file(options[:thumbnail] || :original) rescue file
-      super f, options, attachment
+      source_style = options[:thumbnail] || :original
+      # TODO: вообще queued_for_write[source_style] место в NoCacheS3#to_file
+      f = attachment&.queued_for_write&.dig(source_style)&.tap(&:rewind)
+      # TODO: и надо сносить файл если все же была загрузка
+      f ||= attachment.to_file(source_style) rescue file # rubocop:disable Style/RescueModifier
+      super(f, options, attachment)
     end
   end
 end

lib/paperclip/storage/no_cache_s3.rb

@@ -142,9 +142,8 @@ module Paperclip
           (self.class.store_ids - [self.class.main_store_id]).each { |store_id| enqueue_sync_job(store_id) }
         end
         # HACK: Iostream пишет в tempfile, и он нигде не закрывается. Будем закрывать хотя бы тут
-        if queued_for_write[:original].is_a?(Tempfile)
-          queued_for_write[:original].close
-          queued_for_write[:original].unlink
+        queued_for_write.each_value do |file|
+          file.is_a?(Tempfile) && file.close!
         end
         queued_for_write.clear
       end

lib/paperclip/thumbnail.rb

@@ -46,7 +46,7 @@ module Paperclip
     def make
       src = @file
       ext = @format.present? ? ".#{@format}" : nil
-      dst = Tempfile.new([@basename, ext])
+      dst = Tempfile.new(["#{@basename}-thumb-", ext])
       dst.binmode
 
       command = <<-end_command

test/storage/no_cache_s3_test.rb

@@ -4,6 +4,7 @@ require 'test_helper'
 require 'sidekiq'
 require 'sidekiq/testing'
 require 'aws-sdk-s3'
+require 'base64'
 
 require 'delayed_paperclip'
 DelayedPaperclip::Railtie.insert
@@ -29,6 +30,12 @@ class NoCacheS3Test < Test::Unit::TestCase
       stores: {
         store_1: { access_key_id: '123', secret_access_key: '123', region: 'r', bucket: 'buck' },
         store_2: { access_key_id: '456', secret_access_key: '456', region: 'r', bucket: 'buck' }
+      },
+      styles: {
+        original: { geometry: '4x4>', processors: %i[thumbnail optimizer] },
+        medium: '3x3',
+        small: { geometry: '2x2', processors: [:recursive_thumbnail], thumbnail: :medium },
+        micro: { geometry: '1x1', processors: [:recursive_thumbnail], thumbnail: :small }
       }
     )
     modify_table(:dummies) do |table|
@@ -36,12 +43,14 @@ class NoCacheS3Test < Test::Unit::TestCase
       table.boolean :avatar_synced_to_store_2, null: false, default: false
     end
     @instance = Dummy.create
-    @store1_stub = mock
-    @store2_stub = mock
+    @store1_stub = mock("store1")
+    @store2_stub = mock("store2")
     @store1_stub.stubs(:url).returns('http://store.local')
     @store2_stub.stubs(:url).returns('http://store.local')
     @instance.avatar.class.stubs(:stores).returns({ store_1: @store1_stub, store_2: @store2_stub })
-    Dummy::AvatarAttachment.any_instance.stubs(:to_file).returns(stub_file('text.txt', 'qwe'))
+    Dummy::AvatarAttachment.any_instance.stubs(:to_file).returns(
+      stub_file('pixel.gif', Base64.decode64('R0lGODlhAQABAIABAP///wAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw'))
+    )
   end
 
   teardown { TEST_ROOT.rmtree if TEST_ROOT.exist? }
@@ -83,6 +92,22 @@ class NoCacheS3Test < Test::Unit::TestCase
     end
   end
 
+  context "reprocess" do
+    setup do
+      Sidekiq::Testing.fake!
+      @instance.update_columns avatar_file_name: 'foo.gif', avatar_content_type: 'image/gif'
+    end
+
+    should "delete tmp files" do
+      @store1_stub.expects(:put_object).times(1 + (@instance.avatar.options[:styles].keys - [:original]).size)
+      # Paperclip.expects(:log).with { puts "Log: #{_1}"; true }.at_least(3)
+      existing_files = Dir.children(Dir.tmpdir)
+      @instance.avatar.reprocess!
+      leftover_files = (Dir.children(Dir.tmpdir) - existing_files).sort
+      assert_empty(leftover_files)
+    end
+  end
+
   context 'generating presigned_url' do
     setup do
       Dummy::AvatarAttachment.any_instance.stubs(:storage_url).returns('http://домен.pф/ключ?param1=параметр')

test/test_helper.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require 'rubygems'
+require 'bundler/setup'
+
 require 'test/unit'
 require 'shoulda'
 require 'mocha/test_unit'
@@ -13,6 +15,24 @@ require 'active_record'
 require 'active_support'
 require 'rails'
 
+if ENV['COVERAGE']
+  require 'simplecov'
+
+  SimpleCov.external_at_exit = true
+  Test::Unit.at_exit do
+    SimpleCov.at_exit_behavior
+  end
+  SimpleCov.command_name 'test:units'
+  SimpleCov.start do
+    load_profile "test_frameworks"
+
+    add_group "Storage", "lib/paperclip/storage/"
+    add_group "Libraries", "lib/"
+
+    track_files "{lib}/**/*.rb"
+  end
+end
+
 ROOT = File.expand_path('../', __dir__)
 
 ENV['RAILS_ENV'] = 'test'
@@ -20,9 +40,10 @@ class TestRailsApp < Rails::Application; end
 Rails.application.config.root = "#{ROOT}/tmp/rails"
 
 $LOAD_PATH << File.join(ROOT, 'lib')
-$LOAD_PATH << File.join(ROOT, 'lib', 'paperclip')
+$LOAD_PATH << File.join(ROOT, 'lib/paperclip') # ??
 
-require File.join(ROOT, 'lib', 'paperclip.rb')
+require 'paperclip'
+# require File.join(ROOT, 'lib/paperclip.rb')
 
 require 'shoulda_macros/paperclip'