first pass at making Catalyst act enough like PSGI middleware to be broadly compatible
John Napiorkowski [Fri, 18 Oct 2013 15:28:22 +0000 (10:28 -0500)]
Makefile.PL
lib/Catalyst/Engine.pm
lib/Catalyst/Request.pm
t/aggregate/live_engine_request_uploads.t

index 79b8380..f634e4b 100644 (file)
@@ -70,7 +70,10 @@ requires 'Class::Data::Inheritable';
 requires 'Encode' => '2.49';
 requires 'LWP' => '5.837'; # LWP had unicode fail in 5.8.26
 requires 'URI' => '1.36';
-requires 'JSON::MaybeXS' => '1.000000',
+requires 'JSON::MaybeXS' => '1.000000';
+requires 'Stream::Buffered';
+requires 'Hash::MultiValue';
+requires 'Plack::Request::Upload';
 
 # Install the standalone Regex dispatch modules in order to ease the
 # deprecation transition
index fc55db7..9f97a42 100644 (file)
@@ -13,6 +13,8 @@ use URI::QueryParam;
 use Plack::Loader;
 use Catalyst::EngineLoader;
 use Encode ();
+use Plack::Request::Upload;
+use Hash::MultiValue;
 use utf8;
 
 use namespace::clean -except => 'meta';
@@ -569,6 +571,7 @@ sub prepare_uploads {
 
     my $uploads = $request->_body->upload;
     my $parameters = $request->parameters;
+    my @plack_uploads;
     foreach my $name (keys %$uploads) {
         my $files = $uploads->{$name};
         my @uploads;
@@ -583,9 +586,14 @@ sub prepare_uploads {
                filename => $upload->{filename},
               );
             push @uploads, $u;
+
+            # Plack compatibility.
+            my %copy = (%$upload, headers=>$headers);
+            push @plack_uploads, $name, Plack::Request::Upload->new(%copy);
         }
         $request->uploads->{$name} = @uploads > 1 ? \@uploads : $uploads[0];
 
+
         # support access to the filename as a normal param
         my @filenames = map { $_->{filename} } @uploads;
         # append, if there's already params with this name
@@ -601,6 +609,8 @@ sub prepare_uploads {
             $parameters->{$name} = @filenames > 1 ? \@filenames : $filenames[0];
         }
     }
+
+    $self->env->{'plack.request.upload'} ||= Hash::MultiValue->new(@plack_uploads);
 }
 
 =head2 $self->write($c, $buffer)
index de9f15f..697e1da 100644 (file)
@@ -7,6 +7,9 @@ use URI::http;
 use URI::https;
 use URI::QueryParam;
 use HTTP::Headers;
+use Stream::Buffered;
+use Hash::MultiValue;
+use Scalar::Util;
 
 use Moose;
 
