Bug 29690

Summary: Software error in details.pl when invalid MARCXML
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: CatalogingAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, hayleypelham, jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: masterKeywords: regression
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27266
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29779
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03
Bug Depends on: 23846, 27266    
Bug Blocks: 29697, 30008, 30009    
Attachments: Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML
Bug 29690: (follow-up) Catch other cases
Bug 29690: Unit test for get_components_query
Bug 29690: Always warn if something is wrong with the metadata
Bug 29690: Always warn if something is wrong with the metadata
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML
Bug 29690: (follow-up) Catch other cases
Bug 29690: Unit test for get_components_query
Bug 29690: Always warn if something is wrong with the metadata
Bug 29690: Add selenium test
Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors()
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML
Bug 29690: (follow-up) Catch other cases
Bug 29690: Unit test for get_components_query
Bug 29690: Always warn if something is wrong with the metadata
Bug 29690: Add selenium test
Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors()
Bug 29690: Fix tests count
Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in invalid
Bug 29690: Add selenium test
Bug 29690: Prevent detail.pl to crash if MARCXLM in invalid
Bug 29690: Add selenium test
Bug 29690: Prevent detail.pl to crash if MARCXML is invalid
Bug 29690: Add selenium test
Bug 29690: Prevent detail.pl to crash if MARCXML is invalid

Description Fridolin Somers 2021-12-14 06:19:48 UTC
Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.
Comment 1 Fridolin Somers 2021-12-14 06:27:01 UTC
Created attachment 128484 [details] [review]
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...
Comment 2 Fridolin Somers 2021-12-14 06:27:42 UTC
If we are OK with this implementation, I will amend unit tests (any help is welcome)
Comment 3 Martin Renvoize 2021-12-14 06:40:32 UTC
Could we not move the eval up a level into the record accessor itself this catching all cases?
Comment 4 Martin Renvoize 2021-12-14 07:44:23 UTC
(In reply to Martin Renvoize from comment #3)
> Could we not move the eval up a level into the record accessor itself this
> catching all cases?

OK, we already do and throw an exception.. hence your evals here.. my mistake
Comment 5 Martin Renvoize 2021-12-14 08:14:10 UTC
Created attachment 128485 [details] [review]
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-12-14 08:14:13 UTC
Created attachment 128486 [details] [review]
Bug 29690: (follow-up) Catch other cases

This patch catches the rest of the cases in Koha::Biblio where
`->metadata->record` may fail.
Comment 7 Martin Renvoize 2021-12-14 08:17:46 UTC
It looks to me like `opac/opac-ISBDdetail.pl` and `opac/opac-showmarc.pl` are also affected by this.. should we fix those here too.. and perhaps introduce a QA script check for any `->metadata->record` call not wrapped in eval or try/catch?
Comment 8 Fridolin Somers 2021-12-14 08:56:58 UTC
(In reply to Martin Renvoize from comment #7)
> It looks to me like `opac/opac-ISBDdetail.pl` and `opac/opac-showmarc.pl`
> are also affected by this.. should we fix those here too.. and perhaps
> introduce a QA script check for any `->metadata->record` call not wrapped in
> eval or try/catch?

Mmm lets focus for this bug on regression in detail.pl.
We may open other reports :)
Comment 9 Martin Renvoize 2021-12-14 09:31:22 UTC
Created attachment 128487 [details] [review]
Bug 29690: Unit test for get_components_query
Comment 10 Jonathan Druart 2021-12-14 09:38:11 UTC
> 1) Create a biblio record with invalid MARCXML (see Bug 29690)

