Bug 18206 - REST API: Default exception handling
Summary: REST API: Default exception handling
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Lari Taskula
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 18205
Blocks: 14843 19652
  Show dependency treegraph
 
Reported: 2017-03-03 13:24 UTC by Lari Taskula
Modified: 2019-02-26 12:55 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18206: Default exception handling for REST API (14.35 KB, patch)
2017-03-14 16:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Use this feature for cities (2.79 KB, patch)
2017-03-14 16:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Default exception handling for REST API (14.42 KB, patch)
2017-03-14 16:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Use this feature for cities (2.79 KB, patch)
2017-03-14 16:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Default exception handling for REST API (14.39 KB, patch)
2017-03-16 13:04 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Use this feature for cities (2.79 KB, patch)
2017-03-16 13:04 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206 - REST API: Default exception handling - Squashable, fix regexp (1.28 KB, patch)
2017-10-18 11:51 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18206: Default exception handling for REST API (14.44 KB, patch)
2017-11-20 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Use this feature for cities (2.79 KB, patch)
2017-11-20 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206 - REST API: Default exception handling - Squashable, fix regexp (1.28 KB, patch)
2017-11-20 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18206: Default exception handling for REST API (14.07 KB, patch)
2019-02-26 12:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18206: Use this feature for cities (3.16 KB, patch)
2019-02-26 12:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18206: Fix missing use in api/v1.t test (704 bytes, patch)
2019-02-26 12:55 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2017-03-03 13:24:09 UTC
Improve exception handling in REST controllers.
Comment 1 Lari Taskula 2017-03-14 16:10:36 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2017-03-14 16:22:49 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2017-03-14 16:43:34 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2017-03-14 16:43:46 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2017-03-16 13:04:23 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2017-03-16 13:04:50 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2017-08-23 17:10:13 UTC
Lari, do you think we could call the method 'rethrow'?
Comment 8 Olli-Antti Kivilahti 2017-10-18 11:51:10 UTC Comment hidden (obsolete)
Comment 9 Olli-Antti Kivilahti 2017-10-18 12:12:48 UTC
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?
Comment 10 Katrin Fischer 2017-10-22 10:18:56 UTC
Depends on a patch that doesn't apply, marking BLOCKED for now. Please fix dependency tree from the top and reset status!
Comment 11 Lari Taskula 2017-11-20 12:12:10 UTC
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
Comment 12 Lari Taskula 2017-11-20 12:12:23 UTC
Created attachment 69230 [details] [review]
Bug 18206: Use this feature for cities

To test:
1. prove t/db_dependent/api/v1/cities.t
Comment 13 Lari Taskula 2017-11-20 12:12:39 UTC
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.
Comment 14 Josef Moravec 2019-02-26 12:55:03 UTC
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>
Comment 15 Josef Moravec 2019-02-26 12:55:07 UTC
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>
Comment 16 Josef Moravec 2019-02-26 12:55:10 UTC
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>