Summary: | The API does not handle requesting all resources | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | REST API | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | agustinmoyano, aleisha, kyle, lucas, martin.renvoize, nick, victor |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
20.11.00, 20.05.03, 19.11.09
|
Circulation function: | |
Bug Depends on: | 25570 | ||
Bug Blocks: | 26162 | ||
Attachments: |
Bug 26143: The API is always paginated
Bug 26143: Regression tests Bug 26143: Make the API handle per_page=-1 Bug 26143: The API is always paginated Bug 26143: Regression tests Bug 26143: Make the API handle per_page=-1 |
Description
Jonathan Druart
2020-08-05 11:03:51 UTC
Created attachment 107836 [details] [review] Bug 26143: The API is always paginated Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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> 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> Pushed to master for 20.11, thanks to everybody involved! 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 Again Master_D11 #57 Opened bug 26162 backported to 20.05.x for 20.05.03 backported to 19.11.x for 19.11.09 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 |