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? --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Nov 13, 2007 9:11 AM, João Paulo Lins <jpaulolins@gmail.com> wrote:> 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.> > I looked at Rails code and it is easy to do this improvement. What do > you think about it? > > > >-- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Hi Koz, On Wed, 2007-11-14 at 18:43 +1300, Michael Koziarski wrote:> 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 :-) 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Nov 14, 2007, at 9:29 AM, João Paulo Lins wrote:> 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) > endPerhaps 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. -- Jonathan Yurek thoughtbot, inc. jyurek@thoughtbot.com 617 482 1300 x114 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Wed, 2007-11-14 at 10:28 -0500, Jon Yurek wrote:> On Nov 14, 2007, at 9:29 AM, João Paulo Lins wrote: > > 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 > > 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> 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. -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
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. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Koz, On Thu, 2007-11-15 at 18:09 +1300, Michael Koziarski wrote:> > 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. >I can make it. I don''t know the best place to document this in the RAILS API. Some Suggestion? in ActionView methods file_field_tag and file_field? []''s João --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
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 On Thu, 2007-11-15 at 03:45 -0800, Hongli Lai wrote:> 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. > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---