Bug 35099 - Cannot load records with invalid marcxml
Summary: Cannot load records with invalid marcxml
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_23_11_candidate
Depends on: 26314 23846
Blocks: 35117 34014 35119
  Show dependency treegraph
 
Reported: 2023-10-18 17:33 UTC by Nick Clemens
Modified: 2023-10-25 21:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 35099: Defensive coding for bad MARC21 records (2.25 KB, patch)
2023-10-19 08:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35099: Defensive coding for bad MARC21 records (2.25 KB, patch)
2023-10-19 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35099: Prevent crash from invalid marc in the parent record (1.33 KB, patch)
2023-10-19 11:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35099: Defensive coding for bad MARC21 records (2.32 KB, patch)
2023-10-19 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35099: Prevent crash from invalid marc in the parent record (1.40 KB, patch)
2023-10-19 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35099: Improve consistency (4.61 KB, patch)
2023-10-19 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML (825 bytes, patch)
2023-10-20 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML (879 bytes, patch)
2023-10-20 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML (946 bytes, patch)
2023-10-20 13:27 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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 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 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