Bug 29437

Summary: 500 error when performing a catalog search for an ISBN13 with no valid ISBN10
Product: Koha Reporter: Nick Clemens <nick>
Component: CatalogingAssignee: Nick Clemens <nick>
Status: Pushed to oldoldstable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: david, dcook, fridolin.somers, jonathan.druart+koha, joonas.kylmala, kyle, m.de.rooy, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29438
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.14
Bug Depends on: 29319    
Bug Blocks:    
Attachments: Bug 29437: Search reservoir for term as title, author, or variations of ISBN
Bug 29437: Search reservoir for term as title, author, or variations of ISBN
Bug 29437: Unit tests
Bug 29437: Search reservoir for term as title, author, or variations of ISBN
Bug 29437: Unit tests
Bug 29437: Search reservoir for term as title, author, or variations of ISBN
Bug 29437: Unit tests
Bug 29437: Search reservoir for term as title, author, or variations of ISBN
Bug 29437: (QA follow-up) Remove Business::ISBN from addbooks
Bug 29437: (QA follow-up) Update TODO in Breeding.t

Description Nick Clemens 2021-11-08 19:24:14 UTC
When performing a cataloging search we take the query and test to see if it is a valid isbn:
If not, we perform a SQL search for the term as author or title
If so, we pass it to BreedingSearch as an ISBN - we then normailize it to ISBN 10 - then perform aSQL search for ISBN

In the ISBN case, if it cannot be converted to a 10 digit ISBN we nullify the term - the SQL then has no where statement and fails in plack-error.log:
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 at /kohadevbox/koha/cataloguing/addbooks.pl line 117
Comment 1 Nick Clemens 2021-11-08 19:31:41 UTC
Created attachment 127458 [details] [review]
Bug 29437: Search reservoir for term as title, author, or variations of ISBN

The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather
than try to do this, we can simply search it on the three fields: isbn, title, author

Additionally, we should search as any of the ISBN variations to broaden our matches

Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert
the value will be blank - this is another bug

To test:
1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart:
    9798200834976
2 - 500 error
3 - Apply patch
4 - Repeat, no results
5 - Import some records
6 - Search by title/author/isbn
7 - Confirm searching works as expected
Comment 2 Nick Clemens 2021-11-08 19:32:17 UTC
Needs unit tests for QA, but can be tested
Comment 3 David Nind 2021-11-08 22:55:15 UTC
Created attachment 127460 [details] [review]
Bug 29437: Search reservoir for term as title, author, or variations of ISBN

The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather
than try to do this, we can simply search it on the three fields: isbn, title, author

Additionally, we should search as any of the ISBN variations to broaden our matches

Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert
the value will be blank - this is another bug

To test:
1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart:
    9798200834976
2 - 500 error
3 - Apply patch
4 - Repeat, no results
5 - Import some records
6 - Search by title/author/isbn
7 - Confirm searching works as expected

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-11-08 23:06:06 UTC
(In reply to Nick Clemens from comment #2)
> Needs unit tests for QA, but can be tested

Changed status back to needs signoff - everything work as per the test plan.
Comment 5 Nick Clemens 2021-11-10 13:47:58 UTC
Created attachment 127512 [details] [review]
Bug 29437: Unit tests
Comment 6 Nick Clemens 2021-11-10 13:48:02 UTC
Created attachment 127513 [details] [review]
Bug 29437: Search reservoir for term as title, author, or variations of ISBN

The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather
than try to do this, we can simply search it on the three fields: isbn, title, author

Additionally, we should search as any of the ISBN variations to broaden our matches

Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert
the value will be blank - this is another bug

To test:
1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart:
    9798200834976
2 - 500 error
3 - Apply patch
4 - Repeat, no results
5 - Import some records
6 - Search by title/author/isbn
7 - Confirm searching works as expected

WNC amended to fix spelling

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2021-11-10 17:25:52 UTC
Created attachment 127516 [details] [review]
Bug 29437: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2021-11-10 17:25:56 UTC
Created attachment 127517 [details] [review]
Bug 29437: Search reservoir for term as title, author, or variations of ISBN

The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather
than try to do this, we can simply search it on the three fields: isbn, title, author

Additionally, we should search as any of the ISBN variations to broaden our matches

Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert
the value will be blank - this is another bug

To test:
1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart:
    9798200834976
2 - 500 error
3 - Apply patch
4 - Repeat, no results
5 - Import some records
6 - Search by title/author/isbn
7 - Confirm searching works as expected

WNC amended to fix spelling

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Marcel de Rooy 2021-11-19 07:50:33 UTC
QAing
Comment 10 Marcel de Rooy 2021-11-19 08:32:41 UTC
Created attachment 127842 [details] [review]
Bug 29437: Unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2021-11-19 08:32:44 UTC
Created attachment 127843 [details] [review]
Bug 29437: Search reservoir for term as title, author, or variations of ISBN

The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather
than try to do this, we can simply search it on the three fields: isbn, title, author

Additionally, we should search as any of the ISBN variations to broaden our matches

Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert
the value will be blank - this is another bug

To test:
1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart:
    9798200834976
2 - 500 error
3 - Apply patch
4 - Repeat, no results
5 - Import some records
6 - Search by title/author/isbn
7 - Confirm searching works as expected

WNC amended to fix spelling

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
AMENDED: Useless call of ISBNs (plural) when you only pass one parameter.
Comment 12 Marcel de Rooy 2021-11-19 08:32:48 UTC
Created attachment 127844 [details] [review]
Bug 29437: (QA follow-up) Remove Business::ISBN from addbooks

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2021-11-19 08:32:51 UTC
Created attachment 127845 [details] [review]
Bug 29437: (QA follow-up) Update TODO in Breeding.t

We now have some tests for BreedingSearch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2021-11-19 08:36:38 UTC
(In reply to David Nind from comment #8)
> Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13
> that doesn't convert
> the value will be blank - this is another bug

Did you open a new report ?
Comment 15 Marcel de Rooy 2021-11-19 08:39:20 UTC
Looks good enough to me. A funny side effect is that we find reservoir books via the ISBN variations 10/13/hypens/no hyphens. We wont find them in the catalog. But if we search for a partial isbn 545 (as being part of eg 0-545-01022-5), we will not find anything in the reservoir, but Zebra finds it in the catalog.
Comment 16 Jonathan Druart 2021-11-19 14:15:47 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 17 Kyle M Hall 2021-11-19 17:02:17 UTC
Pushed to 21.05.x for 21.05.05
Comment 18 Fridolin Somers 2021-11-20 02:10:27 UTC
Depends on Bug 29319 not in 20.11.x
Comment 19 Nick Clemens 2021-11-22 14:17:04 UTC
*** Bug 17979 has been marked as a duplicate of this bug. ***
Comment 20 Marcel de Rooy 2021-12-08 14:52:09 UTC
(In reply to Fridolin Somers from comment #18)
> Depends on Bug 29319 not in 20.11.x

Actually a pity that we didnt backport something.
Quite easy to generate this in the logs:
[2021/12/08 15:49:20] [WARN] 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 /usr/share/koha/C4/Breeding.pm line 100.
[2021/12/08 15:49:20] [WARN] DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [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 /usr/share/koha/C4/Breeding.pm line 101.
Comment 21 Marcel de Rooy 2021-12-08 15:09:08 UTC
This is the most minimal maintenance solution I could come up with for 20.11:

diff --git a/C4/Breeding.pm b/C4/Breeding.pm
index 6b103b3..5b0d83a 100644
--- a/C4/Breeding.pm
+++ b/C4/Breeding.pm
@@ -96,6 +96,7 @@ sub BreedingSearch {
         $query .= "isbn like ?";
         push(@bind,"$isbn%");
     }
+    return 0 unless @bind;
     $sth = $dbh->prepare($query);
     $sth->execute(@bind);
     while (my $data = $sth->fetchrow_hashref) {

Since Koha returns a 200, it only gives you a few log lines less.
Comment 22 Victor Grousset/tuxayo 2021-12-22 20:51:35 UTC
Hi, I'll try to backport this for 20.11.

Thanks Marcel for digging into this, how should this change be used? How can I use it to solve the conflicts due to bug 29319 missing?
Or maybe it's a whole alternative patch?

Another way: does bug 29319 seems like a risky backport to you? The odd thing is that I can catalogue search stuff that begins with digits without getting errors. In other words, I don't reproduce the issue.
Comment 23 Marcel de Rooy 2022-01-10 08:55:50 UTC
(In reply to Victor Grousset/tuxayo from comment #22)
> Hi, I'll try to backport this for 20.11.
> 
> Thanks Marcel for digging into this, how should this change be used? How can
> I use it to solve the conflicts due to bug 29319 missing?
> Or maybe it's a whole alternative patch?
> 
> Another way: does bug 29319 seems like a risky backport to you? The odd
> thing is that I can catalogue search stuff that begins with digits without
> getting errors. In other words, I don't reproduce the issue.

29319 seems quite innocent to me
Comment 24 Victor Grousset/tuxayo 2022-01-31 17:04:48 UTC
Backported: Pushed to 20.11.x branch for 20.11.14