Use StringIO and Tempfile subclasses instead of defining singleton methods on each multipart field.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7759 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

2 files changed, 79 insertions, 59 deletions

diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb
index e9a2e797dd..1fa6accd01 100755
--- a/actionpack/lib/action_controller/request.rb
+++ b/actionpack/lib/action_controller/request.rb
@@ -459,36 +459,16 @@ module ActionController
             when Array
               value.map { |v| get_typed_value(v) }
             else
-              # This is an uploaded file.
-              if value.respond_to?(:original_filename) && !value.original_filename.blank?
-                unless value.respond_to?(:full_original_filename)
-                  class << value
-                    alias_method :full_original_filename, :original_filename
-
-                    # Take the basename of the upload's original filename.
-                    # This handles the full Windows paths given by Internet Explorer
-                    # (and perhaps other broken user agents) without affecting
-                    # those which give the lone filename.
-                    # The Windows regexp is adapted from Perl's File::Basename.
-                    def original_filename
-                      if md = /^(?:.*[:\\\/])?(.*)/m.match(full_original_filename)
-                        md.captures.first
-                      else
-                        File.basename full_original_filename
-                      end
-                    end
-                  end
+              if value.is_a?(UploadedFile)
+                # Uploaded file
+                if value.original_filename
+                  value
+                # Multipart param
+                else
+                  result = value.read
+                  value.rewind
+                  result
                 end
-
-                # Return the same value after overriding original_filename.
-                value
-
-              # Multipart values may have content type, but no filename.
-              elsif value.respond_to?(:read)
-                result = value.read
-                value.rewind
-                result
-
               # Unknown value, neither string nor multipart.
               else
                 raise "Unknown form value: #{value.inspect}"
@@ -524,9 +504,9 @@ module ActionController
             head = nil
             content =
               if 10240 < content_length
-                Tempfile.new("CGI")
+                UploadedTempfile.new("CGI")
               else
-                StringIO.new
+                UploadedStringIO.new
               end
             content.binmode if defined? content.binmode
 
@@ -568,25 +548,21 @@ module ActionController
 
             content.rewind
 
-            /Content-Disposition:.* filename=(?:"((?:\\.|[^\"])*)"|([^;]*))/ni.match(head)
-            filename = ($1 or $2 or "")
-            if /Mac/ni.match(env['HTTP_USER_AGENT']) and
-                /Mozilla/ni.match(env['HTTP_USER_AGENT']) and
-                (not /MSIE/ni.match(env['HTTP_USER_AGENT']))
-              filename = CGI.unescape(filename)
+            head =~ /Content-Disposition:.* filename=(?:"((?:\\.|[^\"])*)"|([^;]*))/ni
+            if filename = $1 || $2
+              if /Mac/ni.match(env['HTTP_USER_AGENT']) and
+                  /Mozilla/ni.match(env['HTTP_USER_AGENT']) and
+                  (not /MSIE/ni.match(env['HTTP_USER_AGENT']))
+                filename = CGI.unescape(filename)
+              end
+              content.original_path = filename.dup
             end
 
-            /Content-Type: ([^\r]*)/ni.match(head)
-            content_type = ($1 or "")
+            head =~ /Content-Type: ([^\r]*)/ni
+            content.content_type = $1.dup if $1
 
-            (class << content; self; end).class_eval do
-              alias local_path path
-              define_method(:original_filename) {filename.dup.taint}
-              define_method(:content_type) {content_type.dup.taint}
-            end
-
-            /Content-Disposition:.* name="?([^\";]*)"?/ni.match(head)
-            name = $1.dup
+            head =~ /Content-Disposition:.* name="?([^\";]*)"?/ni
+            name = $1.dup if $1
 
             if params.has_key?(name)
               params[name].push(content)
@@ -695,4 +671,40 @@ module ActionController
         raise TypeError, "Conflicting types for parameter containers. Expected an instance of #{klass} but found an instance of #{value.class}. This can be caused by colliding Array and Hash parameters like qs[]=value&qs[key]=value."
       end
   end
