Bug 26143 - The API does not handle requesting all resources
Summary: The API does not handle requesting all resources
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 26162
  Show dependency treegraph
 
Reported: 2020-08-05 11:03 UTC by Jonathan Druart
Modified: 2020-09-20 12:14 UTC (History)
7 users (show)

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


Attachments
Bug 26143: The API is always paginated (2.64 KB, patch)
2020-08-05 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26143: Regression tests (5.95 KB, patch)
2020-08-05 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26143: Make the API handle per_page=-1 (4.22 KB, patch)
2020-08-05 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26143: The API is always paginated (2.71 KB, patch)
2020-08-05 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26143: Regression tests (6.02 KB, patch)
2020-08-05 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26143: Make the API handle per_page=-1 (4.29 KB, patch)
2020-08-05 15:35 UTC, Jonathan Druart
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-08-05 11:03:51 UTC
http://kohadev-intra.mydnsname.org:8081/api/v1/libraries?_page=1&_per_page=-1&_match=contains&_order_by=%2Bme.name

[2020-08-05 11:02:43.54213] [6610] [debug] GET "/api/v1/libraries" (c37dcb31)
[2020-08-05 11:02:43.54286] [6610] [debug] Routing to controller "Koha::REST::V1::Auth" and action "under"
[2020-08-05 11:02:43.55991] [6610] [debug] Routing to controller "Koha::REST::V1::Libraries" and action "list"
No method pager found for Koha::Libraries Fewer than one entry per page! at /usr/share/perl5/DBIx/Class/ResultSet.pm line 2542.
 at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 127.
[2020-08-05 11:02:43.57390] [6610] [debug] 500 Internal Server Error (0.031753s, 31.493/s)


It has been caught by a selenium failure:
An element could not be located on the page using the given search parameters: //div[@class="main container-fluid"]//a[contains(@href, "/admin/branches.pl?op=add_form&branchcode=UT_BC")],xpath at /kohadevbox/koha/t/lib/Selenium.pm line 174.
Comment 1 Tomás Cohen Arazi 2020-08-05 15:25:51 UTC
Created attachment 107836 [details] [review]
Bug 26143: The API is always paginated

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-08-05 15:25:54 UTC
Created attachment 107837 [details] [review]
Bug 26143: Regression tests

This patch introduces tests for the per_page=-1 handling use case. From
now on per_page=-1 means 'all resources'.

On writing this I noticed that we always paginate results no matter
what, but there was a weird condition under which on pagination headers
were sent back to the API consumer. This is highlighted in the precedent
patch, which is not the -1 situation this one tries to tackle.

Both pagination and searching are broken with per_page=-1, which is a
standard, and we actually didn't explicitly set a way to request all
resources.

To verify this:
1. Apply the previous tests patch and this one
2. Run:
   $ kshell
  k$ prove t/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Things are damn broken

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-08-05 15:25:58 UTC
Created attachment 107838 [details] [review]
Bug 26143: Make the API handle per_page=-1

This patch introduces handling for per_page=-1 and actually adds a
missing feature to the API: being able to request all resources on a
route.

To test this:
1. Visit the libraries admin page
2. On the table, choose to display 'All' rows
=> FAIL: it doesn't refresh, the browser console displays a 500 error
code and so the logs
3. Apply the tests patches
4. Run:
   $ kshell
  k$ prove t/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail loudly
5. Apply this patch
6. Restart plack
7. Repeat 2
=> SUCCESS: choosing to display all, works
8. Repeat 4
=> SUCCESS: Tests pass now!
9. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Jonathan Druart 2020-08-05 15:35:03 UTC
Created attachment 107839 [details] [review]
Bug 26143: The API is always paginated

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2020-08-05 15:35:07 UTC
Created attachment 107840 [details] [review]
Bug 26143: Regression tests

This patch introduces tests for the per_page=-1 handling use case. From
now on per_page=-1 means 'all resources'.

On writing this I noticed that we always paginate results no matter
what, but there was a weird condition under which on pagination headers
were sent back to the API consumer. This is highlighted in the precedent
patch, which is not the -1 situation this one tries to tackle.

Both pagination and searching are broken with per_page=-1, which is a
standard, and we actually didn't explicitly set a way to request all
resources.

To verify this:
1. Apply the previous tests patch and this one
2. Run:
   $ kshell
  k$ prove t/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Things are damn broken

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2020-08-05 15:35:11 UTC
Created attachment 107841 [details] [review]
Bug 26143: Make the API handle per_page=-1

