Make FileFilter check more generic so ActionDispatch::Http::UploadedFile is accepted #236

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@conf
Contributor

conf commented Dec 3, 2014

Hello!

I stumbled with this bug when I tried to use active_interaction and CarrierWave together. The bug consists in the following:
Since actionpack's ActionDispatch::Http::UploadedFile responds to tempfle, CarrierWave (inside ActiveRecord model) receives Tempfile instance and not its wrapper ActionDispatch::Http::UploadedFile, losing this way access to extra variables like original_file_name, content_type and etc. This breaks some Carrierwave validations and ends up with saving file with a generic temporary name like rack20141203-27678-2wbop without extension which is not good.
So I made this check more generic according the fact that all IO-like classes respond to close method.
I couldn't write more explicit specs to test this behavior without adding CarrierWave or actionpack as a dependency. If you're not satisfied with changes in this pull request, I'll be happy to work on it together.
Cheers!

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 3, 2014

Collaborator

We considered supporting any IO before. Check out issue #67. This might be a good motivation to do that.

I have not heard of CarrierWave before. Since it's a gem that we don't claim to support, I would argue that this isn't a bug per se.

Collaborator

tfausak commented Dec 3, 2014

We considered supporting any IO before. Check out issue #67. This might be a good motivation to do that.

I have not heard of CarrierWave before. Since it's a gem that we don't claim to support, I would argue that this isn't a bug per se.

@tfausak tfausak added the enhancement label Dec 3, 2014

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 3, 2014

Contributor

Well, it's quite popular: it seconds Paperclip, and, IMO much flexible than it.
Generally speaking, Paperclip will be broken too, because underlying mechanism works similarly so it won't be able to save uploaded file with original filename and will resort to Tempfile's name instead.
So, what do you propose? Doesn't this PR solve #67, does it?
Since every IO object responds to close, it would be treated as a file object. I would argue against explicit IO class check since it will break wrappers like ActionDispatch::Http::UploadedFile.
P.S. Don't know how to fix the build, it failed very strangely. Any ideas?

Contributor

conf commented Dec 3, 2014

Well, it's quite popular: it seconds Paperclip, and, IMO much flexible than it.
Generally speaking, Paperclip will be broken too, because underlying mechanism works similarly so it won't be able to save uploaded file with original filename and will resort to Tempfile's name instead.
So, what do you propose? Doesn't this PR solve #67, does it?
Since every IO object responds to close, it would be treated as a file object. I would argue against explicit IO class check since it will break wrappers like ActionDispatch::Http::UploadedFile.
P.S. Don't know how to fix the build, it failed very strangely. Any ideas?

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 3, 2014

Collaborator

Yes, this would fix #67.

I would prefer to check for #to_io instead of #close.

Collaborator

tfausak commented Dec 3, 2014

Yes, this would fix #67.

I would prefer to check for #to_io instead of #close.

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 3, 2014

Contributor

Well, ActionDispatch::Http::UploadedFile#to_io is only implemented in edge version of rails (4.2.0beta, still unreleased) and in all released versions of rails ActionDispatch::Http::UploadedFile will fail this check.

Contributor

conf commented Dec 3, 2014

Well, ActionDispatch::Http::UploadedFile#to_io is only implemented in edge version of rails (4.2.0beta, still unreleased) and in all released versions of rails ActionDispatch::Http::UploadedFile will fail this check.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 4, 2014

Collaborator

Oh, I didn't realize that method was so new. I figured that it's been around since Ruby 1.8.6 at least, so any IO-ish object would probably implement it. Looks like I was wrong.

This pull request is effectively making the file filter a shortcut for the interface filter with some methods preset. It might be better to implement it that way.

Collaborator

tfausak commented Dec 4, 2014

Oh, I didn't realize that method was so new. I figured that it's been around since Ruby 1.8.6 at least, so any IO-ish object would probably implement it. Looks like I was wrong.