+
+  module UploadedFile
+    def self.included(base)
+      base.class_eval do
+        attr_accessor :original_path, :content_type
+        alias_method :local_path, :path
+      end
+    end
+
+    # Take the basename of the upload's original filename.
+    # This handles the full Windows paths given by Internet Explorer
+    # (and perhaps other broken user agents) without affecting
+    # those which give the lone filename.
+    # The Windows regexp is adapted from Perl's File::Basename.
+    def original_filename
+      unless defined? @original_filename
+        @original_filename =
+          unless original_path.blank?
+            if original_path =~ /^(?:.*[:\\\/])?(.*)/m
+              $1
+            else
+              File.basename original_path
+            end
+          end
+      end
+      @original_filename
+    end
+  end
+
+  class UploadedStringIO < StringIO
+    include UploadedFile
+  end
+
+  class UploadedTempfile < Tempfile
+    include UploadedFile
+  end
 end
diff --git a/actionpack/test/controller/request_test.rb b/actionpack/test/controller/request_test.rb
index c62b2a6ec7..922699e331 100644
--- a/actionpack/test/controller/request_test.rb
+++ b/actionpack/test/controller/request_test.rb
@@ -527,21 +527,29 @@ class UrlEncodedRequestParameterParsingTest < Test::Unit::TestCase
     assert_equal expected_output, ActionController::AbstractRequest.parse_request_parameters(input)
   end
 
+  UploadedStringIO = ActionController::UploadedStringIO
+  class MockUpload < UploadedStringIO
+    def initialize(content_type, original_path, *args)
+      self.content_type = content_type
+      self.original_path = original_path
+      super *args
+    end
+  end
+
   def test_parse_params_from_multipart_upload
-    mockup = Struct.new(:content_type, :original_filename, :read, :rewind)
-    file = mockup.new('img/jpeg', 'foo.jpg')
-    ie_file = mockup.new('img/jpeg', 'c:\\Documents and Settings\\foo\\Desktop\\bar.jpg')
-    non_file_text_part = mockup.new('text/plain', '', 'abc')
+    file = MockUpload.new('img/jpeg', 'foo.jpg')
+    ie_file = MockUpload.new('img/jpeg', 'c:\\Documents and Settings\\foo\\Desktop\\bar.jpg')
+    non_file_text_part = MockUpload.new('text/plain', '', 'abc')
 
     input = {
-      "something" => [ StringIO.new("") ],
-      "array_of_stringios" => [[ StringIO.new("One"), StringIO.new("Two") ]],
-      "mixed_types_array" => [[ StringIO.new("Three"), "NotStringIO" ]],
-      "mixed_types_as_checkboxes[strings][nested]" => [[ file, "String", StringIO.new("StringIO")]],
-      "ie_mixed_types_as_checkboxes[strings][nested]" => [[ ie_file, "String", StringIO.new("StringIO")]],
-      "products[string]" => [ StringIO.new("Apple Computer") ],
+      "something" => [ UploadedStringIO.new("") ],
+      "array_of_stringios" => [[ UploadedStringIO.new("One"), UploadedStringIO.new("Two") ]],
+      "mixed_types_array" => [[ UploadedStringIO.new("Three"), "NotStringIO" ]],
+      "mixed_types_as_checkboxes[strings][nested]" => [[ file, "String", UploadedStringIO.new("StringIO")]],
+      "ie_mixed_types_as_checkboxes[strings][nested]" => [[ ie_file, "String", UploadedStringIO.new("StringIO")]],
+      "products[string]" => [ UploadedStringIO.new("Apple Computer") ],
       "products[file]" => [ file ],
-      "ie_products[string]" => [ StringIO.new("Microsoft") ],
+      "ie_products[string]" => [ UploadedStringIO.new("Microsoft") ],
       "ie_products[file]" => [ ie_file ],
       "text_part" => [non_file_text_part]
     }
@@ -695,7 +703,7 @@ class MultipartRequestParameterParsingTest < Test::Unit::TestCase
     file = params['file']
     assert_kind_of StringIO, file
     assert_equal 'file.csv', file.original_filename
-    assert_equal '', file.content_type
+    assert_nil file.content_type
     assert_equal 'contents', file.read
 
     file = params['flowers']