Upload - StringIO X FileTemp

I would like to suggest Rails returns only FileTemp, because the
optimization with StringIO may cause to the programmer enter bugs in
your implementation by not knowing what type of return receive.
I know that he can take a test to fix this type of problem:

  If upload_file.instance_of? (Tempfile) # Treat as TempFile else
StringIO

If this test should be the solution rather than modify the rails I
believe this kind of behavior should be documented, otherwise you can
take things like I will guess always receive FileTemp, which is not
true.

I looked at Rails code and it is easy to do this improvement. What do
you think about it?

upload.diff (992 Bytes)

I would like to suggest Rails returns only FileTemp, because the
optimization with StringIO may cause to the programmer enter bugs in
your implementation by not knowing what type of return receive.
I know that he can take a test to fix this type of problem:

  If upload_file.instance_of? (Tempfile) # Treat as TempFile else
StringIO

If this test should be the solution rather than modify the rails I
believe this kind of behavior should be documented, otherwise you can
take things like I will guess always receive FileTemp, which is not
true.

What bugs have you hit due to this optimisation? Assuming that
removing it doesn't impact too poorly on performance, and we can't
make them ducktype nicely, then it seems reasonable to remove.

Hi Koz,

What bugs have you hit due to this optimisation? Assuming that
removing it doesn't impact too poorly on performance, and we can't
make them ducktype nicely, then it seems reasonable to remove.

I had problems on issues of documentation and compatibility with Ruby
versions.

For example:

1. There isn't documentation on Rails to explain to programmer can
receive a StringIO or FileTemp depending on file size.

2. Depending Ruby version that you are using this could make difference
to your implementation have or not BUGS.

In Ruby version 1.8.6 this code working for all cases:

    a = parser(params[:upload_file])
  
But this code is incompatible with Ruby version 1.8.4 that I have to
make a FileTemp.open. in case I have an FileTemp then I tried something
like:

    a = parser(params[:upload_file].open)
  
   This works very well for large files, because the FileTemp has method
FileTemp.open, but will issue to the StringIO because the StringIO.open
is PRIVATE (Though in the documentation Ruby is documented as public).

  Here is the code that works well for large files and is compatible
with Ruby 1.8.4 and 1.8.6. In this case the DuckType could not be used
and it was my second attempt which I generated a BUG Blocker :slight_smile:
  
   This code working for all versions and all sizes file
  
     if params[:upload_file].instance_of?(StringIO)
        a = parser(params[:upload_file])
     else
        a = parser(params[:upload_file].open)
     end

Looking to the code is very simple to implement, but isn't easy to
understand why this was done like that, especially for less experienced
developers.

So I advocate this optimization that the upload could be more simple
(just FileTemp) and compatible with all Ruby versions.

Cheers,

João Lins

Perhaps I'm missing something, but what is wrong with this?

   a = parser(params[:upload_file].read)

That already works in both cases. There's no need to open a Tempfile.

Sorry, you are right. I had not seen that TempFile delegates to File and
File is parent of IO ...
Well, in any case it may be interesting document this kind of thing
anywhere ...
Perhaps I am not the only one to deal with this type of thing, and the
small change in the Rails only avoid this type of misunderstanding.

- João Lins

Sorry, you are right. I had not seen that TempFile delegates to File and
File is parent of IO ...
Well, in any case it may be interesting document this kind of thing
anywhere ...
Perhaps I am not the only one to deal with this type of thing, and the
small change in the Rails only avoid this type of misunderstanding.

Yeah, it would be good to document that behaviour, if someone wants
to whip up a patch for it I'll happily apply it.

There are more issues though. I've documented them here:
http://izumi.plan99.net/blog/index.php/2007/04/07/ruby-on-railss-handling-of-uploaded-files/
To summarize:
1. Some libraries, such as RMagick, also have bugs. RMagick.read (or
something) checks whether its parameter is an IO object, and if not,
it will treat the parameter as a filename. StringIO.is_a?(IO) returns
false, so if I pass a StringIO to RMagick it will think that it's a
filename.
2. If the uploaded file is a Tempfile, then you have to unlink it
manually, or it will stay on the filesystem. StringIO doesn't have an
unlink method so you have to manually check whether you can call
unlink or whether the object is a Tempfile.

Koz,

Hongli,

I think it would be easier if everything was Tempfile.
Even as the optimization made to the upload is for files smaller than
10K, any file larger than it is returned as a Tempfile.
I think that people on the list do not agree with that, then I think it
is better not put the patch on trac, even though it is very simple.
Documenting this behavior is a good start as the KOZ said. I try to find
the best place to do this. Any suggestions?

Cheers,

João