Referring to bug 23846?
Comment 11 Jonathan Druart 2021-12-14 09:43:07 UTC
Should not we at least warn? There is nothing else more frustrating that investigating a problem and find that it's coming from an exception that is being ignored.
Comment 12 Martin Renvoize 2021-12-14 09:46:42 UTC
(In reply to Jonathan Druart from comment #11)
> Should not we at least warn?

Probably a good call.. I hate silent failures... do we not already throw a warning from the parent as part of the exception it throws though?
Comment 13 Martin Renvoize 2021-12-14 09:48:03 UTC
It would appear not.. so yes, throwing a warning in would be beneficial.
Comment 14 Jonathan Druart 2021-12-14 09:56:27 UTC
Created attachment 128488 [details] [review]
Bug 29690: Always warn if something is wrong with the metadata
Comment 15 Jonathan Druart 2021-12-14 09:57:19 UTC
Would something like this patch work for you, Martin?
Comment 16 Martin Renvoize 2021-12-14 09:59:58 UTC
Looks great to me Jonathan, thanks for taking it on :)
Comment 17 Jonathan Druart 2021-12-14 11:26:19 UTC
Created attachment 128497 [details] [review]
Bug 29690: Always warn if something is wrong with the metadata
Comment 18 David Nind 2021-12-14 17:28:37 UTC
Created attachment 128530 [details] [review]
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2021-12-14 17:28:43 UTC
Created attachment 128531 [details] [review]
Bug 29690: (follow-up) Catch other cases

