Bug 8350 - Searching for a 10 or 13 digit string (with a number in it) that does not exist in one's catalog will fail to affect SQL statement correctly in C4::Breeding
Summary: Searching for a 10 or 13 digit string (with a number in it) that does not exi...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact:
URL:
Keywords:
: 8349 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-07-03 12:18 UTC by Mark Tompsett
Modified: 2013-05-23 06:23 UTC (History)
3 users (show)

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


Attachments
Fixed string substitution bug (8349) and better SQL building (1.51 KB, patch)
2012-07-03 12:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. (1.39 KB, patch)
2012-08-30 10:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
(3.8.x) Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. (1.39 KB, patch)
2012-08-30 10:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
(master) Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. (1.39 KB, patch)
2012-08-30 10:22 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8350: warning in logs when searching for nonexistent ISBN (1.49 KB, patch)
2012-08-30 11:40 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2012-07-03 12:18:11 UTC
It is possible that a title or author could be '0', and thus the simple if ($search) will fail. By changing it to defined 0, '0', and '' are included. However, this would mess up the SQL include a pointless 'like %%'. So, add in a length check and all should be good.

($search) becomes (defined($search) && length($search)>0)
Similar logic needs to apply to the isbn number too.
Comment 1 Mark Tompsett 2012-07-03 12:22:17 UTC
*** Bug 8349 has been marked as a duplicate of this bug. ***
Comment 2 Mark Tompsett 2012-07-03 12:30:23 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2012-08-06 15:02:37 UTC
Patch looks ok, but I'm not able to reproduce a bug.
git grep tells me that the unique place where BreedingSearch is used is cataloguing/addbooks.pl.
In cataloguing/addbooks.pl, if I search '0' or 0, BreedingSearch is not called, because of "if($query)" in script.
If you want to allow searching for string '0', I think you should modify addbooks.pl as well.
Comment 4 Mark Tompsett 2012-08-14 02:19:13 UTC
Sadly, I don't know how to reproduce it either. Though, perhaps:
"HTTP 500 error from koha server while importing Z39.50records" on the mailing list will give clues?
Comment 5 Mark Tompsett 2012-08-30 04:18:32 UTC
I was able to trigger this by:
- login
- "Cataloguing" (goes to addbooks.pl)
- Do a cataloguing search for "98765432310" (shouldn't exist in the library)
- check error logs for my 3.6.3:
[Thu Aug 30 12:03:05 2012] [error] [client 192.168.100.2] [Thu Aug 30 12:03:05 2012] addbooks.pl: Use of uninitialized value $search in substitution (s///) at /usr/share/koha/lib/C4/Breeding.pm line 188., referer: https://.../cgi-bin/koha/cataloguing/addbooks.pl
Comment 6 Mark Tompsett 2012-08-30 04:41:10 UTC
Put in patching equivalent to my 3.6.3 system. No errors!

However, fixing Breeding.pm may not be the only problem. I noticed, while trying to think through the possibilities that there are issues in addbooks.pl
The BreedingSearch call has either title=undef or isbn=undef. Is this a reasonable expectation?

Additionally, BreedingSearch has three parameters: search, isbn, z3950random. By calling with only two parameters, z3950 is always undef. This means that the "else" always runs. And as Julian Maurice pointed out, there is only one program currently using the BreedingSearch.

Perhaps an optimization of the function needs to be made too? I was thinking of a substitution for a different function, but I'm not so sure of that. Someone else more familiar with the addbooks.pl processing would need to make that call.
Comment 7 Mark Tompsett 2012-08-30 10:18:12 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2012-08-30 10:20:35 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2012-08-30 10:22:16 UTC Comment hidden (obsolete)
Comment 10 Jared Camins-Esakov 2012-08-30 11:40:47 UTC
Created attachment 11891 [details] [review]
Bug 8350: warning in logs when searching for nonexistent ISBN

Searching for a 10 or 13 digit numeric string that does not exist in
one's catalog will fail to affect SQL statement correctly
in C4::Breeding. Moved string substitution, which was triggering error
when search was undefined, and fixed if statements accordingly.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 11 Paul Poulain 2012-10-02 15:17:37 UTC
QA comment:
  * small patch, tiny side effect risk
  * passes koha-qa.pl

fixes the error & cataloguing still work after patch

passed QA
Comment 12 Paul Poulain 2012-10-02 15:17:58 UTC
Patch pushed to master
Comment 13 Chris Cormack 2012-10-03 19:16:23 UTC
Pushed to 3.8.x will be in 3.8.6