Bugzilla – Attachment 61087 Details for
Bug 18206
REST API: Default exception handling
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18206: Default exception handling for REST API
Bug-18206-Default-exception-handling-for-REST-API.patch (text/plain), 14.42 KB, created by
Lari Taskula
on 2017-03-14 16:43:34 UTC
(
hide
)
Description:
Bug 18206: Default exception handling for REST API
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2017-03-14 16:43:34 UTC
Size:
14.42 KB
patch
obsolete
>From 6e4402577870a4d2bd4ac36f690e5dda3fa48583 Mon Sep 17 00:00:00 2001 >From: Lari Taskula <lari.taskula@jns.fi> >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 | 76 ++++++++++++++++++++++++ > t/db_dependent/api/v1.t | 154 ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 329 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 9adc06e..20d560f 100644 >--- a/Koha/Exceptions.pm >+++ b/Koha/Exceptions.pm >@@ -53,4 +53,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<to_str> 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 9fa43f8..1af33ef 100644 >--- a/Koha/REST/V1.pm >+++ b/Koha/REST/V1.pm >@@ -56,6 +56,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'), >@@ -65,4 +67,30 @@ sub startup { > }); > } > >+=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..ccf5a60 >--- /dev/null >+++ b/t/Koha/Exceptions.t >@@ -0,0 +1,76 @@ >+#!/usr/bin/perl >+# >+# Copyright 2014 Catalyst IT >+# >+# 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; >-- >1.9.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18206
:
61072
|
61073
|
61087
|
61088
|
61170
|
61171
|
68243
|
69229
|
69230
|
69231
|
85715
|
85716
|
85717