Summary: | Fatal error when searching from addbooks.pl due to ISBN flaw | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Searching | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | RESOLVED DUPLICATE | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | jonathan.druart, joonas.kylmala, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29437 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 17979: Fatal error when searching from addbooks.pl due to ISBN flaw
Bug 17979: Fix cataloguing search if ISBN 13 is passed Bug 17979: Fix cataloguing search if ISBN 13 is passed Bug 17979: Improve SQL query build |
Description
David Cook
2017-01-24 04:30:08 UTC
I guess the regex should be m|^[\d-]*$|; Instead of just /\d/ (In reply to Jonathan Druart from comment #1) > I guess the regex should be m|^[\d-]*$|; > Instead of just /\d/ That doesn't account for ISBN-10 where the check digit can be X or x: https://en.wikipedia.org/wiki/Check_digit#ISBN_10 We could use Business::ISBN to check if it's an ISBN and then if it's not, we could treat it as a title. (We could even do ISBN search only for valid ISBNs and do title searches for everything else.) Created attachment 95673 [details] [review] Bug 17979: Fatal error when searching from addbooks.pl due to ISBN flaw This patch adds true ISBN detection to cataloging searches so that ISBNs can be correctly identified and passed to queries of the reservoir. To test, apply the patch and go to Cataloging. - In the "Cataloging search" form at the top of the page, test various searches to confirm that results are returned in both the catalog and the reservoir: - Any title search - A search for a title which begins with a numeral, e.g. "1st to die" - A search by 10-digit ISBN - A search by 13-digit ISBN Hi Owen, why not search the ISBN as is? Or in different formats according to our ISBN variations prefs? I am not sure about the line using the as_isbn10. + if( $isbn = Business::ISBN->new( $query ) ) { + $isbn = $isbn->as_isbn10->as_string; + $isbn =~ s/-//g; As I understand it, if the ISBN is a new one (13), this will return undef. as_isbn10 Returns a new ISBN object. If the object is already ISBN-10, this method clones it. If it is an ISBN-13 with the prefix 978, it returns the ISBN-10 equivalent. For all other cases it returns undef. I was going off existing code, so perhaps I misunderstood that other instance. *** Bug 27008 has been marked as a duplicate of this bug. *** Created attachment 119373 [details] [review] Bug 17979: Fix cataloguing search if ISBN 13 is passed This patch fixes the following error: C4::Breeding::BreedingSearch(): DBI Exception: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use ne ar '' at line 5 [for Statement "SELECT import_record_id, file_name, isbn, title, author FROM import_biblios JOIN import_records USING (import_record_id) JOIN import_batches USING (import_batch_id) WHERE "] at /kohadevbox/koha/cataloguing/addbooks.pl line 126 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 It happens when our GetNormalizedISBN sub does not return an ISBN, the SQL query generated by BreedingSearch is invalid Test plan: Try a cataloguing search with "400638133393x" Trying a fix but not sure it's correct however. Created attachment 119374 [details] [review] Bug 17979: Fix cataloguing search if ISBN 13 is passed This patch fixes the following error: C4::Breeding::BreedingSearch(): DBI Exception: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 5 [for Statement "SELECT import_record_id, file_name, isbn, title, author FROM import_biblios JOIN import_records USING (import_record_id) JOIN import_batches USING (import_batch_id) WHERE "] at /kohadevbox/koha/cataloguing/addbooks.pl line 126 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 It happens when our GetNormalizedISBN sub does not return an ISBN, the SQL query generated by BreedingSearch is invalid Test plan: Try a cataloguing search with "400638133393x" Signed-off-by: Owen Leonard <oleonard@myacpl.org> Tested with the suggested invalid ISBN as well as with some correct ISBNS. No errors. Would be good if the function didn't error on invalid isbn or title at all, the reproducer from bug 27008 still gives the error with this patch applied: > perl -e 'use C4::Context; use C4::Breeding; C4::Breeding::BreedingSearch();' Created attachment 119448 [details] [review] Bug 17979: Improve SQL query build Take also into account an hypotetical call to BreedingSearch() without parameters This certainly improves things, however, the initial problem reported still exists - a search for '1st to die' searches the isbn field (In reply to Nick Clemens from comment #12) > This certainly improves things, however, the initial problem reported still > exists - a search for '1st to die' searches the isbn field The internal server problem here reported initially is gone but maybe you mean that the search result still doesn't make any sense here? If we look at the code the WHERE clause is buggy still. With these patches applied and with the search string '1st to die' we will look for biblios with title '1st to die' OR biblios with author and isbn both being '1st to die' so that doesn't sound like it will provide very good results. The first patch here, Bug 17979: Fix cataloguing search if ISBN 13 is passed should be obsoleted I think. However, still searching for an author whose name is ISBN code makes this fail. For the AND and OR precedence see for example: https://dev.mysql.com/doc/refman/8.0/en/operator-precedence.html It would probably make sense to split this to two functions: one for title search and another for isbn? Makes also unit tests easier, less cases to iterate. (In reply to Joonas Kylmälä from comment #14) > It would probably make sense to split this to two functions: one for title > search and another for isbn? Makes also unit tests easier, less cases to > iterate. I mean if you look how this is called in addbooks.pl we only search one or another at all times. So it definitely makes sense to split this. (In reply to Joonas Kylmälä from comment #14) > It would probably make sense to split this to two functions: one for title > search and another for isbn? Makes also unit tests easier, less cases to > iterate. +1 I believe the patches on bug 29437 resolve the issue here (if a slightly different implementation) *** This bug has been marked as a duplicate of bug 29437 *** (In reply to Nick Clemens from comment #17) > I believe the patches on bug 29437 resolve the issue here (if a slightly > different implementation) > > *** This bug has been marked as a duplicate of bug 29437 *** Not the second patch. (In reply to Jonathan Druart from comment #18) > (In reply to Nick Clemens from comment #17) > > I believe the patches on bug 29437 resolve the issue here (if a slightly > > different implementation) > > > > *** This bug has been marked as a duplicate of bug 29437 *** > > Not the second patch. Doesn't it? The second patch here pushed the conditions to an array depending on whether we think we have an ISBN or not Bug 29437 removes the attempt to determine which kind of query we have and instead uses 'OR' and builds the same condition each time. As the overlap between title/author and ISBN is very small I think one condition that searches all the fields resolves the issue I've applied the test from the second patch and it's failing. See bug 27008. |