Skip to content
This repository was archived by the owner on Jul 13, 2023. It is now read-only.

The uri io adapter should use the content-disposition filename #2210

Closed
wants to merge 5 commits into from
Closed

The uri io adapter should use the content-disposition filename #2210

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a890f98
The uri io adapter should use the content-disposition filename
netmask May 11, 2016
fe1197d
Fix style comments
netmask May 11, 2016
a13fa20
URI Adapter refactoring
netmask May 12, 2016
3c5f9db
Fix comment styles
netmask May 12, 2016
2bc3fbb
- Removed editor tracking file
netmask May 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion lib/paperclip/io_adapters/uri_adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ def download_content
end

def cache_current_values
@original_filename = @target.path.split("/").last
extract_attachment_filename

@original_filename ||= @target.path.split("/").last
@original_filename ||= "index.html"

self.original_filename = @original_filename.strip

@content_type = @content.content_type if @content.respond_to?(:content_type)
Expand All @@ -28,6 +31,13 @@ def cache_current_values
@size = @content.size
end

def extract_attachment_filename
if @content.meta.has_key? "content-disposition"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will @content always have the meta method defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, the adapter uses the open uri module which always contains the meta hash

@original_filename = @content.meta["content-disposition"]
.match(/filename="([^"]*)"/)[1]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.
Use 2 (not 25) spaces for indenting an expression in an assignment spanning multiple lines.

end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are here we can refactor some code. Suggestion:

    def cache_current_values
      self.content_type = content_type_from_content || "text/html"

      self.original_filename =
        filename_from_content_disposition ||
        filename_from_path ||
        default_filename

      @size = @content.size
    end

    def content_type_from_content
      if @content.respond_to?(:content_type)
        @content.content_type
      end
    end

    def filename_from_content_disposition
      if @content.meta.has_key?("content-disposition")
        @original_filename = @content.meta["content-disposition"]
                                 .match(/filename="([^"]*)"/)[1]
      end
    end

    def filename_from_path
      @target.path.split("/").last
    end

    def default_filename
      "index.html"
    end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks much better 👍


def copy_to_tempfile(src)
while data = src.read(16*1024)
destination.write(data)
Expand Down
13 changes: 8 additions & 5 deletions spec/paperclip/io_adapters/http_url_proxy_adapter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
require 'spec_helper'

describe Paperclip::HttpUrlProxyAdapter do
before do
@open_return = StringIO.new("xxx")
@open_return.stubs(:content_type).returns("image/png")
@open_return.stubs(:meta).returns({})
Paperclip::HttpUrlProxyAdapter.any_instance.
stubs(:download_content).returns(@open_return)
end

context "a new instance" do
before do
@open_return = StringIO.new("xxx")
@open_return.stubs(:content_type).returns("image/png")
Paperclip::HttpUrlProxyAdapter.any_instance.stubs(:download_content).returns(@open_return)
@url = "http://thoughtbot.com/images/thoughtbot-logo.png"
@subject = Paperclip.io_adapters.for(@url)
end
Expand Down Expand Up @@ -60,7 +65,6 @@

context "a url with query params" do
before do
Paperclip::HttpUrlProxyAdapter.any_instance.stubs(:download_content).returns(StringIO.new("x"))
@url = "https://github.com/thoughtbot/paperclip?file=test"
@subject = Paperclip.io_adapters.for(@url)
end
Expand All @@ -76,7 +80,6 @@

context "a url with restricted characters in the filename" do
before do
Paperclip::HttpUrlProxyAdapter.any_instance.stubs(:download_content).returns(StringIO.new("x"))
@url = "https://github.com/thoughtbot/paper:clip.jpg"
@subject = Paperclip.io_adapters.for(@url)
end
Expand Down
37 changes: 31 additions & 6 deletions spec/paperclip/io_adapters/uri_adapter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
require 'spec_helper'

describe Paperclip::UriAdapter do
let(:content_type) { "image/png" }
let(:meta) { {} }

before do
@open_return = StringIO.new("xxx")
@open_return.stubs(:content_type).returns(content_type)
@open_return.stubs(:meta).returns(meta)
Paperclip::UriAdapter.any_instance.
stubs(:download_content).returns(@open_return)
end

context "a new instance" do
before do
@open_return = StringIO.new("xxx")
@open_return.stubs(:content_type).returns("image/png")
Paperclip::UriAdapter.any_instance.stubs(:download_content).returns(@open_return)
@uri = URI.parse("http://thoughtbot.com/images/thoughtbot-logo.png")
@subject = Paperclip.io_adapters.for(@uri)
end
Expand Down Expand Up @@ -56,8 +64,9 @@
end

context "a directory index url" do
let(:content_type) { "text/html" }

before do
Paperclip::UriAdapter.any_instance.stubs(:download_content).returns(StringIO.new("xxx"))
@uri = URI.parse("http://thoughtbot.com")
@subject = Paperclip.io_adapters.for(@uri)
end
Expand All @@ -73,7 +82,6 @@

context "a url with query params" do
before do
Paperclip::UriAdapter.any_instance.stubs(:download_content).returns(StringIO.new("xxx"))
@uri = URI.parse("https://github.com/thoughtbot/paperclip?file=test")
@subject = Paperclip.io_adapters.for(@uri)
end
Expand All @@ -83,9 +91,26 @@
end
end

context "a url with content disposition headers" do
let(:file_name) { "test_document.pdf" }
let(:meta) do
{
"content-disposition" => "attachment; filename=\"#{file_name}\";",
}
end

before do
@uri = URI.parse("https://github.com/thoughtbot/paperclip?file=test")
@subject = Paperclip.io_adapters.for(@uri)
end

it "returns a file name" do
assert_equal file_name, @subject.original_filename
end
end

context "a url with restricted characters in the filename" do
before do
Paperclip::UriAdapter.any_instance.stubs(:download_content).returns(StringIO.new("xxx"))
@uri = URI.parse("https://github.com/thoughtbot/paper:clip.jpg")
@subject = Paperclip.io_adapters.for(@uri)
end
Expand Down