to_file broken in master? #833

Closed
uberllama opened this Issue Apr 17, 2012 · 21 comments

Projects

None yet

6 participants

@uberllama

Sikachu is going to love me by the end of this month... :)

I am using the mp3info gem to retrieve id3 tag info from uploaded mp3s (using s3). This no longer works on master. When I debugged my code, the path variable was empty.

Note: I also tried moving the method to before_save. Same result.

asset.rb:


before_post_process :extract_metadata

def extract_metadata
    path = upload.to_file.path
    open_opts = { :encoding => 'utf-8' }
    Mp3Info.open(path, open_opts) do |mp3info|
      self.metadata = mp3info.tag
    end
  end
end
@sikachu
Contributor
sikachu commented Apr 17, 2012

Yeah, we've removed to_file in 3.0.1 (it was supposed to removed in 3.0.0) because we don't think that's the right way to access the file. So, you'll have to do:

  path = Paperclip.io_adapters.for(upload)

Please try that and let me know if it's still breaking.

@uberllama

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Access Denied</Message><RequestId>...</RequestId><HostId>...</HostId></Error> - cannot copy attachments//original/file.mp3 to local file /var/folders/+x/+xGad7udGgGRY2uob-DyR++++TI/-Tmp-/file.mp320120417-48297-mwi4ql

@uberllama

That's the error I get trying to use the io_adapters code. Stack trace indicates something else is happening. I just need to analyze the temp file before its copied over to s3.

@sikachu
Contributor
sikachu commented Apr 17, 2012

Oh, yeah, that shouldn't work because the file isn't on S3 yet. Sorry about that. Can you try this?

def extract_metadata
    path = upload.queued_for_write[:original].path
    open_opts = { :encoding => 'utf-8' }
    Mp3Info.open(path, open_opts) do |mp3info|
      self.metadata = mp3info.tag
    end
  end
end

I think the adapter is being dumb on not be able to recognize that the file hasn't been uploaded. I'll need to fix that.

@uberllama

That did it. Thanks Sikachu! I think I'm going to write some wiki entries for commonly needed extended functionality, such as extracting metadata, dimensions, conditionally resizing etc.Maybe that way, these chunks of code can be updated alongside the codebase.

@sikachu
Contributor
sikachu commented Apr 17, 2012

Awesome! Please do.

I'm going to keep this one open to remind me to make the adapter smarter. However, I'll not do it if it breaks the encapsulation of the adapter. Using #queued_for_write is actually more accurate, because the file didn't get written to the storage yet, hence no need to grab the file from external service.

@uberllama

Perfect thanks.

@uberllama

I've added two entries so far under a new header "Commonly needed functionality". Have to run but will add more shortly, and will try to keep these up to date with new versions.

@phene
Contributor
phene commented Apr 17, 2012

Do you intend on adding another mechanism for retrieving the attachment as a File object long after it's been uploaded to S3? It was rather sketchy to remove that method and not mention it in the release notes.

@sikachu
Contributor
sikachu commented Apr 18, 2012

@phene I don't think so. The way you'll do it now is:

Paperclip.io_adapters.for(object.attachment)

We already applied a patch that fixes the problem that made you unable to pull file down from S3, so this should work now.

@sikachu sikachu closed this Apr 22, 2012
@edison
Contributor
edison commented May 9, 2012

How can I get this by :style?

Because this...

Paperclip.io_adapters.for(object.attachment(:thumb))

will result:

Paperclip::AdapterRegistry::NoHandlerError: No handler found for "/system/attachments/1/thumb.png?1336521470"
...

Thx!

Additional:

1.9.3p194 :022 > f.asset(:thumb)
 => "/system/assets/1/thumb.png?1336521470" 
1.9.3p194 :023 > f.asset.class
 => Paperclip::Attachment 
1.9.3p194 :024 > f.asset(:thumb).class
 => String 
@phene
Contributor
phene commented May 9, 2012

Good catch. I'd have caught this if I wasn't just download originals.

@phene
Contributor
phene commented May 9, 2012

As far as a recommended solution, I'd look into writing an IoAdapter for Paperclip::Style, and invoking like such:

Paperclip.io_adapters.for(object.attachment.styles[:thumb])
@phene
Contributor
phene commented May 9, 2012

This appears to work, @edison

module Paperclip
  class AttachmentStyleAdapter
    def initialize(target)
      @target = target
      @attachment = target.attachment
      cache_current_values
    end

    def original_filename
      @original_filename
    end

    def content_type
      @content_type
    end

    def size
      @size
    end

    def nil?
      false
    end

    def fingerprint
      @fingerprint ||= Digest::MD5.file(path).to_s
    end

    def read(length = nil, buffer = nil)
      @tempfile.read(length, buffer)
    end

    def rewind
      @tempfile.rewind
    end

    def eof?
      @tempfile.eof?
    end

    def path
      @tempfile.path
    end

    private

    def cache_current_values
      @tempfile = copy_to_tempfile(@attachment)
      @original_filename = @attachment.original_filename
      @content_type = @attachment.content_type
      @size = @tempfile.size || @attachment.size
    end

    def copy_to_tempfile(src)
      dest = Tempfile.new(src.original_filename)
      dest.binmode
      if src.respond_to? :copy_to_local_file
        src.copy_to_local_file(@target.name.to_s, dest.path)
      else
        FileUtils.cp(src.path(@target.name.to_s), dest.path)
      end
      dest
    end
  end
end

Paperclip.io_adapters.register Paperclip::AttachmentStyleAdapter do |target|
  Paperclip::Style === target
end
@edison
Contributor
edison commented May 9, 2012

Hi @phene,

Thats nice, will work for s3 too?

Thx

@phene
Contributor
phene commented May 9, 2012

Created #859

@phene
Contributor
phene commented May 9, 2012

Yeah, it will work for any storage type that defines "copy_to_local_file" (both S3 and Fog do). If all you want is the raw file, you could create a tmpfile and pass the path like such:

user.photo # Paperclip::Attachment

dest = Tempfile.new(user.photo_file_name)
dest.binmode

user.photo.copy_to_local_file(:thumb, dest.path)
@edison
Contributor
edison commented May 9, 2012

Cool! Thank you

@mwalkden

This was the thing that finally worked for me. Thank you!

@kandadaboggu

How to get access to the @tempfile? I didn't find any accessor in the Paperclip::AttachmentAdapter files.

I need to access the tempfile so that I can pass it to a CSV parser. I am using hack to get around this issue, i.e.

ioa = Paperclip.io_adapters.for(ht.request_doc)
# HACK find out how to get access to the tempfile
file = ioa.instance_variable_get("@tempfile")

Is there a better way to access the tempfile?

@mwalkden

Here is what worked for me:

    dest = Tempfile.new(upload.batch_upload_file_name)
    dest.binmode
    upload.batch_upload.copy_to_local_file(:default_style, dest.path)
    file_loc = dest.path

    if upload.batch_upload_file_name.to_s.include?('.csv')

      require 'csv'

        CSV.foreach(file_loc, :headers => true, :skip_blanks => true) do |row|
        <code>
        end
     end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment