Bug 17979

Summary: Fatal error when searching from addbooks.pl due to ISBN flaw
Product: Koha Reporter: David Cook <dcook>
Component: SearchingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: jonathan.druart, joonas.kylmala, katrin.fischer, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29437
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
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
If you're doing a "Cataloging Search" from /cgi-bin/koha/cataloguing/addbooks.pl, and you type in "1st to die", you'll see the following:

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 at /<koha>/lib/C4/Breeding.pm line 107

The reason is that the string is 10 characters long and contains a digit, so Koha thinks it's an ISBN. But when it tries to normalize it, it fails, and instead Koha generates no data for the WHERE clause and then the SQL statement fails because it contains a WHERE without the actual clause.


The code:

if ($query) {
# fill isbn or title, depending on what has been entered
#u must do check on isbn because u can find number in beginning of title
#check is on isbn legnth 13 for new isbn and 10 for old isbn
    my ( $title, $isbn );
    if ($query=~/\d/) {
        my $clean_query = $query;
        $clean_query =~ s/-//g; # remove hyphens
        my $querylength = length $clean_query;
        if ( $querylength == 13 || $querylength == 10 ) {
            $isbn = $query;
        }
    }
    if (!$isbn) {
        $title = $query;
    }
    ( $countbr, @resultsbr ) = BreedingSearch( $title, $isbn );
}
Comment 1 Jonathan Druart 2017-01-24 14:35:41 UTC
I guess the regex should be m|^[\d-]*$|;
Instead of just /\d/
Comment 2 David Cook 2017-01-24 22:45:42 UTC
(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.)
Comment 3 Owen Leonard 2019-11-21 16:10:25 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2019-11-22 22:45:39 UTC
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.
Comment 5 Owen Leonard 2019-11-26 19:02:27 UTC
I was going off existing code, so perhaps I misunderstood that other instance.
Comment 6 Jonathan Druart 2021-04-09 12:18:49 UTC
*** Bug 27008 has been marked as a duplicate of this bug. ***
Comment 7 Jonathan Druart 2021-04-09 12:21:36 UTC
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"
Comment 8 Jonathan Druart 2021-04-09 12:22:24 UTC
Trying a fix but not sure it's correct however.
Comment 9 Owen Leonard 2021-04-09 12:31:48 UTC
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.
Comment 10 Joonas Kylmälä 2021-04-09 14:45:02 UTC
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();'
Comment 11 Jonathan Druart 2021-04-12 10:22:08 UTC
Created attachment 119448 [details] [review]
Bug 17979: Improve SQL query build

Take also into account an hypotetical call to BreedingSearch() without
parameters
Comment 12 Nick Clemens 2021-04-20 18:43:41 UTC
This certainly improves things, however, the initial problem reported still exists - a search for '1st to die' searches the isbn field
Comment 13 Joonas Kylmälä 2021-04-21 10:38:51 UTC
(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
Comment 14 Joonas Kylmälä 2021-04-21 10:43:18 UTC
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.
Comment 15 Joonas Kylmälä 2021-04-21 10:50:18 UTC
(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.
Comment 16 David Cook 2021-04-22 04:59:28 UTC
(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
Comment 17 Nick Clemens 2021-11-22 14:17:04 UTC
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 ***
Comment 18 Jonathan Druart 2021-11-22 14:39:50 UTC
(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.
Comment 19 Nick Clemens 2021-11-22 14:47:59 UTC
(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
Comment 20 Jonathan Druart 2021-11-23 12:48:00 UTC
I've applied the test from the second patch and it's failing. See bug 27008.