This patch catches the rest of the cases in Koha::Biblio where
`->metadata->record` may fail.

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2021-12-14 17:28:48 UTC
Created attachment 128532 [details] [review]
Bug 29690: Unit test for get_components_query

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2021-12-14 17:28:57 UTC
Created attachment 128533 [details] [review]
Bug 29690: Always warn if something is wrong with the metadata

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 Fridolin Somers 2021-12-14 19:17:55 UTC
(In reply to Martin Renvoize from comment #9)
> Created attachment 128487 [details] [review] [review]
> Bug 29690: Unit test for get_components_query

Thanks a lot.
We need to also impact other unit tests right ?
get_coins, is_serial ...
Comment 23 Jonathan Druart 2021-12-15 07:24:34 UTC
Created attachment 128541 [details] [review]
Bug 29690: Add selenium test
Comment 24 Jonathan Druart 2021-12-15 07:25:06 UTC
(In reply to Fridolin Somers from comment #22)
> (In reply to Martin Renvoize from comment #9)
> > Created attachment 128487 [details] [review] [review] [review]
> > Bug 29690: Unit test for get_components_query
> 
> Thanks a lot.
> We need to also impact other unit tests right ?
> get_coins, is_serial ...

I actually prefer a selenium test, it will catch possible future regressions.
Comment 25 Fridolin Somers 2021-12-15 08:47:46 UTC
(In reply to Jonathan Druart from comment #24)
> (In reply to Fridolin Somers from comment #22)
> > (In reply to Martin Renvoize from comment #9)
> > > Created attachment 128487 [details] [review] [review] [review] [review]
> > > Bug 29690: Unit test for get_components_query
> > 
> > Thanks a lot.
> > We need to also impact other unit tests right ?
> > get_coins, is_serial ...
> 
> I actually prefer a selenium test, it will catch possible future regressions.

Super giga.
Comment 26 Fridolin Somers 2021-12-15 22:37:14 UTC
Created attachment 128578 [details] [review]
Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors()
Comment 27 Fridolin Somers 2021-12-15 22:38:27 UTC
(In reply to Fridolin Somers from comment #26)
> Created attachment 128578 [details] [review] [review]
> Bug 29690: (follow-up) Add eval on metadata record fetch in
> get_marc_authors()

@Rmaints :
When backporting this, skip this patch if branch does not contain Bug 27266
Comment 28 Hayley Pelham 2021-12-22 01:14:55 UTC
Hello,

What needs signing off here?
Comment 29 Fridolin Somers 2021-12-22 06:15:09 UTC
((In reply to Hayley Pelham from comment #28)
> Hello,
> 
> What needs signing off here?

Last patch :
Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors() 

I bet one can not sign the Selenuim patch.
Comment 30 Hayley Pelham 2021-12-22 21:57:26 UTC
I gave testing the Selenium test regressions.t a go.

It failed, here is the output:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/selenium/regressions.t 
t/db_dependent/selenium/regressions.t .. 1/7 Label not found for "last SKIP" at /usr/share/perl/5.28/Test/More.pm line 1370.
Use of uninitialized value $value in string eq at /home/vagrant/kohaclone/C4/Context.pm line 423.
# Looks like your test exited with 2 just after 7.
t/db_dependent/selenium/regressions.t .. Dubious, test returned 2 (wstat 512, 0x200)
All 7 subtests passed 
	(less 7 skipped subtests: 0 okay)

Test Summary Report
-------------------
t/db_dependent/selenium/regressions.t (Wstat: 512 Tests: 7 Failed: 0)
  Non-zero exit status: 2
Files=1, Tests=7,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.28 cusr  0.17 csys =  1.47 CPU)
Result: FAIL


I think I set up the Selenium server correctly, here is the output from another Selenium test:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/selenium/basic_workflow.t 
t/db_dependent/selenium/basic_workflow.t .. ok     
All tests successful.
Files=1, Tests=22,  1 wallclock secs ( 0.03 usr  0.00 sys +  1.27 cusr  0.11 csys =  1.41 CPU)


Please let me know if you think I have missed a step here

Cheers,
Hayley
Comment 31 Jonathan Druart 2022-01-03 11:29:51 UTC
Hayley, your dev box is not configured correctly for selenium tests. You should first install Selenium::Remote::Driver.

However there is a bug as the tests should return green even if the module is not installed. But it's not related to these patches.
Comment 32 Jonathan Druart 2022-01-03 11:33:47 UTC
(In reply to Jonathan Druart from comment #31)
> However there is a bug as the tests should return green even if the module
> is not installed. But it's not related to these patches.

Opened bug 29779.
Comment 33 Hayley Pelham 2022-01-11 21:20:05 UTC
Hi Jonathan,

Thanks for letting me know.

I've gone through and followed this:
https://wiki.koha-community.org/wiki/Using_Selenium_with_Koha

And made sure to install that module now.

I can get a server running, but get an exception when trying to prove the test:
org.openqa.selenium.firefox.NotConnectedException: Unable to connect to host 127.0.0.1 on port 7055 after 45000 ms

So I may have to leave the testing here to someone else.

Are the Selenium wiki docs up to date?

Cheers,
Hayley
Comment 34 Tomás Cohen Arazi 2022-01-12 13:11:01 UTC
Created attachment 129339 [details] [review]
Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...

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

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2022-01-12 13:11:06 UTC
Created attachment 129340 [details] [review]
Bug 29690: (follow-up) Catch other cases

This patch catches the rest of the cases in Koha::Biblio where
`->metadata->record` may fail.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Tomás Cohen Arazi 2022-01-12 13:11:11 UTC
Created attachment 129341 [details] [review]
Bug 29690: Unit test for get_components_query

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2022-01-12 13:11:16 UTC
Created attachment 129342 [details] [review]
Bug 29690: Always warn if something is wrong with the metadata

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2022-01-12 13:11:21 UTC
Created attachment 129343 [details] [review]
Bug 29690: Add selenium test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2022-01-12 13:11:26 UTC
Created attachment 129344 [details] [review]
Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2022-01-12 13:11:30 UTC
Created attachment 129345 [details] [review]
Bug 29690: Fix tests count

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2022-01-12 13:13:28 UTC
I saw David's signature so was about to PQA it, but it seems it was NSO. Not sure what is the status. Adding my signature as it does the job, looks correct, tests pass and the QA scripts are happy.

My only doubt is about Koha::Biblio calling warn... I'm still trying to figure if it should use the logger instead.
Comment 42 Jonathan Druart 2022-01-13 05:14:34 UTC
Thinking a bit more about it. We decided to not explode on detail.pl to provide a nice message to the end user. But we decided to not care about the other views, there is a script already to catch such problems, etc.

Here I am wondering if we are not actually fixing the problem at the wrong level. Methods should not deal with the errors (or we are going to add checks everywhere), it's controller's job to catch the error IMO.
Comment 43 Jonathan Druart 2022-01-13 05:18:14 UTC
Minor but worth noting: the patches rename $marc with $record. It should actually be $marc_record I think, $record is too generic.
Comment 44 Jonathan Druart 2022-01-13 05:21:40 UTC
Created attachment 129365 [details] [review]
Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in invalid
Comment 45 Jonathan Druart 2022-01-13 05:22:23 UTC
(In reply to Jonathan Druart from comment #44)
> Created attachment 129365 [details] [review] [review]
> Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in
> invalid

Tomas, Martin, Fridolin, what about this patch? It would obsolete the other patches except the selenium tests.
Comment 46 Fridolin Somers 2022-01-13 06:40:03 UTC
(In reply to Jonathan Druart from comment #45)
> (In reply to Jonathan Druart from comment #44)
> > Created attachment 129365 [details] [review] [review] [review]
> > Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in
> > invalid
> 
> Tomas, Martin, Fridolin, what about this patch? It would obsolete the other
> patches except the selenium tests.

Hi, I like this shorter patch.
We deal with a regression, I think the most simple fix is the best.
Comment 47 Jonathan Druart 2022-01-13 07:25:19 UTC
Created attachment 129381 [details] [review]
Bug 29690: Add selenium test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Jonathan Druart 2022-01-13 07:25:24 UTC
Created attachment 129382 [details] [review]
Bug 29690: Prevent detail.pl to crash if MARCXLM in invalid
Comment 49 Fridolin Somers 2022-01-13 07:33:25 UTC
Small typo in commit message : 
"if MARCXLM in invalid"
=>  MARCXML
Comment 50 Fridolin Somers 2022-01-17 19:50:54 UTC
Created attachment 129542 [details] [review]
Bug 29690: Add selenium test

Run prove t/db_dependent/selenium/regressions.t on KTD with selenium

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 51 Fridolin Somers 2022-01-17 19:51:23 UTC
Created attachment 129543 [details] [review]
Bug 29690: Prevent detail.pl to crash if MARCXML is invalid

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 52 Fridolin Somers 2022-01-17 19:55:24 UTC
I signoff.

Test successful with koha-testing-docker.

Fixed number of tests in selenium test file, from 7 to 8.
Copied test plan from old patch to new main patch.
Fixed typo in main patch message "if MARCXLM in invalid"

Go go team :D
Comment 53 Martin Renvoize 2022-01-24 15:12:41 UTC
Created attachment 129730 [details] [review]
Bug 29690: Add selenium test

Run prove t/db_dependent/selenium/regressions.t on KTD with selenium

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2022-01-24 15:12:45 UTC
Created attachment 129731 [details] [review]
Bug 29690: Prevent detail.pl to crash if MARCXML is invalid

Bug 23846 added support for invalid MARCXML.
But now page details.pl fails again with software error.
This comes from several Koha::Biblio calling $self->metadata->record without eval.

Test plan :
1) Create a biblio record with invalid MARCXML (see Bug 29690)
In koha-testing-docker there is biblionumber=369
2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
3) You see the page with a message :
There is an error with this bibliographic record, the view may be degraded.
Error: Invalid data, cannot decode metadata object ...

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2022-01-24 15:13:24 UTC
Read over the history to remind myself how we got to this point.. all appear to be in order.. Tested.

Passing QA
Comment 56 Fridolin Somers 2022-01-29 07:54:10 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 57 Jonathan Druart 2022-02-02 14:09:51 UTC
*** Bug 29970 has been marked as a duplicate of this bug. ***
Comment 58 Kyle M Hall 2022-02-07 15:26:11 UTC
Pushed to 21.11.x for 21.11.03
Comment 59 Andrew Fuerste-Henry 2022-02-21 15:12:33 UTC
Missing dependency, not backported to 21.05