This pull request is effectively making the file filter a shortcut for the interface filter with some methods preset. It might be better to implement it that way.

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 9, 2014

Contributor

Hi, reimplemented FileFilter via InterfaceFilter as you suggested. Any thoughts?

Contributor

conf commented Dec 9, 2014

Hi, reimplemented FileFilter via InterfaceFilter as you suggested. Any thoughts?

- super
- end
+ def methods
+ super + [:close]

This comment has been minimized.

@tfausak

tfausak Dec 9, 2014

Collaborator

I think this should not call super. If it does, you could do this:

file :whatever,
  methods: [:foobar]
@tfausak

tfausak Dec 9, 2014

Collaborator

I think this should not call super. If it does, you could do this:

file :whatever,
  methods: [:foobar]

This comment has been minimized.

@conf

conf Dec 9, 2014

Contributor

Agreed.

@conf

conf Dec 9, 2014

Contributor

Agreed.

@@ -32,8 +32,6 @@ def cast(value)
# @return [Boolean]
def matches?(object)
methods.all? { |method| object.respond_to?(method) }
- rescue NoMethodError
- false

This comment has been minimized.

@tfausak

tfausak Dec 9, 2014

Collaborator

Why did you remove this?

@tfausak

tfausak Dec 9, 2014

Collaborator

Why did you remove this?

This comment has been minimized.

@conf

conf Dec 9, 2014

Contributor

Because there's no exception can be thrown here. respond_to? is boolean method and methods always returns an array so this rescue clause is extra.

@conf

conf Dec 9, 2014

Contributor

Because there's no exception can be thrown here. respond_to? is boolean method and methods always returns an array so this rescue clause is extra.

This comment has been minimized.

@tfausak

tfausak Dec 9, 2014

Collaborator

Only subclasses of Object respond to #respond_to?. Case in point:

BasicObject.new.respond_to? :something
# NoMethodError: undefined method `respond_to?' for #<BasicObject:0x...>
@tfausak

tfausak Dec 9, 2014

Collaborator

Only subclasses of Object respond to #respond_to?. Case in point:

BasicObject.new.respond_to? :something
# NoMethodError: undefined method `respond_to?' for #<BasicObject:0x...>

This comment has been minimized.

@conf

conf Dec 9, 2014

Contributor

