New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add implicit to path conversion to uploaded file #28676

Merged
merged 2 commits into from Jul 22, 2018

Conversation

Projects
None yet
6 participants
@cupakromer
Contributor

cupakromer commented Apr 5, 2017

History

Ruby has a few implicit conversion protocols (e.g. to_hash, to_str, to_path, etc.). These are considered implicit conversion protocols because in certain instances Ruby (MRI core objects) will check if an argument responds to the appropriate protocol and automatically convert it when it does; this is why you can provide a Pathname instance into File.read without having to explicitly call to_s.

a_file_path = 'some/path/file.ext'
File.write a_file_path, 'String Path Content'
File.read a_file_path
# => "String Path Content"

a_pathname = Pathname(a_file_path)
File.write a_pathname, 'Pathname Content'
File.read a_pathname
# => "Pathname Content"

core_file = File.new(a_pathname)
File.write core_file, 'File Content'
File.read core_file
# => "File Content"

tmp_file = Tempfile.new('example')
File.write tmp_file, 'Tempfile Content'
File.read tmp_file
# => "Tempfile Content"

So how does an uploaded file work in such cases?

tmp_file = Tempfile.new('example')
File.write tmp_file, 'Uploaded Content'
uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: tmp_file)
File.read uploaded_file
# => TypeError: no implicit conversion of ActionDispatch::Http::UploadedFile into String

It fails with a TypeError:

no implicit conversion of ActionDispatch::Http::UploadedFile into String

In order to make an uploaded file work it must be explicitly converted to a file path using path.

File.read uploaded_file.path

This requires any code that expects path/file like objects to either special case an uploaded file, re-implement the path conversion protocol to use path, or forces the developer to explicitly cast uploaded files to paths. This last option can sometimes be difficult to do when such calls are deep within the inner workings of libraries.

Proposed Changes

Since an uploaded file already has a path it makes sense to implement the implicit "path" conversion protocol (just like File and Tempfile). This change allows uploaded file content to be treated more closely to regular file content, without requiring any special case handling or explicit conversion for common file utilities.

For the example above would now read the file content:

tmp_file = Tempfile.new('example')
File.write tmp_file, 'Uploaded Content'
uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: tmp_file)
File.read uploaded_file
# => "Uploaded Content"
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Apr 5, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Apr 5, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Apr 5, 2017

Contributor

I was also wondering if maybe this should also add the to_io protocol (which already is part of Tempfile)? Or even if this should simply subclass DelegateClass(Tempfile) to provide the full range of temp file behavior.

Contributor

cupakromer commented Apr 5, 2017

I was also wondering if maybe this should also add the to_io protocol (which already is part of Tempfile)? Or even if this should simply subclass DelegateClass(Tempfile) to provide the full range of temp file behavior.

@maclover7 maclover7 added the actionpack label Apr 8, 2017

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Apr 2, 2018

Contributor

Is this something that could be included in Rails 5.2?

Contributor

cupakromer commented Apr 2, 2018

Is this something that could be included in Rails 5.2?

@gmcgibbon

Still reproducible in 6.0.0.alpha. While its worth noting that File, Pathname, Tempfile, and ActionDispatch::Http::UploadedFile all have a #read method, I agree that File.read should also work with ActionDispatch::Http::UploadedFile objects to stay consistent.

# Shortcut for +tempfile.to_path+.
def to_path
@tempfile.to_path
end

This comment has been minimized.

@gmcgibbon

gmcgibbon Jul 2, 2018

Member

You should add a simple delegation test for #to_path to actionpack/test/dispatch/uploaded_file_test.rb in order for this to be mergeable.

@gmcgibbon

gmcgibbon Jul 2, 2018

Member

You should add a simple delegation test for #to_path to actionpack/test/dispatch/uploaded_file_test.rb in order for this to be mergeable.

This comment has been minimized.

@cupakromer

cupakromer Jul 3, 2018

Contributor

👍Thanks! I'll get to it a little later today 😄

@cupakromer

cupakromer Jul 3, 2018

Contributor

👍Thanks! I'll get to it a little later today 😄

cupakromer added some commits Apr 5, 2017

Add implicit to path conversion to uploaded file
Ruby has a few implicit conversion protocols (e.g. `to_hash`, `to_str`,
`to_path`, etc.). These are considered implicit conversion protocols
because in certain instances Ruby (MRI core objects) will check if an
argument responds to the appropriate protocol and automatically convert
it when it does; this is why you can provide a `Pathname` instance into
`File.read` without having to explicitly call `to_s`.

```ruby
a_file_path = 'some/path/file.ext'
File.write a_file_path, 'String Path Content'
File.read a_file_path

a_pathname = Pathname(a_file_path)
File.write core_file, 'Pathname Content'
File.read a_file_path

core_file = File.new(a_pathname)
File.write core_file, 'File Content'
File.read core_file

tmp_file = Tempfile.new('example')
File.write tmp_file, 'Tempfile Content'
File.read tmp_file
```

So how does an uploaded file work in such cases?

```ruby
tmp_file = Tempfile.new('example')
File.write tmp_file, 'Uploaded Content'
uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: tmp_file)
File.read uploaded_file
```

It fails with a `TypeError`:

    no implicit conversion of ActionDispatch::Http::UploadedFile into String

In order to make an uploaded file work it must be explicitly converted
to a file path using `path`.

```ruby
File.read uploaded_file.path
```

This requires any code that expects path/file like objects to either
special case an uploaded file, re-implement the path conversion protocol
to use `path`, or forces the developer to explicitly cast uploaded files
to paths. This last option can sometimes be difficult to do when such
calls are deep within the inner workings of libraries.

Since an uploaded file already has a path it makes sense to implement
the implicit "path" conversion protocol (just like `File` and
`Tempfile`). This change allows uploaded file content to be treated more
closely to regular file content, without requiring any special case
handling or explicit conversion for common file utilities.

@kaspth kaspth merged commit 20543c0 into rails:master Jul 22, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 22, 2018

Member

Thanks!

Member

kaspth commented Jul 22, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment