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

[conf]

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]

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.

[conf]

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]

Yes, this would fix #67.

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

[conf]

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]

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]

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

[tfausak]

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

file :whatever,
  methods: [:foobar]

[conf]

Agreed.

[tfausak]

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

[AaronLasseigne]

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]

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]

@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 is a breaking change to that API we provide for file inputs, so it'll have to be part of v2.0.0.

[conf]

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

[tfausak]

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

[conf]

Sure: #243