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 Created attachment 183273 [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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 183274 [details] [review] 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> Sorry, tests are failing for me with this patchset applied: prove -v /kohadevbox/koha/t/db_dependent/api/v1/patrons.t Did you run `yarn api:bundle` first ? If the spec is not up to date, tests will fail. 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. (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`. (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 ? > 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 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 (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 (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" |