Bug 21203

Summary: ILS-DI - GetRecords crashes on non-existent records
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Web servicesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: major    
Priority: P5 - low CC: fridolin.somers, josef.moravec, katrin.fischer, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18255    
Bug Blocks: 21202    
Attachments: Bug 21203: ILS-DI - Make GetRecords handle non-existent records
Bug 21203: ILS-DI - Make GetRecords handle non-existent records
Bug 21203: ILS-DI - Make GetRecords handle non-existent records

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