Improve exception handling in REST controllers.
Created attachment 61072 [details] [review] 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
Created attachment 61073 [details] [review] Bug 18206: Use this feature for cities To test: 1. prove t/db_dependent/api/v1/cities.t
Created attachment 61087 [details] [review] 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
Created attachment 61088 [details] [review] Bug 18206: Use this feature for cities To test: 1. prove t/db_dependent/api/v1/cities.t
Created attachment 61170 [details] [review] 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
Created attachment 61171 [details] [review] Bug 18206: Use this feature for cities To test: 1. prove t/db_dependent/api/v1/cities.t
Lari, do you think we could call the method 'rethrow'?
Created attachment 68243 [details] [review] Bug 18206 - REST API: Default exception handling - Squashable, fix regexp t/db_dependent/api/v1.t .. Unescaped left brace in regex is deprecated, passed through in regex; marked by <-- HERE in m/ERROR - Unknown::Exception::OhNo => { <-- HERE "what":"test unknown exception"}/ at t/db_dependent/api/v1.t line 100.
Wow! This feature is orgastic. Looking at the codebase, it is obvious Perl has way too many types of Exceptions. This will turn this incredible frustration to something manageable. >> Lari, do you think we could call the method 'rethrow'? rethrow() is already reserved by Exception::Class?
Depends on a patch that doesn't apply, marking BLOCKED for now. Please fix dependency tree from the top and reset status!
Created attachment 69229 [details] [review] 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
Created attachment 69230 [details] [review] Bug 18206: Use this feature for cities To test: 1. prove t/db_dependent/api/v1/cities.t
Created attachment 69231 [details] [review] Bug 18206 - REST API: Default exception handling - Squashable, fix regexp t/db_dependent/api/v1.t .. Unescaped left brace in regex is deprecated, passed through in regex; marked by <-- HERE in m/ERROR - Unknown::Exception::OhNo => { <-- HERE "what":"test unknown exception"}/ at t/db_dependent/api/v1.t line 100.
Created attachment 85715 [details] [review] 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 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 85716 [details] [review] Bug 18206: Use this feature for cities To test: 1. prove t/db_dependent/api/v1/cities.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 85717 [details] [review] Bug 18206: Fix missing use in api/v1.t test Test plan: prove t/d_dependent/api/v1.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
I like the idea of a more generic fallback exception handling method. I'm not sure how it fits what we already do in Koha::REST::V1::Auth::under. I will mark it as blocked until bug 18205 moves to avoid confusion.
I'd love to see this one start moving again..
Some ideas from this bug could be reused on top of bug 25032. Marking as duplicate for now. *** This bug has been marked as a duplicate of bug 25032 ***