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