From: John Napiorkowski Date: Fri, 18 Oct 2013 15:28:22 +0000 (-0500) Subject: first pass at making Catalyst act enough like PSGI middleware to be broadly compatible X-Git-Tag: 5.90050~1^2~37 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=191665f3c88f4b1f81e4198717077ac08d06bdb7 first pass at making Catalyst act enough like PSGI middleware to be broadly compatible --- diff --git a/Makefile.PL b/Makefile.PL index 79b8380..f634e4b 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -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 diff --git a/lib/Catalyst/Engine.pm b/lib/Catalyst/Engine.pm index fc55db7..9f97a42 100644 --- a/lib/Catalyst/Engine.pm +++ b/lib/Catalyst/Engine.pm @@ -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) diff --git a/lib/Catalyst/Request.pm b/lib/Catalyst/Request.pm index de9f15f..697e1da 100644 --- a/lib/Catalyst/Request.pm +++ b/lib/Catalyst/Request.pm @@ -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 ) = @_; diff --git a/t/aggregate/live_engine_request_uploads.t b/t/aggregate/live_engine_request_uploads.t index 8906e03..5ba8f18 100644 --- a/t/aggregate/live_engine_request_uploads.t +++ b/t/aggregate/live_engine_request_uploads.t @@ -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' ); } }