Bug 1707 - Search for patron '%' returns all results
Summary: Search for patron '%' returns all results
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords: Academy
Depends on:
Blocks: 6253
  Show dependency treegraph
 
Reported: 2007-12-26 02:08 UTC by Chris Cormack
Modified: 2022-02-09 09:39 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 1707: Do not return more than 100 patrons matching the attributes search (2.54 KB, patch)
2016-11-02 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 1707: Do not return more than 100 patrons matching the attributes search (2.58 KB, patch)
2017-04-18 15:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 1707: Do not return more than 100 patrons matching the attributes search (2.71 KB, patch)
2017-04-18 19:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 1707 (QA Followup) Add a warning that attribute searches will be truncated (1.08 KB, patch)
2017-05-10 11:05 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:33:40 UTC


---- Reported by drdrulm@yahoo.com 2007-12-26 14:08:47 ----

Intranet: If the request.pl?biblionumber= hold reserve request screen is given a patron of the character % this pulls in all patrons and crashes mozilla, this may be a problem elsewhere where patrons can be searched or in other conditions where too many patrons are pulled into the selection widget



---- Additional Comments From oleonard@myacpl.org 2007-12-31 17:28:16 ----

I guess the crux of the problem is that searching for patrons by inputting '%' brings up all results.



---- Additional Comments From jmf@liblime.com 2007-12-31 18:42:28 ----

The solution here, as with the solution to an identical problem that happens with the circulation search, is to allow paged results of patrons on the request.pl and circulation.pl patron searches. This has been done with the standard patron search  (member.pl), and that search should be ported to circ and reserve. Assigning to Chris.



---- Additional Comments From oleonard@myacpl.org 2008-01-01 05:35:34 ----

Is it really necessary to allow the user to search using '%' in these contexts? Someone looking for a list of all patrons could use reports.



---- Additional Comments From jmf@liblime.com 2008-03-17 16:12:52 ----

I propose that we use this opportunity to create a generic patron search API that both circulation and patron searches use, and it should feature paged search results, like in the patron search. The UI for patron and circulation searches should likewise be merged, for consistency sake. Re-assigning to Galen for now as this touches circulation and patrons.



---- Additional Comments From nengard@gmail.com 2009-12-23 17:58:42 ----

This bug hasn't been touched in nearly a year, please revisit and test and close if appropriate.



---- Additional Comments From oleonard@myacpl.org 2009-12-25 19:14:43 ----

This problem is mitigated now that patron searches from members-home.pl return paged results, but still exists when searching via the circulation search form.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:33 UTC  ---

This bug was previously known as _bug_ 1707 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=1707

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was drdrulm@yahoo.com.

Comment 1 Chris Cormack 2012-10-25 23:28:26 UTC
This is still a valid bug.
Comment 2 Katrin Fischer 2015-02-12 10:17:00 UTC
Is this still a problem with the new server side processing of the patron search table?
Comment 3 Petter Goksøyr Åsen 2016-10-13 08:30:53 UTC
This is still a problem. I'd say the member search is dangerous and generates some horrible queries which takes many minutes to run if you have a large dataset.

We have over 300,000 patrons.