You're right. And still this is a very rare case. Remember, we're talking about user-supplied data here (correct me, if it's not the case) and I can't imagine such a scenario when user will send me data that will be converted to a BasicObject.

@conf

conf Dec 9, 2014

Contributor

You're right. And still this is a very rare case. Remember, we're talking about user-supplied data here (correct me, if it's not the case) and I can't imagine such a scenario when user will send me data that will be converted to a BasicObject.

This comment has been minimized.

@AaronLasseigne

AaronLasseigne Dec 9, 2014

Owner

It's not always user supplied. We've successfully used interactions to share code between our main application controllers and our API controllers or import tasks. We've passed in value objects that aren't directly things in params.

@AaronLasseigne

AaronLasseigne Dec 9, 2014

Owner

It's not always user supplied. We've successfully used interactions to share code between our main application controllers and our API controllers or import tasks. We've passed in value objects that aren't directly things in params.

This comment has been minimized.

@conf

conf Dec 9, 2014

Contributor

All right. Do you want me to put it back?

@conf

conf Dec 9, 2014

Contributor

All right. Do you want me to put it back?

This comment has been minimized.

@tfausak

tfausak Dec 9, 2014

Collaborator

Yes please. I'm working on a test that would notice this regression.

@tfausak

tfausak Dec 9, 2014

Collaborator

Yes please. I'm working on a test that would notice this regression.

This comment has been minimized.

@conf

conf Dec 9, 2014

Contributor

Done.

@conf

conf Dec 9, 2014

Contributor

Done.

- else
- super
- end
+ def methods

This comment has been minimized.

@tfausak

tfausak Dec 9, 2014

Collaborator

This method should be private.

@tfausak

tfausak Dec 9, 2014

Collaborator

This method should be private.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 9, 2014

Collaborator

Thank you! I think it looks a lot better. We should see what @AaronLasseigne thinks.

Collaborator

tfausak commented Dec 9, 2014

Thank you! I think it looks a lot better. We should see what @AaronLasseigne thinks.

@AaronLasseigne

This comment has been minimized.

Show comment
Hide comment
@AaronLasseigne

AaronLasseigne Dec 9, 2014

Owner

I'm a little concerned about using close as the method to check. It feels too wide open to me. I could see non-file objects implementing that method for other purposes. If we're going to use a method to check I think eof? might be better. It's more file specific and less likely to be implemented elsewhere. Additionally, we could specifically check for File, Tempfile, or ActionDispatch::Http::UploadedFile. Thoughts?

Also, thanks @conf for the PR!

Owner

AaronLasseigne commented Dec 9, 2014

I'm a little concerned about using close as the method to check. It feels too wide open to me. I could see non-file objects implementing that method for other purposes. If we're going to use a method to check I think eof? might be better. It's more file specific and less likely to be implemented elsewhere. Additionally, we could specifically check for File, Tempfile, or ActionDispatch::Http::UploadedFile. Thoughts?

Also, thanks @conf for the PR!

@AaronLasseigne

This comment has been minimized.

Show comment
Hide comment
@AaronLasseigne

AaronLasseigne Dec 9, 2014

Owner

I should also note that we originally pulled the tempfile so that you were guaranteed an object that responded to all the File methods. Accepting ActionDispatch::Http::UploadedFile would make the thing you get from a file filter less consistent (i.e. polymorphic). That might be a trade worth making though for the purposes of practicality.

Owner

AaronLasseigne commented Dec 9, 2014

I should also note that we originally pulled the tempfile so that you were guaranteed an object that responded to all the File methods. Accepting ActionDispatch::Http::UploadedFile would make the thing you get from a file filter less consistent (i.e. polymorphic). That might be a trade worth making though for the purposes of practicality.

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 9, 2014

Contributor

@AaronLasseigne Ok, changed check to .eof?.
I don't think we should concern a lot about strict guarantees of File objects. The main purpose of active_interaction's filters is validation of user-supplied data and in this context clear separation of strings, nils and IO-like objects is enough. Actually, this pull-request has been born because of too strict check than was needed.
Thank you for your time and for great gem!

Contributor

conf commented Dec 9, 2014

@AaronLasseigne Ok, changed check to .eof?.
I don't think we should concern a lot about strict guarantees of File objects. The main purpose of active_interaction's filters is validation of user-supplied data and in this context clear separation of strings, nils and IO-like objects is enough. Actually, this pull-request has been born because of too strict check than was needed.
Thank you for your time and for great gem!

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 9, 2014

Collaborator

👍

This is a breaking change to that API we provide for file inputs, so it'll have to be part of v2.0.0.

Collaborator

tfausak commented Dec 9, 2014

👍

This is a breaking change to that API we provide for file inputs, so it'll have to be part of v2.0.0.

@tfausak tfausak added this to the v2.0.0 milestone Dec 9, 2014

@tfausak tfausak self-assigned this Dec 9, 2014

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 10, 2014

Contributor

So, when this is going to be merged? Should I rebase it against some other branch or smth?

Contributor

conf commented Dec 10, 2014

So, when this is going to be merged? Should I rebase it against some other branch or smth?

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Dec 10, 2014

Collaborator

Yes, could you please open a new pull request against the v2.0.0 branch?

Collaborator

tfausak commented Dec 10, 2014

Yes, could you please open a new pull request against the v2.0.0 branch?

@conf

This comment has been minimized.

Show comment
Hide comment
@conf

conf Dec 10, 2014

Contributor

Sure: #243

Contributor

conf commented Dec 10, 2014

Sure: #243

@conf conf closed this Dec 10, 2014

@conf conf deleted the conf:fix-carrierwave-integration branch Dec 10, 2014

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