Attaching an unlinked Tempfile fails

jonasoberschweiber commented 2 years ago

Brief Description

Attaching an unlinked Tempfile using Attacher#attach fails with a TypeError.

Expected behavior

attach should attach the Tempfile.

Actual behavior

In extract_filename, Shrine passes the Tempfile's path, which is nil, to File::basename. basename fails because it doesn't expect nil.

When a Tempfile is unlinked, its path is set to nil, but its contents remain accessible. This is valid on POSIX systems and it's what Puma does for request bodies beyond a certain size. We're trying to attach request.body in an API handler.

For now, I've monkeypatched extract_filename like this. It seems to work, but I'm not sure if it's the optimal solution.

def extract_filename(io)
  if io.respond_to?(:original_filename)
    io.original_filename
  elsif io.respond_to?(:path) && !io.path.nil?
    File.basename(io.path)
  end
end

Simplest self-contained example code to demonstrate issue

require "sequel"
require "shrine"
require "shrine/storage/memory"
require "down"

require "bundler/setup" # if you want to debug shrine locally
require 'minitest/autorun' # if you wanna use minitest

# require 'byebug'  ## if you're using byebug
# byebug

Shrine.storages = {
  cache: Shrine::Storage::Memory.new,
  store: Shrine::Storage::Memory.new,
}

Shrine.plugin :sequel

class MyUploader < Shrine
  # plugins and uploading logic
end

DB = Sequel.sqlite # SQLite memory database
DB.create_table :posts do
  primary_key :id
  String :image_data
end

class Post < Sequel::Model
  include MyUploader::Attachment(:image)
end

class PostTest < Minitest::Test
  def test_linked_tempfile
    # This works.
    t = Tempfile.new
    attacher = MyUploader::Attacher.new
    attacher.attach(t)
  end

  def test_unlinked_tempfile
    # This fails.
    t = Tempfile.new
    t.unlink
    attacher = MyUploader::Attacher.new
    attacher.attach(t)
  end
end

PostTest#test_unlinked_tempfile:
TypeError: no implicit conversion of nil into String
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:256:in `basename'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:256:in `extract_filename'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:230:in `extract_metadata'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:288:in `get_metadata'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:207:in `upload'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine.rb:108:in `upload'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine/attacher.rb:182:in `upload'
    /Users/jobe/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/shrine-3.4.0/lib/shrine/attacher.rb:117:in `attach'
    sequel_template.rb:46:in `test_unlinked_tempfile'

System configuration

Ruby version: Reproduced on 2.7.5

Shrine version: 3.4.0

benkoshy commented 2 years ago

I can confirm that I reproduced the issue, with the report submitted (a superb report by the way - made it super easy to reproduce etc.)
As far as optimal solution for this: i cannot really comment.

jrochkind commented 2 years ago

That solution makes sense to me as a PR to shrine ... I think shrine can handle having no extracted filename already? @janko , good PR? @jonasoberschweiber , if the maintainers like it, are you interested in making a PR with a test?

shrinerb / shrine