PerlDancer / Dancer2

Perl Dancer Next Generation (rewrite of Perl Dancer)
http://perldancer.org/
Other
546 stars 274 forks source link

Error after returning content from before hook #944

Closed dtcyganov closed 9 years ago

dtcyganov commented 9 years ago

Here is test case, which reproduces bug:

use strict;
use warnings;

use Test::More;
use Plack::Test;
use HTTP::Request::Common;

{
    package App; ## no critic
    use Dancer2;
    set serializer => 'JSON';
    hook before => sub {
        response->content({ foo => 'bar' });
        response->halt;
    };

    get '/' => sub {1};
}

my $test = Plack::Test->create( App->to_app );

my $res = $test->request( GET '/' );
ok( $res->is_success, 'Successful request' );
is( $res->content, '{"foo":"bar"}', 'Correct content' );

It fails with error "Failed to serialize the request: hash- or arrayref expected (not a simple scalar, use allow_nonref to allow this) at /usr/local/booking-perl/5.18.2/site/lib/JSON.pm line 154. in (eval 106) l. 1". This happens because serialiser applies two times. First when we call content(…) from the hook and second call is from Dancer2/Core/App.pm from method _add_content_to_response, which is called from _dispatch_route:

if ( $response->is_halted ) {
    return $self->_add_content_to_response(
        $response, $response->content,
     );
} 
veryrusty commented 9 years ago

Thanks for reporting this @dtcyganov !

There is no need to set the response content to the value of the responses content (that is a little loopy!). I also found that the same issue applied to any route that sets response content. eg.

    get '/content' => sub {
        response->content({ foo => 'bar' });
        return 'this is ignored';
    };
xsawyerx commented 9 years ago

@veryrusty While it isn't useful to do in a regular route request, we are trying to do it at $work in order to prevent the running of the route. This prevents having to decorate all the routes.

veryrusty commented 9 years ago

I re-read my comment after a nights sleep; it was meant as a note to myself that the issue involved more than just before hooks. Sorry @dtcyganov & @xsawyerx if that caused any confusion.

(I have a patch, just cleaning up some tests)

veryrusty commented 9 years ago

Bugfix in #946 with somewhat of a better explanation :) Thanks again @dtcyganov for reporting and supplying the test case!

xsawyerx commented 9 years ago

Whoa! @veryrusty++

@dtcyganov, now you have to fix the next bug we find at work. :)