If someone search for something stupid like %29% - this is (part of) the generated query:
https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql
Comment 4 Jonathan Druart 2016-10-18 08:17:27 UTC
(In reply to Petter Goksøyr Åsen from comment #3)
> This is still a problem. I'd say the member search is dangerous and
> generates some horrible queries which takes many minutes to run if you have
> a large dataset.
> 
> We have over 300,000 patrons.
> 
> If someone search for something stupid like %29% - this is (part of) the
> generated query:
> https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/
> raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql

How d(In reply to Petter Goksøyr Åsen from comment #3)
> This is still a problem. I'd say the member search is dangerous and
> generates some horrible queries which takes many minutes to run if you have
> a large dataset.
> 
> We have over 300,000 patrons.
> 
> If someone search for something stupid like %29% - this is (part of) the
> generated query:
> https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/
> raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql

For the records: http://irc.koha-community.org/koha/2016-10-13#i_1866950

Petter, how did you finally fix it?
I have several ways to fix it but I don't figure out which one is the best.
Comment 5 Petter Goksøyr Åsen 2016-10-18 12:31:55 UTC
I have put a limit on the query as you suggested:
http://paste.koha-community.org/135 
It's not in production yet, but testing in staging so far seems good.

The result is probably not accurate, as you said, but I think it's only potentially wrong if you have a very wide search (i.e few numbers which matches lots of borrowernumbers). If the result list is long, say tens of thousands, it does not matter that its not accurate.

If you search for full fields (in our case its social security number as extended patron attribute), the returned result is always correct

In my mind, it's quite dangerous that Koha allows to generate SQL-queries which are several MB of text, which generated huge loads on our server, so I'd prefer safety to accuracy here.
Comment 6 Jonathan Druart 2016-11-02 09:22:50 UTC
Created attachment 57068 [details] [review]
Bug 1707: Do not return more than 100 patrons matching the attributes search

As you can feel, this patch is not perfect as we are hardcodind the
number of results to return.
But this number should be correct in the most cases.

Test plan:
With a large number of patrons, using attributes, you can search for
something very generic, like '%'.
Without this patch, the MySQL server could be overloaded because of the
generated query (built from borrowernumbers).
Comment 7 Mark Tompsett 2017-01-16 04:49:25 UTC
Comment on attachment 57068 [details] [review]
Bug 1707: Do not return more than 100 patrons matching the attributes search

Review of attachment 57068 [details] [review]:
-----------------------------------------------------------------

::: C4/Members/Attributes.pm
@@ +152,4 @@
>  JOIN borrower_attribute_types USING (code)
>  WHERE staff_searchable = 1
>  AND (} . join (" OR ", map "attribute like ?", @$filter) .qq{)};
> +    $query .= q{ LIMIT ?} if $limit;

LIMIT is a MySQLism.
Perhaps DBIx would be a good way to get around this?
Not that I know how to do it, but this looks like a good place to start:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/ResultSet.pm#rows
Comment 8 Jonathan Druart 2017-01-16 08:58:09 UTC
(In reply to M. Tompsett from comment #7)
> LIMIT is a MySQLism.

Hum? Ref needed please.
https://www.postgresql.org/docs/current/static/queries-limit.html
Comment 9 Owen Leonard 2017-04-18 14:49:08 UTC
(In reply to M. Tompsett from comment #7)
> LIMIT is a MySQLism.

If this page is accurate, LIMIT is not standard but is not unique to MySQL:

http://troels.arvin.dk/db/rdbms/#select-limit

IMO there is a LIMIT to how much database-agnosticism we should worry about.
Comment 10 Jonathan Druart 2017-04-18 15:39:50 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2017-04-18 19:11:13 UTC
Created attachment 62324 [details] [review]
Bug 1707: Do not return more than 100 patrons matching the attributes search

As you can feel, this patch is not perfect as we are hardcodind the
number of results to return.
But this number should be correct in the most cases.

Test plan:
With a large number of patrons, using attributes, you can search for
something very generic, like '%'.
Without this patch, the MySQL server could be overloaded because of the
generated query (built from borrowernumbers).

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Nick Clemens 2017-04-25 11:00:30 UTC
I think this fixes a problem, but I'd really like to see a notification/error pushed back to the user letting them know the search was truncated and suggesting a more exact search

This feels similar to bug 15812
Comment 13 Jonathan Druart 2017-05-10 10:47:52 UTC
We cannot really know the search has been truncated.
Comment 14 Nick Clemens 2017-05-10 11:05:50 UTC
Created attachment 63335 [details] [review]
Bug 1707 (QA Followup) Add a warning that attribute searches will be truncated
Comment 15 Nick Clemens 2017-05-10 11:06:39 UTC
(In reply to Jonathan Druart from comment #13)
> We cannot really know the search has been truncated.

True, but in that case we can just always warn - if we are going to have a hardcoded limit it just needs to be announced
Comment 16 Jonathan Druart 2017-05-10 11:32:38 UTC
(In reply to Nick Clemens from comment #15)
> (In reply to Jonathan Druart from comment #13)
> > We cannot really know the search has been truncated.
> 
> True, but in that case we can just always warn - if we are going to have a
> hardcoded limit it just needs to be announced

Yes but the note is wrong, and will be hard to make it correct :)

Note you added says "Note: Patron attribute searching is enabled, to avoid memory issues a limit of 100 patrons will be returned during "standard" searches"

If you are searching for a pattern that is used 250x in patron attribute, and 100x in other standard fields, we will certainly get less 100 results even if the whole set would have been 100.