[Bug 39637] New: Patron search generates URIs that are too long and can cause "HTTP 414 URI Too Long" errors
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Bug ID: 39637 Summary: Patron search generates URIs that are too long and can cause "HTTP 414 URI Too Long" errors Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Continuation of bug 31104 which did not solve the problem completely. Steps to reproduce: 1. Log in to 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" and submit the form 3. The patrons table should fail to load with a message like this: "Something went wrong when loading the table. 414: Request-URI Too Long.". If the table loads successfully, try adding a few more letters (separated by space) to the search. Another error that can happen is a 5XX error caused by the length of Link headers. Here is the (decoded) URL generated for "a b c d e": http://koha-dev-main/api/v1/patrons?_page=1&_per_page=20&q=[{"-and":[[{"me.firstname":{"like":"a%"}},{"me.preferred_name":{"like":"a%"}},{"me.middle_name":{"like":"a%"}},{"me.surname":{"like":"a%"}},{"me.othernames":{"like":"a%"}},{"me.cardnumber":{"like":"a%"}},{"me.userid":{"like":"a%"}}],[{"me.firstname":{"like":"b%"}},{"me.preferred_name":{"like":"b%"}},{"me.middle_name":{"like":"b%"}},{"me.surname":{"like":"b%"}},{"me.othernames":{"like":"b%"}},{"me.cardnumber":{"like":"b%"}},{"me.userid":{"like":"b%"}}],[{"me.firstname":{"like":"c%"}},{"me.preferred_name":{"like":"c%"}},{"me.middle_name":{"like":"c%"}},{"me.surname":{"like":"c%"}},{"me.othernames":{"like":"c%"}},{"me.cardnumber":{"like":"c%"}},{"me.userid":{"like":"c%"}}],[{"me.firstname":{"like":"d%"}},{"me.preferred_name":{"like":"d%"}},{"me.middle_name":{"like":"d%"}},{"me.surname":{"like":"d%"}},{"me.othernames":{"like":"d%"}},{"me.cardnumber":{"like":"d%"}},{"me.userid":{"like":"d%"}}],[{"me.firstname":{"like":"e%"}},{"me.preferred_name":{"like":"e%"}},{"me.middle_name":{"like":"e%"}},{"me.surname":{"like":"e%"}},{"me.othernames":{"like":"e%"}},{"me.cardnumber":{"like":"e%"}},{"me.userid":{"like":"e%"}}]]},{"-or":[{"me.firstname":{"like":"a+b+c+d+e%"}},{"me.preferred_name":{"like":"a+b+c+d+e%"}},{"me.middle_name":{"like":"a+b+c+d+e%"}},{"me.surname":{"like":"a+b+c+d+e%"}},{"me.othernames":{"like":"a+b+c+d+e%"}},{"me.cardnumber":{"like":"a+b+c+d+e%"}},{"me.userid":{"like":"a+b+c+d+e%"}}]},{"-and":[[{"extended_attributes.value":{"like":"a%"},"extended_attributes.code":[["Nickname","PAT_1"]]}],[{"extended_attributes.value":{"like":"b%"},"extended_attributes.code":[["Nickname","PAT_1"]]}],[{"extended_attributes.value":{"like":"c%"},"extended_attributes.code":[["Nickname","PAT_1"]]}],[{"extended_attributes.value":{"like":"d%"},"extended_attributes.code":[["Nickname","PAT_1"]]}],[{"extended_attributes.value":{"like":"e%"},"extended_attributes.code":[["Nickname","PAT_1"]]}]]}]&_match=contains&_order_by=+me.surname,+me.preferred_name,+me.firstname,+me.middle_name,+me.othernames,+me.street_number,+me.address,+me.address2,+me.city,+me.state,+me.postal_code,+me.country (Ignore the extended_attributes.* params that are not the main issue here, even though they contribute to the problem) If I'm counting correctly, for each word searched, the URL length is increased by (216 + 7 * <word length>) characters (before encoding, which also increases the URI length by a lot). For me, with Apache, the problem happens around 8k characters (after encoding). 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 :)) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- (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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38745 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 181005 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181005&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #4 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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 ;) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Failed QA Assignee|koha-bugs@lists.koha-commun |julian.maurice@biblibre.com |ity.org | --- Comment #5 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #6 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 182264 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182264&action=edit Bug 39637: Fix tests by mocking appropriate sysprefs DefaultPatronSearchMethod and DefaultPatronSearchFields -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Lin Wei <lin-wei.li@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lin-wei.li@inLibro.com Status|Needs Signoff |Failed QA --- Comment #7 from Lin Wei <lin-wei.li@inLibro.com> --- # 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #181005|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 182282 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182282&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182264|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 182283 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182283&action=edit Bug 39637: Fix tests by mocking appropriate sysprefs DefaultPatronSearchMethod and DefaultPatronSearchFields Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #10 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- This test still seems to have an issue still unrelated to this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- (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
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182282|0 |1 is obsolete| | --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 183273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183273&action=edit 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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182283|0 |1 is obsolete| | --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 183274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183274&action=edit Bug 39637: Fix tests by mocking appropriate sysprefs DefaultPatronSearchMethod and DefaultPatronSearchFields Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Passed QA |Failed QA --- Comment #17 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Sorry, tests are failing for me with this patchset applied: prove -v /kohadevbox/koha/t/db_dependent/api/v1/patrons.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- Did you run `yarn api:bundle` first ? If the spec is not up to date, tests will fail. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Patrons |REST API CC| |jonathan.druart@gmail.com, | |martin.renvoize@openfifth.c | |o.uk, tomascohen@gmail.com --- Comment #19 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi, I just noticed this attempt to make a big change on the API architecture. Lucas brought it to me. Adding Jonathan and Martin to the conversation as well. I saw several reports about the same and I agree with the basics of this implementation: i.e. a query parameter for telling what to search, and another one for where. What we lack here is: * The query builder should be a helper in `Koha::REST::Plugin::Query` * Retrieving the parameters and building a query (with the helper) should be backed into `objects.search`. * The JS part should be done in datatables.js so we fix all tables the same way. The patrons one has a lot of fields to query on because of the extended attributes, but so the ILL one, etc. * I would add an option parameter to KohaTable, something like `compact_search: true` to choose which kind of search to build. This way we can migrate the tables one by one, taking care of their specifics. I'm willing to help here. My two cents. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #20 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #19)
* Retrieving the parameters and building a query (with the helper) should be backed into `objects.search`.
I meant `objects.search_rs`. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #21 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #19)
Hi, I just noticed this attempt to make a big change on the API architecture. Lucas brought it to me. Adding Jonathan and Martin to the conversation as well.
This is not that big :) Just a few additional parameters to repair what was broken when we switched from svc/members/search to the REST API.
I saw several reports about the same and I agree with the basics of this implementation: i.e. a query parameter for telling what to search, and another one for where.
Is this actually needed elsewhere ?
What we lack here is:
* The query builder should be a helper in `Koha::REST::Plugin::Query` * Retrieving the parameters and building a query (with the helper) should be backed into `objects.search`. * The JS part should be done in datatables.js so we fix all tables the same way. The patrons one has a lot of fields to query on because of the extended attributes, but so the ILL one, etc. * I would add an option parameter to KohaTable, something like `compact_search: true` to choose which kind of search to build. This way we can migrate the tables one by one, taking care of their specifics.
I'm willing to help here.
This looks like trying to generalize too much too soon. Is there another API endpoint that can benefit from this right now ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #22 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> ---
Is this actually needed elsewhere ?
I've ran into this error on both of these bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38967 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36135 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- For bug 38967, whatever is done on the backend, we still need to send all those itemnumbers, right ? I don't see how Tomás suggestion can prevent long URLs. In that particular case it would be better to send data in the request body IMO -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #24 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Julian Maurice from comment #23)
For bug 38967, whatever is done on the backend, we still need to send all those itemnumbers, right ? I don't see how Tomás suggestion can prevent long URLs. In that particular case it would be better to send data in the request body IMO
Sending the query in the request body would solve this one too. I haven't proposed it because I was unsure about it's support across browsers and proxy software. But we implemented it for that specific case when `q=` was introduced. We also had a `x-koha-query` header we ditched because of this kind of limitations. And I'm sure we are going to remove the `Link` headers soon too, because sometimes the query is not that big, but we repeat it 3 times in the header :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #25 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #24)
Sending the query in the request body would solve this one too. True, but... I guess I simply dislike the fact that we have to send a DBIC query to the API for simple cases like that :)
I haven't proposed it because I was unsure about it's support across browsers and proxy software. Do you mean sending a body with a GET request ? Then I agree, we shouldn't do that. That would be asking for problems. For bug 38967 I meant "send itemnumbers in body and change the method to POST"
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED --- Comment #26 from Julian Maurice <julian.maurice@biblibre.com> --- Back to ASSIGNED This breaks patron search in acquisitions, suggestions, and erm modules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |blawlor@clamsnet.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183273|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183274|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #27 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 199670 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199670&action=edit 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::Plugin::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. Verify that patrons search works everywhere else: - Acquisition baskets (managers) - Acquisition orders (users) - Acquisition funds (owners/users) - Article requests - ERM - Patrons creation/modification (guarantor) - Patron cards - Serials routing lists - Suggestions 8. prove t/db_dependent/api/v1/acquisitions_baskets.t prove t/db_dependent/api/v1/acquisitions_funds.t prove t/db_dependent/api/v1/erm_users.t prove t/db_dependent/api/v1/patrons.t prove t/db_dependent/api/v1/suggestions.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #28 from Julian Maurice <julian.maurice@biblibre.com> --- This new patch fixes patron search everywhere it's used. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #29 from Jonathan Druart <jonathan.druart@gmail.com> --- Why not those occurrences? koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js: let patronquery = buildPatronSearchQuery(patron, { koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js: let managed_byquery = buildPatronSearchQuery(managed_by, { koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js: let patronquery = buildPatronSearchQuery(keyword, { koha-tmpl/intranet-tmpl/prog/js/modals/display_ticket.js: let q = buildPatronSearchQuery(params.term); koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js: let q = buildPatronSearchQuery(params.term); To finally remove the JS code (and way more JS code that this function: expandPatronSearchFields, defaultPatronSearchFields, defaultPatronSearchMethod): koha-tmpl/intranet-tmpl/prog/js/staff-global.js: let q = buildPatronSearchQuery(request.term); koha-tmpl/intranet-tmpl/prog/js/staff-global.js:function buildPatronSearchQuery(term, options) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #30 from Julian Maurice <julian.maurice@biblibre.com> --- I was focused on places where the 'patron_search_js' TT block is used... :/ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #199670|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #31 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 199728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199728&action=edit 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::Plugin::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. Verify that patrons search works everywhere else: - Acquisition baskets (managers) - Acquisition orders (users) - Acquisition funds (owners/users) - Article requests - ERM - Patrons creation/modification (guarantor) - Patron cards - Serials routing lists - Suggestions 8. prove t/db_dependent/api/v1/acquisitions_baskets.t prove t/db_dependent/api/v1/acquisitions_funds.t prove t/db_dependent/api/v1/erm_users.t prove t/db_dependent/api/v1/patrons.t prove t/db_dependent/api/v1/suggestions.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #32 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 199729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199729&action=edit Bug 39637: Remove use of buildPatronSearchQuery in modals Patch from commit 24dc25b -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #33 from Julian Maurice <julian.maurice@biblibre.com> --- The remaining occurrences in ill-list-table.js cannot be removed so easily, as the result of buildPatronSearchQuery is used as a subquery. Can we do this in a followup bug ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 --- Comment #34 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Julian Maurice from comment #33)
The remaining occurrences in ill-list-table.js cannot be removed so easily, as the result of buildPatronSearchQuery is used as a subquery.
Can we do this in a followup bug ?
Yes, I guess so. However I think it should be done, it's not good to keep both (perl and js) code doing the same thing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39637 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42508 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org