Bug 21203 - ILS-DI - GetRecords crashes on non-existent records
Summary: ILS-DI - GetRecords crashes on non-existent records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 18255
Blocks: 21202
  Show dependency treegraph
 
Reported: 2018-08-10 15:18 UTC by Jonathan Druart
Modified: 2019-10-14 19:57 UTC (History)
5 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 21203: ILS-DI - Make GetRecords handle non-existent records (1.61 KB, patch)
2018-08-10 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21203: ILS-DI - Make GetRecords handle non-existent records (1.68 KB, patch)
2018-08-14 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21203: ILS-DI - Make GetRecords handle non-existent records (1.72 KB, patch)
2018-08-16 10:24 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-08-10 15:18:23 UTC
Caused by
  commit cb336e633bf7e0cb25051a8ed8157af8486a58d7
  Bug 18255: Koha::Biblio - Replace GetBiblioItemByBiblioNumber with Koha::Biblio->biblioitem


Can't call method "biblioitem" on an undefined value at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 212
Comment 1 Jonathan Druart 2018-08-10 15:21:01 UTC
Created attachment 77688 [details] [review]
Bug 21203: ILS-DI - Make GetRecords handle non-existent records

Caused by
  commit cb336e633bf7e0cb25051a8ed8157af8486a58d7
  Bug 18255: Koha::Biblio - Replace GetBiblioItemByBiblioNumber with Koha::Biblio->biblioitem

Can't call method "biblioitem" on an undefined value at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 212

Test plan:
- Enable ilsdi
- hit /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=X
With X an existing biblionumber and a non existing one

Both requests must succeed
Comment 2 Martin Renvoize 2018-08-13 09:53:28 UTC
QA Comment: Can we 100% guarantee that there is always 1 and only 1 biblioitem attached to a biblio object?

Looking at the dbic schema it appears there's no db level constraint for this (it's a has_many relationship), looking a level up in the Koha::Biblio object it appears we do indeed fire a ->find rather than using the underlying dbic relationship, which suggests there's only 1 (or 0) biblioitem attached to a biblio but there's no test to confirm we actually get a biblioitem object back.

I'd love to see the underlying db constraints added, but would accept a second check that the biblioitem exists before calling unblessed upon it.

Perhaps I'm being over cautious?
Comment 3 Jonathan Druart 2018-08-13 17:39:07 UTC
(In reply to Martin Renvoize from comment #2)
> QA Comment: Can we 100% guarantee that there is always 1 and only 1
> biblioitem attached to a biblio object?
> 
> Looking at the dbic schema it appears there's no db level constraint for
> this (it's a has_many relationship), looking a level up in the Koha::Biblio
> object it appears we do indeed fire a ->find rather than using the
> underlying dbic relationship, which suggests there's only 1 (or 0)
> biblioitem attached to a biblio but there's no test to confirm we actually
> get a biblioitem object back.
> 
> I'd love to see the underlying db constraints added, but would accept a
> second check that the biblioitem exists before calling unblessed upon it.
> 
> Perhaps I'm being over cautious?

I do not think this is valid, biblio and biblioitem must be considered 1-1, we discussed it several times on the mailing lists.
Both tables must be merged, but it's a huge work.
Comment 4 Katrin Fischer 2018-08-13 19:24:10 UTC
I agree with Jonathan - there are bigger issues with our database structure, but assuming 1:1 is safe for now and we should fix this bug.
Comment 5 Martin Renvoize 2018-08-14 08:19:55 UTC
OK, I'm happy with that then.. I must have missed the mailing list conversations regarding their 1-1ness during my absence.

I'll go ahead and complete the QA then. :)
Comment 6 Martin Renvoize 2018-08-14 09:40:50 UTC
Created attachment 77777 [details] [review]
Bug 21203: ILS-DI - Make GetRecords handle non-existent records

Caused by
  commit cb336e633bf7e0cb25051a8ed8157af8486a58d7
  Bug 18255: Koha::Biblio - Replace GetBiblioItemByBiblioNumber with Koha::Biblio->biblioitem

Can't call method "biblioitem" on an undefined value at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 212

Test plan:
- Enable ilsdi
- hit /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=X
With X an existing biblionumber and a non existing one

Both requests must succeed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2018-08-14 09:41:19 UTC
Sorry.. I meant signoff rather than qa there.. Signed Off.
Comment 8 Josef Moravec 2018-08-16 10:24:11 UTC
Created attachment 77887 [details] [review]
Bug 21203: ILS-DI - Make GetRecords handle non-existent records

Caused by
  commit cb336e633bf7e0cb25051a8ed8157af8486a58d7
  Bug 18255: Koha::Biblio - Replace GetBiblioItemByBiblioNumber with Koha::Biblio->biblioitem

Can't call method "biblioitem" on an undefined value at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 212

Test plan:
- Enable ilsdi
- hit /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=X
With X an existing biblionumber and a non existing one

Both requests must succeed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Nick Clemens 2018-08-21 13:57:23 UTC
Awesome work all!

Pushed to master for 18.11
Comment 10 Martin Renvoize 2018-08-24 08:55:50 UTC
Pushed to 18.05.x for 18.05.03.
Comment 11 Fridolin Somers 2018-08-28 13:33:48 UTC
Depends on Bug 18255 not in 17.11.x