From e20530561f5dc67c4d6d467fa8c3fc647487f9e1 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Tue, 14 Mar 2017 16:17:40 +0200 Subject: [PATCH] Bug 18206: Default exception handling for REST API Many of our operations in REST API controllers now use try-catch blocks to catch exceptions and handle them appropriately. This is great, but we should introduce a centralized way of handling default HTTP 500 errors. Currently they are checked over and over again in each operation. As an example this same lovely poem, in many cases, is currently replicated for each operation: sub list { ... try { blabla } catch { # This should stay here, custom error handling for this particular operation if ($_->isa('Koha::Exceptions::Patron::Something')) { return $c->render(status => 400, openapi => { error => $_->error }); } # But the checks below can be centralized! elsif ($_->isa('DBIx::Class::Exception')) { return $c->render(status => 500, openapi => { error => $_->{msg} }); } elsif ($_->isa('Koha::Exceptions::Exception')) { return $c->render(status => 500, openapi => { error => $_->error }); } else { return $c->render(status => 500, openapi => { error => "Something went wrong, check the logs." }); } }; } Instead, my proposal for a more centralized solution is to use a before_render hook to catch all of the default exceptions before rendering that are expected to return a 500, logging the error and displaying an appropriate error message in response body. After this patch, the above example would then look like this: sub list { ... try { blabla } catch { # This should stay here, custom error handling for this particular operation if ($_->isa('Koha::Exceptions::Patron::Something')) { return $c->render(status => 400, openapi => { error => $_->error }); } # Simply rethrow the exception with the help of below function - it will then # be handled in before_render hook Koha::Exceptions::rethrow_exception($_); }; } What does this patch actually do? After this patch, in case of an exception, we will normally visit the catch-block. If none of the specified Koha::Exceptions match the thrown $_, we will now rethrow the exception. This does not crash the whole app, but forwards the exception eventually into our before_render hook at Koha::REST::V1::handle_default_exceptions. There, we are able to customize our way of handling these exceptions. In this patch I have added an error logging there. We should also discuss whether we want to display a detailed error message, or simply "Something went wrong, check the logs." for all of the default exceptions. Perhaps this could be controlled by some sort of configuration for development/production (e.g. MOJO_MODE) ? To test: 1. prove t/db_dependent/api 2. prove t/Koha/Exceptions.t --- Koha/Exceptions.pm | 71 ++++++++++++++++++++++ Koha/REST/V1.pm | 28 +++++++++ t/Koha/Exceptions.t | 74 +++++++++++++++++++++++ t/db_dependent/api/v1.t | 154 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 327 insertions(+) create mode 100644 t/Koha/Exceptions.t create mode 100644 t/db_dependent/api/v1.t diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index 9d7f397..f08514b 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -68,4 +68,75 @@ use Exception::Class ( } ); +use Mojo::JSON; +use Scalar::Util qw( blessed ); + +=head1 NAME + +Koha::Exceptions + +=head1 API + +=head2 Class Methods + +=head3 rethrow_exception + + try { + # .. + } catch { + # .. + Koha::Exceptions::rethrow_exception($e); + } + +A function for re-throwing any given exception C<$e>. This also includes other +exceptions than Koha::Exceptions. + +=cut + +sub rethrow_exception { + my ($e) = @_; + + die $e unless blessed($e); + die $e if ref($e) eq 'Mojo::Exception'; # Mojo::Exception is rethrown by die + die $e unless $e->can('rethrow'); + $e->rethrow; +} + +=head3 to_str + +A function for representing any given exception C<$e> as string. + +C is aware of some of the most common exceptions and how to stringify +them, however, also stringifies unknown exceptions by encoding them into JSON. + +=cut + +sub to_str { + my ($e) = @_; + + return (ref($e) ? ref($e) ." => " : '') . _stringify_exception($e); +} + +sub _stringify_exception { + my ($e) = @_; + + return $e unless blessed($e); + + # Stringify a known exception + return $e->to_string if ref($e) eq 'Mojo::Exception'; + return $e->{'msg'} if ref($e) eq 'DBIx::Class::Exception'; + return $e->error if $e->isa('Koha::Exception'); + + # Stringify an unknown exception by attempting to use some methods + return $e->to_str if $e->can('to_str'); + return $e->to_string if $e->can('to_string'); + return $e->error if $e->can('error'); + return $e->message if $e->can('message'); + return $e->string if $e->can('string'); + return $e->str if $e->can('str'); + + # Finally, handle unknown exception by encoding it into JSON text + return Mojo::JSON::encode_json({%$e}); +} + 1; diff --git a/Koha/REST/V1.pm b/Koha/REST/V1.pm index 667f12c..9641409 100644 --- a/Koha/REST/V1.pm +++ b/Koha/REST/V1.pm @@ -50,6 +50,8 @@ sub startup { $self->secrets([$secret_passphrase]); } + $self->app->hook(before_render => \&default_exception_handling); + $self->plugin(OpenAPI => { url => $self->home->rel_file("api/v1/swagger/swagger.json"), route => $self->routes->under('/api/v1')->to('Auth#under'), @@ -60,4 +62,30 @@ sub startup { $self->plugin( 'Koha::REST::Plugin::Pagination' ); } +=head3 default_exception_handling + +A before_render hook for handling default exceptions. + +=cut + +sub default_exception_handling { + my ($c, $args) = @_; + + if ($args->{exception} && $args->{exception}->{message}) { + my $e = $args->{exception}->{message}; + $c->app->log->error(Koha::Exceptions::to_str($e)); + %$args = ( + status => 500, + # TODO: Do we want a configuration for displaying either + # a detailed description of the error or simply a "Something + # went wrong, check the logs."? Now that we can stringify all + # exceptions with Koha::Exceptions::to_str($e), we could also + # display the detailed error if some DEBUG variable is enabled. + # Of course the error is still logged if log4perl is configured + # appropriately... + json => { error => 'Something went wrong, check the logs.' } + ); + } +} + 1; diff --git a/t/Koha/Exceptions.t b/t/Koha/Exceptions.t new file mode 100644 index 0000000..176368f --- /dev/null +++ b/t/Koha/Exceptions.t @@ -0,0 +1,74 @@ +#!/usr/bin/perl +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Test::More tests => 2; + +use Koha::Exceptions; +use Mojo::Exception; +use DBIx::Class::Exception; + +subtest 'rethrow_exception() tests' => sub { + plan tests => 4; + + my $e = Koha::Exceptions::Exception->new( + error => 'houston, we have a problem' + ); + eval { Koha::Exceptions::rethrow_exception($e) }; + is(ref($@), 'Koha::Exceptions::Exception', ref($@)); + + eval { DBIx::Class::Exception->throw('dang') }; + $e = $@; + eval { Koha::Exceptions::rethrow_exception($e) }; + is(ref($@), 'DBIx::Class::Exception', ref($@)); + + eval { Mojo::Exception->throw('dang') }; + $e = $@; + eval { Koha::Exceptions::rethrow_exception($e) }; + is(ref($@), 'Mojo::Exception', ref($@)); + + eval { die "wow" }; + $e = $@; + eval { Koha::Exceptions::rethrow_exception($e) }; + like($@, qr/^wow at .*Exceptions.t line \d+\.$/, $@); +}; + +subtest 'to_str() tests' => sub { + plan tests => 4; + + my $text; + eval { Koha::Exceptions::Exception->throw(error => 'dang') }; + is($text = Koha::Exceptions::to_str($@), + 'Koha::Exceptions::Exception => dang', $text); + eval { DBIx::Class::Exception->throw('dang') }; + like($text = Koha::Exceptions::to_str($@), + qr/DBIx::Class::Exception => .*dang/, $text); + eval { Mojo::Exception->throw('dang') }; + is($text = Koha::Exceptions::to_str($@), + 'Mojo::Exception => dang', $text); + eval { + my $exception = { + what => 'test unknown exception', + otherstuffs => 'whatever' + }; + bless $exception, 'Unknown::Exception'; + die $exception; + }; + is($text = Koha::Exceptions::to_str($@), 'Unknown::Exception => ' + .'{"otherstuffs":"whatever","what":"test unknown exception"}', $text); +}; diff --git a/t/db_dependent/api/v1.t b/t/db_dependent/api/v1.t new file mode 100644 index 0000000..832fa4e --- /dev/null +++ b/t/db_dependent/api/v1.t @@ -0,0 +1,154 @@ +#!/usr/bin/env perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Test::More tests => 1; +use Test::Mojo; +use Test::Warn; + +use t::lib::Mocks; +use Koha::Exceptions; + +use Log::Log4perl; +use Mojolicious::Lite; +use Try::Tiny; + +my $config = { + 'log4perl.logger.rest.Koha.REST.V1' => 'ERROR, TEST', + 'log4perl.appender.TEST' => 'Log::Log4perl::Appender::TestBuffer', + 'log4perl.appender.TEST.layout' => 'SimpleLayout', +}; +t::lib::Mocks::mock_config('log4perl_conf', $config); + +my $remote_address = '127.0.0.1'; +my $t = Test::Mojo->new('Koha::REST::V1'); +my $tx; + +subtest 'default_exception_handling() tests' => sub { + plan tests => 5; + + add_default_exception_routes($t); + + my $appender = Log::Log4perl->appenders->{TEST}; + + subtest 'Mojo::Exception' => sub { + plan tests => 4; + + $t->get_ok('/default_exception_handling/mojo') + ->status_is(500) + ->json_is('/error' => 'Something went wrong, check the logs.'); + + like($appender->buffer, qr/ERROR - test mojo exception/, + 'Found test mojo exception in log'); + $appender->{appender}->{buffer} = undef; + }; + + subtest 'die() outside try { } catch { };' => sub { + plan tests => 4; + + $t->get_ok('/default_exception_handling/dieoutsidetrycatch') + ->status_is(500) + ->json_is('/error' => 'Something went wrong, check the logs.'); + like($appender->buffer, qr/ERROR - die outside try-catch/, + 'Found die outside try-catch in log'); + $appender->{appender}->{buffer} = undef; + }; + + subtest 'DBIx::Class::Exception' => sub { + plan tests => 4; + + $t->get_ok('/default_exception_handling/dbix') + ->status_is(500) + ->json_is('/error' => 'Something went wrong, check the logs.'); + like($appender->buffer, qr/ERROR - DBIx::Class::Exception => .* test dbix exception/, + 'Found test dbix exception in log'); + $appender->{appender}->{buffer} = undef; + }; + + subtest 'Koha::Exceptions::Exception' => sub { + plan tests => 4; + + $t->get_ok('/default_exception_handling/koha') + ->status_is(500) + ->json_is('/error' => 'Something went wrong, check the logs.'); + like($appender->buffer, qr/ERROR - Koha::Exceptions::Exception => test koha exception/, + 'Found test koha exception in log'); + $appender->{appender}->{buffer} = undef; + }; + + subtest 'Unknown exception' => sub { + plan tests => 4; + + $t->get_ok('/default_exception_handling/unknown') + ->status_is(500) + ->json_is('/error' => 'Something went wrong, check the logs.'); + like($appender->buffer, qr/ERROR - Unknown::Exception::OhNo => {"what":"test unknown exception"}/, + 'Found test unknown exception in log'); + $appender->{appender}->{buffer} = undef; + }; +}; + +sub add_default_exception_routes { + my ($t) = @_; + + # Mojo::Exception + $t->app->routes->get('/default_exception_handling/mojo' => sub { + try { + die "test mojo exception"; + } catch { + Koha::Exceptions::rethrow_exception($_); + }; + }); + + # die outside try-catch + $t->app->routes->get('/default_exception_handling/dieoutsidetrycatch' => sub { + die "die outside try-catch"; + }); + + # DBIx::Class::Exception + $t->app->routes->get('/default_exception_handling/dbix' => sub { + package Koha::REST::V1::Test; + try { + DBIx::Class::Exception->throw('test dbix exception'); + } catch { + Koha::Exceptions::rethrow_exception($_); + }; + }); + + # Koha::Exceptions::Exception + $t->app->routes->get('/default_exception_handling/koha' => sub { + try { + Koha::Exceptions::Exception->throw('test koha exception'); + } catch { + Koha::Exceptions::rethrow_exception($_); + }; + }); + + # Unknown exception + $t->app->routes->get('/default_exception_handling/unknown' => sub { + try { + my $exception = { what => 'test unknown exception'}; + bless $exception, 'Unknown::Exception::OhNo'; + die $exception; + } catch { + Koha::Exceptions::rethrow_exception($_); + }; + }); +} + +1; -- 2.7.4