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.
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 ;)
Should we have regression tests?
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>
The use of $validator->bundle breaks Stretch
(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........
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...
It means that the improvement speed bug 24700 was supposed to bring, never actually worked?
(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
Stretch is the only OS we really support right now.
(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.
This is really problematic as it hides the original error. Ere could you have a look?
I think we should at least log the error from the try block.
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.
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.
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.
Would the attached patch suffice?
At first glance it looks great, thanks Ere!
Could we add: my $logger = Koha::Logger->get({ interface => 'api' }); $logger->error("$_"); instead of carp?
It's now pretty verbose but at least we have the full stacktrace!
Created attachment 109227 [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.
Created attachment 109228 [details] [review] Bug 25504: Use Koha::Logger and display the full stacktrace
Created attachment 109233 [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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 109234 [details] [review] Bug 25504: Use Koha::Logger and display the full stacktrace Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 109235 [details] [review] Bug 25504: (QA follow-up) Remove unused include Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 109420 [details] [review] Bug 25504: Fix failing tests
(In reply to Jonathan Druart from comment #26) > Created attachment 109420 [details] [review] [review] > Bug 25504: Fix failing tests Pushed to master for 20.11.
doesnt apply clean to 20.05.x, no backport if needed please rebase