Bug 24700 - Improve Mojo startup speed for REST APIs
Summary: Improve Mojo startup speed for REST APIs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Ere Maijala
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 25504
  Show dependency treegraph
 
Reported: 2020-02-20 14:01 UTC by Ere Maijala
Modified: 2020-11-30 21:49 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24700: Avoid validating REST API spec multiple times (4.49 KB, patch)
2020-02-20 14:19 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24700: Avoid validating REST API spec multiple times (4.54 KB, patch)
2020-02-20 15:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24700: Avoid validating REST API spec multiple times (4.54 KB, patch)
2020-02-20 15:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24700: Avoid validating REST API spec multiple times (4.60 KB, patch)
2020-03-26 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ere Maijala 2020-02-20 14:01:49 UTC
Currently the API spec is validated multiple times, and the whole spec loaded so far is re-validated for each (if any) REST API plugin. Coming up with a patch to avoid multiple validation unless the complete spec doesn't validate.
Comment 1 Ere Maijala 2020-02-20 14:19:32 UTC
Created attachment 99314 [details] [review]
Bug 24700: Avoid validating REST API spec multiple times

This patch changes the spec loading so that it is fully loaded and merged with any plugin routes before validation. The individual parts are separately validated only if the resulting complete spec is invalid. Note: the OpenAPI plugin validates the spec given to it, so normally no manual validation is necessary.

Test plan:
1. prove -v t/db_dependent/Koha/REST/*
2. prove -v t/db_dependent/api/v1/*

Sponsored-by: The National Library of Finland
Comment 2 Martin Renvoize 2020-02-20 15:06:53 UTC
Created attachment 99319 [details] [review]
Bug 24700: Avoid validating REST API spec multiple times

This patch changes the spec loading so that it is fully loaded and merged with any plugin routes before validation. The individual parts are separately validated only if the resulting complete spec is invalid. Note: the OpenAPI plugin validates the spec given to it, so normally no manual validation is necessary.

Test plan:
1. prove -v t/db_dependent/Koha/REST/*
2. prove -v t/db_dependent/api/v1/*

Sponsored-by: The National Library of Finland
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-02-20 15:16:54 UTC
Created attachment 99320 [details] [review]
Bug 24700: Avoid validating REST API spec multiple times

This patch changes the spec loading so that it is fully loaded and merged with any plugin routes before validation. The individual parts are separately validated only if the resulting complete spec is invalid. Note: the OpenAPI plugin validates the spec given to it, so normally no manual validation is necessary.

Test plan:
1. prove -v t/db_dependent/Koha/REST/*
2. prove -v t/db_dependent/api/v1/*

Sponsored-by: The National Library of Finland
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2020-02-20 15:18:24 UTC
This is a nice improvement and the code is fairly clear.

I'm struggling a little to find where this is tested to date though.. so I haven't run tests against it yet to fully prove there are no regressions.. human testing gave me no issues though so I'm signing off.
Comment 5 Ere Maijala 2020-02-21 07:12:38 UTC
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t contains the tests for PluginRoutes, but I don't think there's anything for V1.pm's startup. I can try to create something, but it's pretty huge if all that the startup method does is to be tested.
Comment 6 Martin Renvoize 2020-02-25 15:28:07 UTC
I'll just verify for myself that the pluginroutes tests cover us.. thanks for the pointer.
Comment 7 Katrin Fischer 2020-03-22 13:36:01 UTC
Don't understand the code well enough to QA here - still gave it a go to provoke some movement. 

Might be my data, but I have failing tests with and without the patch:

Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255

#   Failed test 'No tests run for subtest "Disabled plugins tests"'
#   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 121.
Can't locate Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm in @INC (you may need to install the Koha::Plugin::Com::ByWaterSolutions::KitchenSink module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base t/db_dependent/Koha/REST/Plugin/../../../../lib) at /usr/share/perl/5.24/Module/Load.pm line 77.
Can't locate Koha/Plugin/Com/ByWaterSolutions/KitchenSink in @INC (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base t/db_dependent/Koha/REST/Plugin/../../../../lib) at /usr/share/perl/5.24/Module/Load.pm line 77.
# Looks like your test exited with 255 just after 2.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 


t/db_dependent/api/v1/acquisitions_orders.t          (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1

[Sun Mar 22 13:31:12 2020] [debug] GET "/api/v1/acquisitions/orders"
[Sun Mar 22 13:31:12 2020] [debug] Routing to controller "Koha::REST::V1::Auth" and action "under"
[Sun Mar 22 13:31:12 2020] [debug] 400 Bad Request (0.027112s, 36.884/s)
    ok 6 - GET //W0txdhLojIJjsOm41yB0E6aNafhS1vdorHkDUn9Z17KvscSt:thePassword123@/api/v1/acquisitions/orders?order_blah=blah
    ok 7 - 400 Bad Request
    ok 8 - exact match for JSON Pointer ""
    # Looks like you failed 2 tests of 8.
not ok 1 - list() tests
Comment 8 Tomás Cohen Arazi 2020-03-26 19:24:10 UTC
Created attachment 101906 [details] [review]
Bug 24700: Avoid validating REST API spec multiple times

This patch changes the spec loading so that it is fully loaded and merged with any plugin routes before validation. The individual parts are separately validated only if the resulting complete spec is invalid. Note: the OpenAPI plugin validates the spec given to it, so normally no manual validation is necessary.

Test plan:
1. prove -v t/db_dependent/Koha/REST/*
2. prove -v t/db_dependent/api/v1/*

Sponsored-by: The National Library of Finland
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Martin Renvoize 2020-03-27 12:24:06 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 10 Joy Nelson 2020-04-03 22:49:43 UTC
enhancement not backported to 19.11.x