Bug 35099

Summary: Cannot load records with invalid marcxml
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: MARC Bibliographic data supportAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, martin.renvoize, ron.houk
Version: MainKeywords: rel_23_11_candidate
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Bug Depends on: 26314, 23846    
Bug Blocks: 35117, 34014, 35119    
Attachments: Bug 35099: Defensive coding for bad MARC21 records
Bug 35099: Defensive coding for bad MARC21 records
Bug 35099: Prevent crash from invalid marc in the parent record
Bug 35099: Defensive coding for bad MARC21 records
Bug 35099: Prevent crash from invalid marc in the parent record
Bug 35099: Improve consistency
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML

Description Nick Clemens (kidclamp) 2023-10-18 17:33:39 UTC
After bug 26314 we cannot load records with marcxml issues - the 
get_volumes_query calls Koha::Biblio->metadata->record and does not catch the exception
Comment 1 Jonathan Druart 2023-10-19 05:36:20 UTC
There is a test to catch that t/db_dependent/selenium/regressions.t
Comment 2 Martin Renvoize 2023-10-19 08:31:48 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2023-10-19 08:51:01 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens (kidclamp) 2023-10-19 11:51:20 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2023-10-19 12:41:44 UTC
Created attachment 157430 [details] [review]
Bug 35099: Defensive coding for bad MARC21 records

This patch modernises the get_marc_volumes method to match the form of
get_marc_components. The original code was submitted around a similar
time but didn't get the modernisations introduced since.

We remove the original localised caching as well as putting an eval
around the search_simple_compat call to prevent crashes on bad marc
records found in the database.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2023-10-19 12:41:46 UTC
Created attachment 157431 [details] [review]
Bug 35099: Prevent crash from invalid marc in the parent record

When getting the query for components, we need to parse the MARC::Record.
If that fails, we simply should not attempt the component search as the user should see a warning about the degraded MARC

To test:
prove -v t/db_dependent/selenium/regressions.t
It fails
apply patch, restart_all
prove -v t/db_dependent/selenium/regressions.t
It passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2023-10-19 12:41:48 UTC
Created attachment 157432 [details] [review]
Bug 35099: Improve consistency

This patch inproves the consistency between get_components_query and
get_get_volumes_query methods so they both check for MARC21 and check
that the record is valid prior to building the query.

We also add a check for !$invalid_marc_record in the staff client
details view to match the check for the equivilent compenents check.

We need both as the OPAC doesn't do an early check for invalid records
in the controller.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Jonathan Druart 2023-10-19 15:47:04 UTC
1. tests are missing

2.  739     return [] if ( C4::Context->preference('marcflavour') ne 'MARC21' ); 
Why that? Where is it advertised?

3. 
 781     my $marc;                                                                                        
 782     eval { $marc = $self->metadata->record; };                                                       
 783     return unless $marc;      

I disagree with that, and I thought we agreed on it.

We must assume the MARC is good, and deal with invalid record on the detail page (and edition now) only.

The comment is also wrong, you could call this method from somewhere else, and we just ignore the error, which we should (almost) never do.

4. Finally, isn't this change enough?
-my $show_volumes = @{ $biblio->get_marc_volumes(1) } ? 1 : 0;
+my $show_volumes = ( !$invalid_marc_record && @{ $biblio->get_marc_volumes(1) } ) ? 1 : 0;
Comment 9 Martin Renvoize 2023-10-19 16:36:19 UTC
I just tried to make it all more consistent with the existing methods that accomplish the same.

The OPAC doesn't do early checks and thus currently explodes I suspect without this patchset.. looks likely only staff was covered to me.

I worked in a fix quickly for release..
Comment 10 Jonathan Druart 2023-10-20 12:37:44 UTC
I confirm that the following change makes the test pass and 369 is accessible with a nice red warning.

-my $show_volumes = @{ $biblio->get_marc_volumes(1) } ? 1 : 0;
+my $show_volumes = ( !$invalid_marc_record && @{ $biblio->get_marc_volumes(1) } ) ? 1 : 0;
Comment 11 Jonathan Druart 2023-10-20 12:39:06 UTC
Created attachment 157510 [details] [review]
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML
Comment 12 Jonathan Druart 2023-10-20 12:39:28 UTC
This patch fixes the original issue. If you have other concerns you should open new bug reports and provide tests.
Comment 13 Nick Clemens (kidclamp) 2023-10-20 13:16:59 UTC
Created attachment 157513 [details] [review]
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens (kidclamp) 2023-10-20 13:18:06 UTC
Trivial fix, passing QA - there are larger issues around this, but this gets things working
Comment 15 Martin Renvoize 2023-10-20 13:27:04 UTC
Created attachment 157515 [details] [review]
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-10-20 13:27:35 UTC
lol

We clashed.. but were doing the same thing :)
Comment 17 Tomás Cohen Arazi 2023-10-20 14:04:14 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-10-25 21:42:08 UTC
Depends on Bug 26314 not in 23.05.x