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

netmask · 2016-05-11T16:00:57Z

When a URI is passed to the model attachment as the "attachment source"

example:

model.document = "http://examle.com/download_pdf"

paperclip will download this resource and set the last element of the path in this case, "download_pdf" as the file name, This PR allows the URI adapter to use the "content-disposition" header if is available and set that value as the original filename which in most cases are the expected one.

model.document = "http://examle.com/download_pdf"
# the response header contains the real name
# Content-disposition: attachment; filename=awesome_pdf_2016.pdf
# model.document.url => /storage/awesome_pdf_2016.pdf instead of download_pdf

- The uri io adapter now seeks for the content-disposition header if this is pressent the value filename is taken instead of the last path segment for the resource file name

houndci-bot · 2016-05-11T16:01:03Z

spec/paperclip/io_adapters/uri_adapter_spec.rb

@@ -83,9 +90,22 @@
    end
  end

+  context "a url with content disposition headers" do
+    let(:file_name) { 'test_document.pdf' }


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-05-11T16:33:28Z

spec/paperclip/io_adapters/uri_adapter_spec.rb

+    let(:file_name) { "test_document.pdf" }
+    let(:meta) {
+      {
+          "content-disposition" => "attachment; filename=\"#{file_name}\";"


Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
Put a comma after the last item of a multiline hash.

tute · 2016-05-12T14:34:37Z

lib/paperclip/io_adapters/uri_adapter.rb

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


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

this looks much better 👍

tute · 2016-05-12T14:37:09Z

Looks good! Thank you.

Left a comment on a potential refactor. What do you think of it?
Can you please add a note to the NEWS file?
If this is backwards compatible, do you think we should only apply it to the master branch, and not the v4.3 one?

Again, thanks for your contribution! 🎉

netmask · 2016-05-12T16:22:10Z

I think this should be applied also to v4.3 branch because is the latest stable version :)

houndci-bot · 2016-05-12T16:44:55Z

lib/paperclip/io_adapters/uri_adapter.rb

+    end
+
+    def default_filename
+      'index.html'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

- Applied the Tute Costa refactor to URI Adapter. - Added entry to the NEWS file.

houndci-bot · 2016-05-12T16:57:21Z

lib/paperclip/io_adapters/uri_adapter.rb


-    private
+      self.original_filename = filename_from_content_disposition ||
+        filename_from_path || default_filename


Align the operands of an expression in an assignment spanning multiple lines.

tute · 2016-05-12T16:59:09Z

I think this should be applied also to v4.3 branch because is the latest stable version :)

But this changes behavior, and is thus backwards incompatible.

tute · 2016-05-12T16:59:24Z

We can consider it a bugfix, though, indeed.

houndci-bot · 2016-05-12T17:05:34Z

lib/paperclip/io_adapters/uri_adapter.rb

+    def default_filename
+      "index.html"
+    end
+


Trailing whitespace detected.

netmask · 2016-05-13T14:12:06Z

Yes , we can considere it a bug, because the URI specifies the resource location, protocol but not the file name itself(?).

tute · 2016-05-13T16:18:11Z

lib/paperclip/io_adapters/.#data_uri_adapter.rb

@@ -0,0 +1 @@
+[email protected]


It seems like this is a runaway file?

ups yea, its an emacs backup file :P sorry about that

tute · 2016-05-13T16:18:56Z

Sounds good to me. Left a single comment, this looks almost ready. Thank you! 🎉

The uri io adapter now seeks for the content-disposition header if this is pressent the value filename is taken instead of the last path segment for the resource file name. [fixes #2210]

tute · 2016-05-16T14:46:48Z

Merged into master. Thank you! :)

tute · 2016-07-01T10:20:13Z

Merged into master. Thank you! :)

I meant, merged into v4.3. Do you want to port this fix into master, too? Thank you!

netmask · 2016-07-01T18:44:52Z

sure !!

This is generally a better filename than extracting it from URL. See thoughtbot/paperclip#2210

The uri io adapter now seeks for the content-disposition header if this is pressent the value filename is taken instead of the last path segment for the resource file name. [fixes thoughtbot#2210]

The uri io adapter should use the content-disposition filename

a890f98

- The uri io adapter now seeks for the content-disposition header if this is pressent the value filename is taken instead of the last path segment for the resource file name

houndci-bot reviewed May 11, 2016
View reviewed changes

netmask force-pushed the v4.3 branch 2 times, most recently from 7c9d474 to dbc2549 Compare May 11, 2016 16:33

houndci-bot reviewed May 11, 2016
View reviewed changes

Fix style comments

fe1197d

netmask force-pushed the v4.3 branch from dbc2549 to fe1197d Compare May 11, 2016 16:35

tute reviewed May 12, 2016
View reviewed changes

houndci-bot reviewed May 12, 2016
View reviewed changes

URI Adapter refactoring

a13fa20

- Applied the Tute Costa refactor to URI Adapter. - Added entry to the NEWS file.

netmask force-pushed the v4.3 branch from 2d97005 to a13fa20 Compare May 12, 2016 16:57

houndci-bot reviewed May 12, 2016
View reviewed changes

lib/paperclip/io_adapters/uri_adapter.rb

def default_filename

"index.html"

end

Copy link

houndci-bot May 12, 2016

Choose a reason for hiding this comment

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

Trailing whitespace detected.

netmask force-pushed the v4.3 branch from 0e639d1 to 24aa787 Compare May 12, 2016 17:10

tute reviewed May 13, 2016
View reviewed changes

tute mentioned this pull request May 14, 2016

original_filename should follow Content-Disposition #2212

Closed

Fix comment styles

3c5f9db

netmask force-pushed the v4.3 branch from 24aa787 to 3c5f9db Compare May 16, 2016 13:42

- Removed editor tracking file

2bc3fbb

tute closed this May 16, 2016

netmask mentioned this pull request Jul 14, 2016

[backport] The uri io adapter should use the content-disposition filename #2250

Merged

janko added a commit to janko/down that referenced this pull request Jul 26, 2016

Read original filename from Content-Disposition

a625997

This is generally a better filename than extracting it from URL. See thoughtbot/paperclip#2210

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

netmask commented May 11, 2016 •

edited

Loading

houndci-bot May 11, 2016

houndci-bot May 11, 2016

tute May 12, 2016

netmask May 12, 2016

tute commented May 12, 2016

netmask commented May 12, 2016

houndci-bot May 12, 2016

houndci-bot May 12, 2016

tute commented May 12, 2016

tute commented May 12, 2016

houndci-bot May 12, 2016

netmask commented May 13, 2016

tute May 13, 2016

netmask May 16, 2016

tute commented May 13, 2016

tute commented May 16, 2016

tute commented Jul 1, 2016

netmask commented Jul 1, 2016

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

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

Conversation

netmask commented May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented May 12, 2016

netmask commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented May 12, 2016

tute commented May 12, 2016

Choose a reason for hiding this comment

netmask commented May 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tute commented May 13, 2016

tute commented May 16, 2016

tute commented Jul 1, 2016

netmask commented Jul 1, 2016

netmask commented May 11, 2016 •

edited

Loading