Summary: | Patron search generates URIs that are too long and can cause "HTTP 414 URI Too Long" errors | ||
---|---|---|---|
Product: | Koha | Reporter: | Julian Maurice <julian.maurice> |
Component: | Patrons | Assignee: | Julian Maurice <julian.maurice> |
Status: | Signed Off --- | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | david, dcook, fridolin.somers, gmcharlt, kyle.m.hall, lin-wei.li, m.de.rooy |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31104 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38745 |
||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 39637: Build the patron search query on the server side
Bug 39637: Fix tests by mocking appropriate sysprefs Bug 39637: Build the patron search query on the server side Bug 39637: Fix tests by mocking appropriate sysprefs |
Description
Julian Maurice
2025-04-15 13:35:49 UTC
(In reply to Julian Maurice from comment #0) > IMO it would be better to send only parameters like 'search', 'search_type' > and 'search_field' to the API and translate this to DBIC dialect on the > backend. This would make it a lot harder to reach the limit (and would make > the URI far more readable, which is always nice :)) +1 (In reply to David Cook from comment #1) > (In reply to Julian Maurice from comment #0) > > IMO it would be better to send only parameters like 'search', 'search_type' > > and 'search_field' to the API and translate this to DBIC dialect on the > > backend. This would make it a lot harder to reach the limit (and would make > > the URI far more readable, which is always nice :)) > > +1 Could be a good candidate for the growing idea for a search RPC endpoint. Created attachment 181005 [details] [review] Bug 39637: Build the patron search query on the server side Building the patron search query on the browser side causes the URI to grow rapidly and can reach the HTTP server limit, resulting in a 414 error (Request-URI Too Long) This patch moves the logic to the backend (more precisely, in Koha::REST::V1::Patrons) so that only user input needs to be sent as URL parameters. Test plan: 1. Log in to the staff interface 2. In the top search bar, select "Patrons search" and type "a b c d e f g h i j k l m n o p q r s t u v w x y z" and submit the form 3. The patrons table should load without errors (but will probably be empty) 4. Test the various filters on the left sidebar and verify that they work exactly as before the patch. 5. Modify system preferences DefaultPatronSearchFields and DefaultPatronSearchMethod and verify that they work exactly as before the patch. 6. If not done already, create searchable patron attributes and verify that search works for those too. 7. prove t/db_dependent/api/v1/patrons.t Looks like Apache limit is 8190. We face this issue more with Nginx has reverse proxy. "Sky is the limit" they say but not for here ;) The tests fail for me: prove t/db_dependent/api/v1/patrons.t t/db_dependent/api/v1/patrons.t .. # Failed test 'has no value for JSON Pointer "/0"' # at t/db_dependent/api/v1/patrons.t line 177. # Looks like you failed 1 test of 27. # Failed test 'search & search_type & search_fields' # at t/db_dependent/api/v1/patrons.t line 211. # Looks like you failed 1 test of 16. # Failed test 'librarian access tests' # at t/db_dependent/api/v1/patrons.t line 266. # Looks like you failed 1 test of 3. t/db_dependent/api/v1/patrons.t .. 1/8 # Failed test 'list() tests' # at t/db_dependent/api/v1/patrons.t line 320. t/db_dependent/api/v1/patrons.t .. 6/8 # Looks like you failed 1 test of 8. t/db_dependent/api/v1/patrons.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/8 subtests Test Summary Report ------------------- t/db_dependent/api/v1/patrons.t (Wstat: 256 (exited 1) Tests: 8 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=8, 11 wallclock secs ( 0.05 usr 0.01 sys + 8.54 cusr 1.78 csys = 10.38 CPU) Result: FAIL Testing notes: 1. After applying the patch, run: yarn build Created attachment 182264 [details] [review] Bug 39637: Fix tests by mocking appropriate sysprefs DefaultPatronSearchMethod and DefaultPatronSearchFields # Results before Applying Patch - Patron search with "a b c d e f g h i j k a a a a a a a a" fails with 414. - The URL that is expectedly too long. # Results after Applying Patch - Patron search with "a b c d e f g h i j k a a a a a a a a" fails with 400. - The URL is shortened as expected with "search=a b c d e f g h i j k a a a a a a a a". - Response is (raw JSON): [{"message":"Malformed query string","path":"\/query\/search_fields"},{"message":"Malformed query string","path":"\/query\/search_type"},{"message":"Malformed query string","path":"\/query\/search"}] - Tested patron search again with "a b c d e f g h i j k l m n o p q r s t u v w x y z". Fails also. # Test Results prove t/db_dependent/api/v1/patrons.t t/db_dependent/api/v1/patrons.t .. # No tests run! # Failed test 'No tests run for subtest "restricted & expired"' # at t/db_dependent/api/v1/patrons.t line 158. # Looks like you planned 16 tests but ran 14. # Looks like you failed 1 test of 14 run. # Failed test 'librarian access tests' # at t/db_dependent/api/v1/patrons.t line 275. # Looks like you planned 3 tests but ran 2. # Looks like you failed 1 test of 2 run. t/db_dependent/api/v1/patrons.t .. 1/8 # Failed test 'list() tests' # at t/db_dependent/api/v1/patrons.t line 329. Broken FK constraint # Failed test 'no warnings' # at /usr/local/share/perl/5.38.2/Test/Builder.pm line 187. # There were 1 warning(s) # Previous test 0 '' # DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_master_dev_linwei`.`borrower_debarments`, CONSTRAINT `borrower_debarments_ibfk_2` FOREIGN KEY (`type`) REFERENCES `restriction_types` (`code`) ON DELETE NO ACTION ON UPDATE CASCADE) at /inlibro/git/koha-master-dev-linwei/Koha/Object.pm line 174 # at /inlibro/git/koha-master-dev-linwei/Koha/Object.pm line 179. # Koha::Object::catch {...} (DBIx::Class::Exception=HASH(0x60b907ec7050)) called at /usr/share/perl5/Try/Tiny.pm line 123 # Try::Tiny::try(CODE(0x60b907dfa8d0), Try::Tiny::Catch=REF(0x60b908006fd8)) called at /inlibro/git/koha-master-dev-linwei/Koha/Object.pm line 225 # Koha::Object::store(Koha::Patron::Restriction=HASH(0x60b907e17f50)) called at /inlibro/git/koha-master-dev-linwei/Koha/Patron/Debarments.pm line 80 # Koha::Patron::Debarments::AddDebarment(HASH(0x60b9080368b8)) called at t/db_dependent/api/v1/patrons.t line 142 # main::__ANON__() called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # eval {...} called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # Test::Builder::subtest(Test::Builder=HASH(0x60b8fc0d7f10), "restricted & expired", CODE(0x60b907f8e7b8)) called at /usr/local/share/perl/5.38.2/Test/More.pm line 831 # Test::More::subtest("restricted & expired", CODE(0x60b907f8e7b8)) called at t/db_dependent/api/v1/patrons.t line 158 # main::__ANON__() called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # eval {...} called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # Test::Builder::subtest(Test::Builder=HASH(0x60b8fc0d7f10), "librarian access tests", CODE(0x60b907e80918)) called at /usr/local/share/perl/5.38.2/Test/More.pm line 831 # Test::More::subtest("librarian access tests", CODE(0x60b907e80918)) called at t/db_dependent/api/v1/patrons.t line 275 # main::__ANON__() called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # eval {...} called at /usr/local/share/perl/5.38.2/Test/Builder.pm line 368 # Test::Builder::subtest(Test::Builder=HASH(0x60b8fc0d7f10), "list() tests", CODE(0x60b907fbf120)) called at /usr/local/share/perl/5.38.2/Test/More.pm line 831 # Test::More::subtest("list() tests", CODE(0x60b907fbf120)) called at t/db_dependent/api/v1/patrons.t line 329 # # Looks like your test exited with 2 just after 2. t/db_dependent/api/v1/patrons.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 8/8 subtests Test Summary Report ------------------- t/db_dependent/api/v1/patrons.t (Wstat: 512 (exited 2) Tests: 2 Failed: 2) Failed tests: 1-2 Non-zero exit status: 2 Parse errors: Bad plan. You planned 8 tests but ran 2. Files=1, Tests=2, 3 wallclock secs ( 0.01 usr 0.00 sys + 2.56 cusr 0.10 csys = 2.67 CPU) Result: FAIL Created attachment 182282 [details] [review] Bug 39637: Build the patron search query on the server side Building the patron search query on the browser side causes the URI to grow rapidly and can reach the HTTP server limit, resulting in a 414 error (Request-URI Too Long) This patch moves the logic to the backend (more precisely, in Koha::REST::V1::Patrons) so that only user input needs to be sent as URL parameters. Test plan: 1. Log in to the staff interface 2. In the top search bar, select "Patrons search" and type "a b c d e f g h i j k l m n o p q r s t u v w x y z" and submit the form 3. The patrons table should load without errors (but will probably be empty) 4. Test the various filters on the left sidebar and verify that they work exactly as before the patch. 5. Modify system preferences DefaultPatronSearchFields and DefaultPatronSearchMethod and verify that they work exactly as before the patch. 6. If not done already, create searchable patron attributes and verify that search works for those too. 7. prove t/db_dependent/api/v1/patrons.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 182283 [details] [review] Bug 39637: Fix tests by mocking appropriate sysprefs DefaultPatronSearchMethod and DefaultPatronSearchFields Signed-off-by: David Nind <david@davidnind.com> Thanks for testing Lin Wei and your testing notes! A yarn build and a restart-all (in KTD) is required for things to work correctly. This test still seems to have an issue still unrelated to this patch. Should something similar be done for js below ? intranet-tmpl/prog/js/ill-list-table.js: let patronquery = buildPatronSearchQuery(patron, { intranet-tmpl/prog/js/ill-list-table.js: let patronquery = buildPatronSearchQuery(keyword, { intranet-tmpl/prog/js/modals/display_ticket.js: let q = buildPatronSearchQuery(params.term); intranet-tmpl/prog/js/modals/place_booking.js: let q = buildPatronSearchQuery(params.term); intranet-tmpl/prog/js/staff-global.js: let q = buildPatronSearchQuery(request.term); intranet-tmpl/prog/js/staff-global.js:function buildPatronSearchQuery(term, options) { FAIL t/db_dependent/api/v1/patrons.t FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/api/v1/patrons.t` (In reply to Marcel de Rooy from comment #12) > Should something similar be done for js below ? I would say yes, if this patch is approved. It can be done in followup bug reports IMO |