@@ -224,98 +227,81 @@ has _uploadtmp => (
 sub prepare_body {
     my ( $self ) = @_;
 
-    #warn "XXX ${\$self->_uploadtmp}" if $self->_has_uploadtmp;
+    # If previously applied middleware created the HTTP::Body object, then we
+    # just use that one.  
 
     if(my $plack_body = $self->env->{'plack.request.http.body'}) {
-      warn "wtF" x 100;
         $self->_body($plack_body);
-        $self->_body->cleanup(1); # Make extra sure!
-        $self->_body->tmpdir( $self->_uploadtmp )
-          if $self->_has_uploadtmp;
-    } else {
-
+        $self->_body->cleanup(1);
+        return;
     }
 
-    if ( my $length = $self->_read_length ) {
-        unless ( $self->_body ) {
-            my $type = $self->header('Content-Type');
-            $self->_body(HTTP::Body->new( $type, $length ));
-            $self->_body->cleanup(1); # Make extra sure!
-            $self->_body->tmpdir( $self->_uploadtmp )
-              if $self->_has_uploadtmp;
-        }
+    # Define PSGI ENV placeholders, or for empty should there be no content
+    # body (typical in HEAD or GET).  Looks like from Plack::Request that
+    # middleware would probably expect to see this, even if empty
 
-        # Check for definedness as you could read '0'
-        while ( defined ( my $buffer = $self->read() ) ) {
-            $self->prepare_body_chunk($buffer);
-        }
+    $self->env->{'plack.request.body'}   = Hash::MultiValue->new;
+    $self->env->{'plack.request.upload'} = Hash::MultiValue->new;
 
-        # paranoia against wrong Content-Length header
-        my $remaining = $length - $self->_read_position;
-        if ( $remaining > 0 ) {
-            Catalyst::Exception->throw(
-                "Wrong Content-Length value: $length" );
-        }
-    }
-    else {
-        # Defined but will cause all body code to be skipped
-        $self->_body(0);
-    }
-}
-
-sub prepare_bodyXXX {
-    my ( $self ) = @_;
-    if(my $plack_body = $self->env->{'plack.request.http.body'}) {
-    
+    # If there is nothing to read, set body to naught and return.  This
+    # will cause all body code to be skipped
 
-    } else {
+    return $self->_body(0) unless my $length = $self->_read_length;
 
-    }
+    # Unless the body has already been set, create it.  Not sure about this
+    # code, how else might it be set, but this was existing logic.
 
-    die "XXX ${\$self->_uploadtmp}" x1000; $self->_has_uploadtmp;
+    unless ($self->_body) {
+        my $type = $self->header('Content-Type');
+        $self->_body(HTTP::Body->new( $type, $length ));
+        $self->_body->cleanup(1);
 
-    if ( my $length = $self->_read_length ) {
-        unless ( $self->_body ) {
-            
-            ## If something plack middle already ready the body, just use
-            ## that.
+        # JNAP: I'm not sure this is doing what we expect, but it also doesn't
+        # seem to be hurting (seems ->_has_uploadtmp is true more than I would
+        # expect.
 
-            my $body;
-            if(my $plack_body = $self->env->{'plack.request.http.body'}) {
-                $body = $plack_body;
-            } else {
-                my $type = $self->header('Content-Type');
-                $body = HTTP::Body->new($type, $length);
+        $self->_body->tmpdir( $self->_uploadtmp )
+          if $self->_has_uploadtmp;
+    }
 
-                ## Play nice with Plak Middleware that looks for a body
-                $self->env->{'plack.request.http.body'} = $body;
-                $self->_body($body);
+    # Ok if we get this far, we have to read psgi.input into the new body
+    # object.  Lets play nice with any plack app or other downstream, so
+    # we create a buffer unless one exists.
+     
+    my $stream_buffer;
+    if ($self->env->{'psgix.input.buffered'}) {
+        # Be paranoid about previous psgi middleware or apps that read the
+        # input but didn't return the buffer to the start.
+        $self->env->{'psgi.input'}->seek(0, 0);
+    } else {
+        $stream_buffer = Stream::Buffered->new($length);
+    }
 
-                $body->cleanup(1); # Make extra sure!
-                $body->tmpdir( $self->_uploadtmp )
-                  if $self->_has_uploadtmp;
-            }
-        }
+    # Check for definedness as you could read '0'
+    while ( defined ( my $chunk = $self->read() ) ) {
+        $self->prepare_body_chunk($chunk);
+        $stream_buffer->print($chunk) if $stream_buffer;
+    }
 
-        # Check for definedness as you could read '0'
-        while ( defined ( my $buffer = $self->read() ) ) {
-            $self->prepare_body_chunk($buffer);
-        }
+    # Ok, we read the body.  Lets play nice for any PSGI app down the pipe
 
-        # paranoia against wrong Content-Length header
-        my $remaining = $length - $self->_read_position;
-        if ( $remaining > 0 ) {
-            Catalyst::Exception->throw(
-                "Wrong Content-Length value: $length" );
-        }
+    if ($stream_buffer) {
+        $self->env->{'psgix.input.buffered'} = 1;
+        $self->env->{'psgi.input'} = $stream_buffer->rewind;
+    } else {
+        $self->env->{'psgi.input'}->seek(0, 0); # Reset the buffer for downstream middleware or apps
     }
-    else {
-        # Defined but will cause all body code to be skipped
-        $self->_body(0);
+
+    $self->env->{'plack.request.http.body'} = $self->_body;
+    $self->env->{'plack.request.body'} = Hash::MultiValue->from_mixed($self->_body->param);
+
+    # paranoia against wrong Content-Length header
+    my $remaining = $length - $self->_read_position;
+    if ( $remaining > 0 ) {
+        Catalyst::Exception->throw("Wrong Content-Length value: $length" );
     }
 }
 
-
 sub prepare_body_chunk {
     my ( $self, $chunk ) = @_;
 
index 8906e03..5ba8f18 100644 (file)
@@ -332,6 +332,14 @@ use Path::Class::Dir;
         if ( $ENV{CATALYST_SERVER} ) {
             skip 'Not testing for deleted file on remote server', 1;
         }
+
+        # JNAP, I added the following line in order to properly let
+        # the $env go out of scope so that the associated tempfile
+        # would be deleted.  I think somewhere Catalyst::Test closed
+        # over ENV and holds state until a new command is issues but 
+        # I can't find it.
+
+        request GET  'http://localhost/';
         ok( !-e $body->body->filename, 'Upload temp file was deleted' );
     }
 }