This patch introduces handling for per_page=-1 and actually adds a
missing feature to the API: being able to request all resources on a
route.

To test this:
1. Visit the libraries admin page
2. On the table, choose to display 'All' rows
=> FAIL: it doesn't refresh, the browser console displays a 500 error
code and so the logs
3. Apply the tests patches
4. Run:
   $ kshell
  k$ prove t/Koha/REST/Plugin/Pagination.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail loudly
5. Apply this patch
6. Restart plack
7. Repeat 2
=> SUCCESS: choosing to display all, works
8. Repeat 4
=> SUCCESS: Tests pass now!
9. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2020-08-05 15:37:11 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 8 Jonathan Druart 2020-08-06 09:52:13 UTC
Got something new on Master_U18 #862, I am still not sure it's directly related to this.


18:47:07 selenium_1   | 09:47:07.478 WARN - Exception: Element not found in the cache - perhaps the page has changed since it was looked up
18:47:07 selenium_1   | For documentation on this error, please visit: http://seleniumhq.org/exceptions/stale_element_reference.html
18:47:07 selenium_1   | Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
18:47:07 selenium_1   | System info: host: '78b9a07f51f2', ip: '192.168.16.2', os.name: 'Linux', os.arch: 'amd64', os.version: '4.19.0-9-amd64', java.version: '1.8.0_91'
18:47:07 selenium_1   | Driver info: driver.version: unknown
18:47:07 koha_1       | 
18:47:07 koha_1       | STRACE:	/usr/share/perl5/Try/Tiny.pm:123 in Selenium::Remote::Driver::catch {...} 
18:47:07 koha_1       | 	/usr/local/share/perl/5.26.1/Selenium/Remote/Driver.pm:353 in Try::Tiny::try
18:47:07 koha_1       | 	(eval 1571):1 in Selenium::Remote::Driver::__ANON__
18:47:07 koha_1       | 	(eval 1573):2 in Selenium::Remote::Driver::__ANON__
18:47:07 koha_1       | 	(eval 1546):17 in Selenium::Remote::Driver::_execute_command
18:47:07 koha_1       | 	/usr/local/share/perl/5.26.1/Selenium/Remote/WebElement.pm:63 in Selenium::Remote::WebElement::_execute_command
18:47:07 koha_1       | 	/kohadevbox/koha/t/lib/Selenium.pm:184 in Selenium::Remote::WebElement::click
18:47:07 koha_1       | 	/kohadevbox/koha/t/lib/Selenium.pm:172 in t::lib::Selenium::click_when_visible
18:47:07 koha_1       | 	t/db_dependent/selenium/administration_tasks.t:131 in t::lib::Selenium::click
Comment 9 Jonathan Druart 2020-08-06 11:33:04 UTC
Again Master_D11 #57
Comment 10 Jonathan Druart 2020-08-06 13:08:42 UTC
Opened bug 26162
Comment 11 Lucas Gass 2020-08-13 18:18:07 UTC
backported to 20.05.x for 20.05.03
Comment 12 Aleisha Amohia 2020-08-16 21:54:42 UTC
backported to 19.11.x for 19.11.09
Comment 13 Victor Grousset/tuxayo 2020-09-20 12:14:37 UTC
Couldn't backport to 19.05.x
Conflicts weren't an issue but the last test fails.
If there is an interest in having this backported, feel free to submit a patch for 19.05.

kohadev-koha@b206c3f13d4a:/kohadevbox/koha$ prove t/Koha/REST/Plugin/Pagination.t t/db_dependent/Koha/REST/Plugin/Objects.t
t/Koha/REST/Plugin/Pagination.t ............ ok   
t/db_dependent/Koha/REST/Plugin/Objects.t .. 
    #   Failed test '_per_page=-1 means all resources'
    #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 266.
    #          got: '23'
    #     expected: '24'

    #   Failed test 'When _per_page=-1 the page param is not considered'
    #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 272.
    #          got: '23'
    #     expected: '24'
    # Looks like you failed 2 tests of 102.
t/db_dependent/Koha/REST/Plugin/Objects.t .. 1/2 
#   Failed test 'objects.search helper'
#   at t/db_dependent/Koha/REST/Plugin/Objects.t line 275.
# Looks like you failed 1 test of 2.
t/db_dependent/Koha/REST/Plugin/Objects.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/Objects.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=2, Tests=4,  4 wallclock secs ( 0.04 usr  0.02 sys +  3.18 cusr  0.41 csys =  3.65 CPU)
Result: FAIL