Bug 25504 - Wrong API spec breaks plack without meaningful error
Summary: Wrong API spec breaks plack without meaningful error
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Ere Maijala (away 13Jul-30Aug)
QA Contact: Testopia
URL:
Keywords: rel_20_11_target, RM_priority
Depends on: 24700
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-14 13:33 UTC by Jonathan Druart
Modified: 2020-06-18 14:12 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25504: Crash with the original error if API spec are invalid (1.80 KB, patch)
2020-05-14 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25504: Crash with the original error if API spec are invalid (1.86 KB, patch)
2020-05-14 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25504: Improve REST API spec loading errors (4.16 KB, patch)
2020-06-18 13:44 UTC, Ere Maijala (away 13Jul-30Aug)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-05-14 13:33:45 UTC
Without the patches from 25482, if you start plack under D10, you get the output from bug 25482 comment 11.

We should have something more meaningful to help the debug.
Comment 1 Jonathan Druart 2020-05-14 13:39:15 UTC
Created attachment 104884 [details] [review]
Bug 25504: Crash with the original error if API spec are invalid

Bug 24700 put the whole spec validation into a try catch block that
hides the original error raised by the JSON validator.

If you don't have the patches from bug 25482 and start plack under D10
you will see a huge stacktrace with, lost in the middle:
    Koha::REST::V1::catch {...} ("Duplicate keys not allowed, at character offset 4789 (before "...) called at /usr/share/perl5/Try/Tiny.pm line 123
The complete output is at bug 25482 comment 11.

With this patch, you will get a single line:
Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Duplicate keys not allowed, at character offset 4789 (before "editcatalogue": "cre...") at /usr/share/perl5/Mojo/JSON.pm line 39.

I am all for stacktraces, but not if the error is hidden ;)
Comment 2 Tomás Cohen Arazi 2020-05-14 13:48:40 UTC
Should we have regression tests?
Comment 3 Tomás Cohen Arazi 2020-05-14 18:17:20 UTC
Created attachment 104894 [details] [review]
Bug 25504: Crash with the original error if API spec are invalid

Bug 24700 put the whole spec validation into a try catch block that
hides the original error raised by the JSON validator.

If you don't have the patches from bug 25482 and start plack under D10
you will see a huge stacktrace with, lost in the middle:
    Koha::REST::V1::catch {...} ("Duplicate keys not allowed, at character offset 4789 (before "...) called at /usr/share/perl5/Try/Tiny.pm line 123
The complete output is at bug 25482 comment 11.

With this patch, you will get a single line:
Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Duplicate keys not allowed, at character offset 4789 (before "editcatalogue": "cre...") at /usr/share/perl5/Mojo/JSON.pm line 39.

I am all for stacktraces, but not if the error is hidden ;)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2020-05-14 21:57:45 UTC
The use of $validator->bundle breaks Stretch
Comment 5 Tomás Cohen Arazi 2020-05-14 22:11:08 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> The use of $validator->bundle breaks Stretch

It is always falling back to the catch block in stretch........
Comment 6 Tomás Cohen Arazi 2020-05-14 22:47:54 UTC
Jonathan, I didn't manage to make things work on JSON::Validator 0.97. I give up on this one.

Lets discuss using local::lib as everyone else, and bundle our dependencies...
Comment 7 Jonathan Druart 2020-05-15 09:06:53 UTC
It means that the improvement speed bug 24700 was supposed to bring, never actually worked?
Comment 8 Tomás Cohen Arazi 2020-05-15 10:55:43 UTC
(In reply to Jonathan Druart from comment #7)
> It means that the improvement speed bug 24700 was supposed to bring, never
> actually worked?

It means not in stretch. Unless you cpan some libs to buster version
Comment 9 Jonathan Druart 2020-05-15 11:38:39 UTC
Stretch is the only OS we really support right now.
Comment 10 Tomás Cohen Arazi 2020-05-15 12:51:46 UTC
(In reply to Jonathan Druart from comment #9)
> Stretch is the only OS we really support right now.

I tried by all means to make the validator fail on the broken spec (the permissions problem in advancededitor_macros) without luck.

I think this means the 'feature' doesn't work on Stretch, but is not breaking anything either.
Comment 11 Jonathan Druart 2020-06-17 09:55:17 UTC
This is really problematic as it hides the original error.

Ere could you have a look?
Comment 12 Jonathan Druart 2020-06-17 09:56:29 UTC
I think we should at least log the error from the try block.
Comment 13 Ere Maijala (away 13Jul-30Aug) 2020-06-18 06:07:20 UTC
I'll take a look. I can't see right away how Bug 24700 broke this, since the try block is just trying to take a shortcut, and if validation fails, it should do the very same as before the patch. I'm probably missing a detail here, but I'll try with D10 to see what happens.

Letting it crash with the failed bundle might be confusing since the result is an amalgamation of the basic spec and any REST plugins. That's why it's swallowed and the one-by-one validation takes place in the catch block.
Comment 14 Jonathan Druart 2020-06-18 07:27:28 UTC
Ere, the problem I am having is for debugging purpose.
Say you have a syntax problem in one of your json files, the error from the first try block will not be logged.
Comment 15 Ere Maijala (away 13Jul-30Aug) 2020-06-18 13:44:12 UTC
Created attachment 106011 [details] [review]
Bug 25504: Improve REST API spec loading errors

Test plan:
1. Introduce a typo in swagger.json or another spec json file.
2. Restart plack if used or try to access the REST APIs
3. Without the patch, verify that  an incomplete error message and potentially lots of stack trace are logged.
4. With the patch, verify that much more meaningful error messages are logged and stack trace is omitted.
5. Fix the problem introduced in step 1 and verify that no messages are logged, or only warning about bundle is logged with Debian Stretch.
6. Repeat with a REST API plugin.
Comment 16 Ere Maijala (away 13Jul-30Aug) 2020-06-18 13:45:38 UTC
Would the attached patch suffice?
Comment 17 Jonathan Druart 2020-06-18 14:12:04 UTC
At first glance it looks great, thanks Ere!