From 73830ebe387cf3dd8fec1f046d57cdcb36b8e479 Mon Sep 17 00:00:00 2001 From: John Napiorkowski Date: Wed, 25 Dec 2013 08:40:23 -0600 Subject: [PATCH 1/4] let an http-exception object that ->can("as_psgi") recieve the psgi $env as its first argument --- lib/Plack/Middleware/HTTPExceptions.pm | 5 ++- t/Plack-Middleware/httpexceptions-as_psgi.t | 41 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 t/Plack-Middleware/httpexceptions-as_psgi.t diff --git a/lib/Plack/Middleware/HTTPExceptions.pm b/lib/Plack/Middleware/HTTPExceptions.pm index 23a7e935f..394664256 100644 --- a/lib/Plack/Middleware/HTTPExceptions.pm +++ b/lib/Plack/Middleware/HTTPExceptions.pm @@ -47,7 +47,7 @@ sub transform_error { my($code, $message); if (blessed $e && $e->can('as_psgi')) { - return $e->as_psgi; + return $e->as_psgi($env); } if (blessed $e && $e->can('code')) { $code = $e->code; @@ -123,7 +123,8 @@ the status message of the error code, such as I for C<503>. Finally, the exception object may implement C, and the result -of this will be returned directly as the PSGI response. +of this will be returned directly as the PSGI response. When calling +the C method, the PSGI C<$env> will be passed. If the code is in the 3xx range and the exception implements the 'location' method (HTTP::Exception::3xx does), the Location header will be set in the diff --git a/t/Plack-Middleware/httpexceptions-as_psgi.t b/t/Plack-Middleware/httpexceptions-as_psgi.t new file mode 100644 index 000000000..3e01fb89d --- /dev/null +++ b/t/Plack-Middleware/httpexceptions-as_psgi.t @@ -0,0 +1,41 @@ +use strict; +use warnings; +use Plack::Test; +use HTTP::Request::Common; +use Plack::Middleware::HTTPExceptions; +use Test::More; + +{ + package MyApp::Exception::Tuple; + + sub new { + my ($class, @args) = @_; + return bless +{res => \@args}, $class; + } + + sub as_psgi { + my ($self, $env) = @_; + Test::More::ok $env && $env->{'psgi.version'}, + 'has $env and its a psgi $env'; + return $self->{res}; + } +} + +ok my $psgi_app = sub { + my $env = shift; + die MyApp::Exception::Tuple->new( + 404, ['content-type'=>'text/plain'], ['Not Found']); +}; + +ok $psgi_app = Plack::Middleware::HTTPExceptions->wrap($psgi_app); + +test_psgi $psgi_app, sub { + my $cb = shift; + my $res = $cb->(GET "/"); + is $res->code, 404; + is $res->content, 'Not Found'; +}; + +# need to list the expected test number because of the test case in the +# exception class. +done_testing(5); From b0b89a88ef739e82bef8036786f4673da05e69b3 Mon Sep 17 00:00:00 2001 From: John Napiorkowski Date: Thu, 26 Dec 2013 10:10:34 -0600 Subject: [PATCH 2/4] test case for as_psgig --- t/Plack-Middleware/httpexceptions-as_psgi.t | 42 +++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/t/Plack-Middleware/httpexceptions-as_psgi.t b/t/Plack-Middleware/httpexceptions-as_psgi.t index 3e01fb89d..14660e5e8 100644 --- a/t/Plack-Middleware/httpexceptions-as_psgi.t +++ b/t/Plack-Middleware/httpexceptions-as_psgi.t @@ -19,23 +19,59 @@ use Test::More; 'has $env and its a psgi $env'; return $self->{res}; } + + package MyApp::Exception::CodeRef; + + sub new { + my ($class, @args) = @_; + return bless +{res => \@args}, $class; + } + + sub as_psgi { + my ($self, $env) = @_; + Test::More::ok $env && $env->{'psgi.version'}, + 'has $env and its a psgi $env'; + + my ($code, $headers, $body) = @{$self->{res}}; + + return sub { + my $responder = shift; + $responder->([$code, $headers, $body]); + }; + } + } ok my $psgi_app = sub { my $env = shift; + die MyApp::Exception::Tuple->new( - 404, ['content-type'=>'text/plain'], ['Not Found']); + 404, ['content-type'=>'text/plain'], ['Not Found']) + if $env->{PATH_INFO} eq '/tuple'; + + die MyApp::Exception::CodeRef->new( + 404, ['content-type'=>'text/plain'], ['Not Found']) + if $env->{PATH_INFO} eq '/coderef'; + }; ok $psgi_app = Plack::Middleware::HTTPExceptions->wrap($psgi_app); test_psgi $psgi_app, sub { my $cb = shift; - my $res = $cb->(GET "/"); + my $res = $cb->(GET "/tuple"); + is $res->code, 404; + is $res->content, 'Not Found'; +}; + +test_psgi $psgi_app, sub { + my $cb = shift; + my $res = $cb->(GET "/coderef"); is $res->code, 404; is $res->content, 'Not Found'; }; + # need to list the expected test number because of the test case in the # exception class. -done_testing(5); +done_testing(8); From 00e2a939d736f5cea5ab9708eedf09ab8f637d44 Mon Sep 17 00:00:00 2001 From: John Napiorkowski Date: Thu, 26 Dec 2013 12:01:39 -0600 Subject: [PATCH 3/4] handle the pathological case where a http exception responds with a other and so on --- lib/Plack/Middleware/HTTPExceptions.pm | 18 ++-- t/Plack-Middleware/httpexceptions-as_psgi.t | 102 +++++++++++++++++++- 2 files changed, 110 insertions(+), 10 deletions(-) diff --git a/lib/Plack/Middleware/HTTPExceptions.pm b/lib/Plack/Middleware/HTTPExceptions.pm index 394664256..03430c76f 100644 --- a/lib/Plack/Middleware/HTTPExceptions.pm +++ b/lib/Plack/Middleware/HTTPExceptions.pm @@ -24,22 +24,28 @@ sub call { return $res if ref $res eq 'ARRAY'; - return sub { - my $respond = shift; - + my $unroll_coderef_responses; + $unroll_coderef_responses = sub { + my ($responder, $response) = @_; my $writer; try { - $res->(sub { return $writer = $respond->(@_) }); + $response->(sub { return $writer = $responder->(@_) }); } catch { if ($writer) { Carp::cluck $_; $writer->close; } else { - my $res = $self->transform_error($_, $env); - $respond->($res); + my $error_psgi_response = $self->transform_error($_, $env); + return $responder->($error_psgi_response) if ref $error_psgi_response eq 'ARRAY'; + return $unroll_coderef_responses->($responder, $error_psgi_response); } }; }; + + return sub { + my $responder = shift; + return $unroll_coderef_responses->($responder, $res); + }; } sub transform_error { diff --git a/t/Plack-Middleware/httpexceptions-as_psgi.t b/t/Plack-Middleware/httpexceptions-as_psgi.t index 14660e5e8..bddcc2745 100644 --- a/t/Plack-Middleware/httpexceptions-as_psgi.t +++ b/t/Plack-Middleware/httpexceptions-as_psgi.t @@ -40,6 +40,28 @@ use Test::More; }; } + package MyApp::Exception::CodeRefWithWrite; + + sub new { + my ($class, @args) = @_; + return bless +{res => \@args}, $class; + } + + sub as_psgi { + my ($self, $env) = @_; + Test::More::ok $env && $env->{'psgi.version'}, + 'has $env and its a psgi $env'; + + my ($code, $headers, $body) = @{$self->{res}}; + + return sub { + my $responder = shift; + my $writer = $responder->([$code, $headers]); + $writer->write($_) for @$body; + $writer->close; + }; + } + } ok my $psgi_app = sub { @@ -50,9 +72,15 @@ ok my $psgi_app = sub { if $env->{PATH_INFO} eq '/tuple'; die MyApp::Exception::CodeRef->new( - 404, ['content-type'=>'text/plain'], ['Not Found']) + 303, ['content-type'=>'text/plain'], ['See Other']) if $env->{PATH_INFO} eq '/coderef'; + die MyApp::Exception::CodeRefWithWrite->new( + 400, ['content-type'=>'text/plain'], ['Bad Request']) + if $env->{PATH_INFO} eq '/coderefwithwrite'; + + return [200, ['Content-Type'=>'html/plain'], ['ok']] + if $env->{PATH_INFO} eq '/ok'; }; ok $psgi_app = Plack::Middleware::HTTPExceptions->wrap($psgi_app); @@ -61,17 +89,83 @@ test_psgi $psgi_app, sub { my $cb = shift; my $res = $cb->(GET "/tuple"); is $res->code, 404; - is $res->content, 'Not Found'; + is $res->content, 'Not Found', 'NOT FOUND'; +}; + +test_psgi $psgi_app, sub { + my $cb = shift; + my $res = $cb->(GET "/ok"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; }; test_psgi $psgi_app, sub { my $cb = shift; my $res = $cb->(GET "/coderef"); + is $res->code, 303; + is $res->content, 'See Other', 'SEE OTHER'; +}; + +test_psgi $psgi_app, sub { + my $cb = shift; + my $res = $cb->(GET "/coderefwithwrite"); + is $res->code, 400; + is $res->content, 'Bad Request', 'BAD REQUEST'; +}; + +ok my $psgi_app_delayed = sub { + my $env = shift; + return sub { + my $responder = shift; + + die MyApp::Exception::Tuple->new( + 404, ['content-type'=>'text/plain'], ['Not Found']) + if $env->{PATH_INFO} eq '/tuple'; + + die MyApp::Exception::CodeRef->new( + 303, ['content-type'=>'text/plain'], ['See Other']) + if $env->{PATH_INFO} eq '/coderef'; + + die MyApp::Exception::CodeRefWithWrite->new( + 400, ['content-type'=>'text/plain'], ['Bad Request']) + if $env->{PATH_INFO} eq '/coderefwithwrite'; + + return $responder-> + ([200, ['Content-Type'=>'html/plain'], ['ok']]) + if $env->{PATH_INFO} eq '/ok'; + }; +}; + +ok $psgi_app_delayed = Plack::Middleware::HTTPExceptions->wrap($psgi_app_delayed); + +test_psgi $psgi_app_delayed, sub { + my $cb = shift; + my $res = $cb->(GET "/tuple"); is $res->code, 404; - is $res->content, 'Not Found'; + is $res->content, 'Not Found', 'NOT FOUND'; +}; + +test_psgi $psgi_app_delayed, sub { + my $cb = shift; + my $res = $cb->(GET "/ok"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; }; +test_psgi $psgi_app_delayed, sub { + my $cb = shift; + my $res = $cb->(GET "/coderef"); + is $res->code, 303; + is $res->content, 'See Other', 'SEE OTHER'; +}; + +test_psgi $psgi_app_delayed, sub { + my $cb = shift; + my $res = $cb->(GET "/coderefwithwrite"); + is $res->code, 400, 'correct 400 code'; + is $res->content, 'Bad Request', 'BAD REQUEST'; +}; # need to list the expected test number because of the test case in the # exception class. -done_testing(8); +done_testing(26); From b419595f7f1655056287430d344dd87c102f9c88 Mon Sep 17 00:00:00 2001 From: John Napiorkowski Date: Thu, 26 Dec 2013 14:17:25 -0600 Subject: [PATCH 4/4] documented and created test case for when the exception occurs midway through a streamed response --- lib/Plack/Middleware/HTTPExceptions.pm | 41 ++++++++++++--- t/Plack-Middleware/httpexceptions-as_psgi.t | 55 ++++++++++++++++++++- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/lib/Plack/Middleware/HTTPExceptions.pm b/lib/Plack/Middleware/HTTPExceptions.pm index 03430c76f..b2c62ac91 100644 --- a/lib/Plack/Middleware/HTTPExceptions.pm +++ b/lib/Plack/Middleware/HTTPExceptions.pm @@ -31,14 +31,16 @@ sub call { try { $response->(sub { return $writer = $responder->(@_) }); } catch { - if ($writer) { - Carp::cluck $_; - $writer->close; - } else { - my $error_psgi_response = $self->transform_error($_, $env); - return $responder->($error_psgi_response) if ref $error_psgi_response eq 'ARRAY'; - return $unroll_coderef_responses->($responder, $error_psgi_response); - } + if($writer) { + # In the case where the exception happens part way through a write + # We just die since we can't at this point change the response + Carp::cluck $_; + $writer->close; + } else { + my $error_psgi_response = $self->transform_error($_, $env); + return $responder->($error_psgi_response) if ref $error_psgi_response eq 'ARRAY'; + return $unroll_coderef_responses->($responder, $error_psgi_response); + } }; }; @@ -150,6 +152,29 @@ enabled by default when C is set to C, so that the L middleware can catch it instead. +=head1 NOTES + +In the case where an exception rises during the middle of a streaming +response (such as the following): + + my $psgi_app = sub { + my $env = shift; + return sub { + my $responder = shift; + my $writer = $responder->([200, ['content-type'=>'text/html']]); + $writer->write('ok'); + + # Stuff... + + die MyApp::Exception::ServerError->new($env); + }; + +We can't meaningfully set the response from this exception, since at this +point HTTP Headers and a partial body have been returned. If you need to +verify such a case you'll need to rely on alternative means, such as setting +expected content-length or providing a checksum, that the client can use to +valid the returned content. + =head1 AUTHOR Tatsuhiko Miyagawa diff --git a/t/Plack-Middleware/httpexceptions-as_psgi.t b/t/Plack-Middleware/httpexceptions-as_psgi.t index bddcc2745..b94475690 100644 --- a/t/Plack-Middleware/httpexceptions-as_psgi.t +++ b/t/Plack-Middleware/httpexceptions-as_psgi.t @@ -166,6 +166,59 @@ test_psgi $psgi_app_delayed, sub { is $res->content, 'Bad Request', 'BAD REQUEST'; }; +ok my $psgi_app_delayed_with_write = sub { + my $env = shift; + return sub { + my $responder = shift; + my $writer = $responder->([200, ['content-type'=>'text/html']]); + $writer->write('ok'); + + die MyApp::Exception::Tuple->new( + 404, ['content-type'=>'text/plain'], ['Not Found']) + if $env->{PATH_INFO} eq '/tuple'; + + die MyApp::Exception::CodeRef->new( + 303, ['content-type'=>'text/plain'], ['See Other']) + if $env->{PATH_INFO} eq '/coderef'; + + die MyApp::Exception::CodeRefWithWrite->new( + 400, ['content-type'=>'text/plain'], ['Bad Request']) + if $env->{PATH_INFO} eq '/coderefwithwrite'; + + return $writer->close if $env->{PATH_INFO} eq '/ok'; }; +}; + +ok $psgi_app_delayed_with_write = Plack::Middleware::HTTPExceptions->wrap($psgi_app_delayed_with_write); + +test_psgi $psgi_app_delayed_with_write, sub { + my $cb = shift; + my $res = $cb->(GET "/tuple"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; +}; + +test_psgi $psgi_app_delayed_with_write, sub { + my $cb = shift; + my $res = $cb->(GET "/ok"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; +}; + +test_psgi $psgi_app_delayed_with_write, sub { + my $cb = shift; + my $res = $cb->(GET "/coderef"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; +}; + +test_psgi $psgi_app_delayed_with_write, sub { + my $cb = shift; + my $res = $cb->(GET "/coderefwithwrite"); + is $res->code, 200; + is $res->content, 'ok', 'OK'; +}; + + # need to list the expected test number because of the test case in the # exception class. -done_testing(26); +done_testing(36);