From 9363836d8cc00ea4d59f453ed2bd5d51c1a5f732 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 26 Feb 2026 10:23:19 +0000 Subject: [PATCH] Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. --- C4/Biblio.pm | 13 ++- Koha/Biblio/Metadata.pm | 60 ++++++++++-- cataloguing/addbiblio.pl | 93 +++++++++++++------ cataloguing/merge.pl | 8 +- .../prog/en/modules/cataloguing/addbiblio.tt | 12 +++ .../prog/en/modules/cataloguing/merge.tt | 11 ++- t/db_dependent/Koha/Biblio/Metadata.t | 46 +++++---- 7 files changed, 181 insertions(+), 62 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 60d56e6e937..629ed885c4a 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -298,6 +298,7 @@ sub AddBiblio { { skip_record_index => 1, record_source_id => $options->{record_source_id}, + ( ref $options->{warnings} eq 'ARRAY' ? ( warnings => $options->{warnings} ) : () ), } ); @@ -394,7 +395,12 @@ sub ModBiblio { ( exists $options->{record_source_id} ) ? ( record_source_id => $options->{record_source_id} ) : () - ) + ), + ( + ( ref $options->{warnings} eq 'ARRAY' ) + ? ( warnings => $options->{warnings} ) + : () + ), }; if ( !$record ) { @@ -2998,6 +3004,11 @@ sub ModBiblioMarc { } )->store; + if ( $m_rs->stripped_nonxml && ref $options->{warnings} eq 'ARRAY' ) { + push @{ $options->{warnings} }, + { type => 'nonxml_stripped', message => $m_rs->stripped_nonxml }; + } + unless ($skip_record_index) { my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm index 926f5d85bc5..2dca4d7641c 100644 --- a/Koha/Biblio/Metadata.pm +++ b/Koha/Biblio/Metadata.pm @@ -26,6 +26,7 @@ use C4::Items qw( GetMarcItem ); use Koha::Database; use Koha::Exceptions::Metadata; +use Koha::Logger; use Koha::RecordSources; use base qw(Koha::Object); @@ -45,6 +46,13 @@ Koha::Metadata - Koha Metadata Object class Metadata specific store method to catch errant characters prior to committing to the database. +If the MARCXML cannot be parsed but can be recovered by stripping non-XML +characters (C), the stripped version is saved and +C is set on the object so callers can notify the user. + +If the MARCXML cannot be recovered at all, a +I exception is thrown. + =cut sub store { @@ -62,20 +70,56 @@ sub store { my $marcxml_error = $@; chomp $marcxml_error; unless ($marcxml) { - warn $marcxml_error; - Koha::Exceptions::Metadata::Invalid->throw( - id => $self->id, - biblionumber => $self->biblionumber, - format => $self->format, - schema => $self->schema, - decoding_error => $marcxml_error, - ); + + # Attempt recovery by stripping non-XML characters + my $stripped_metadata = StripNonXmlChars( $self->metadata ); + my $stripped_marcxml = eval { MARC::Record::new_from_xml( $stripped_metadata, 'UTF-8', $self->schema ); }; + + if ($stripped_marcxml) { + + # Stripping fixed it – save the clean version and record what happened + Koha::Logger->get->warn( + sprintf( + "Non-XML characters stripped from bibliographic record (biblionumber=%s): %s", + $self->biblionumber // 'N/A', $marcxml_error + ) + ); + $self->metadata($stripped_metadata); + $self->{_stripped_nonxml} = $marcxml_error; + } else { + + # Truly unrecoverable + Koha::Logger->get->warn($marcxml_error); + Koha::Exceptions::Metadata::Invalid->throw( + id => $self->id, + biblionumber => $self->biblionumber, + format => $self->format, + schema => $self->schema, + decoding_error => $marcxml_error, + ); + } } } return $self->SUPER::store; } +=head3 stripped_nonxml + + if ( my $error = $metadata->stripped_nonxml ) { + # warn user that non-XML characters were removed on last store() + } + +Returns the original decode error string if non-XML characters were stripped +during the most recent call to C, or C otherwise. + +=cut + +sub stripped_nonxml { + my $self = shift; + return $self->{_stripped_nonxml}; +} + =head3 record my $record = $metadata->record; diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl index c9baa96b3ea..4bb600a029a 100755 --- a/cataloguing/addbiblio.pl +++ b/cataloguing/addbiblio.pl @@ -703,8 +703,9 @@ if ( $op eq "cud-addbiblio" ) { # it is not a duplicate (determined either by Koha itself or by user checking it's not a duplicate) if ( !$duplicatebiblionumber or $confirm_not_duplicate ) { my $oldbibitemnum; + my @save_warnings; my $encode_error = try { - if ( $is_a_modif ) { + if ($is_a_modif) { ModBiblio( $record, $biblionumber, @@ -714,11 +715,13 @@ if ( $op eq "cud-addbiblio" ) { source => $z3950 ? 'z3950' : 'intranet', categorycode => $logged_in_patron->categorycode, userid => $logged_in_patron->userid, - } + }, + warnings => \@save_warnings, } ); } else { - ( $biblionumber, $oldbibitemnum ) = AddBiblio( $record, $frameworkcode ); + ( $biblionumber, $oldbibitemnum ) = + AddBiblio( $record, $frameworkcode, { warnings => \@save_warnings } ); } return 0; } catch { @@ -730,41 +733,71 @@ if ( $op eq "cud-addbiblio" ) { } return 1; }; - if (!$encode_error && ($redirect eq "items" || ($mode ne "popup" && !$is_a_modif && $redirect ne "view" && $redirect ne "just_save"))){ - if ($frameworkcode eq 'FA'){ - print $input->redirect( - '/cgi-bin/koha/cataloguing/additem.pl?' - .'biblionumber='.$biblionumber - .'&frameworkcode='.$frameworkcode - .'&circborrowernumber='.$fa_circborrowernumber - .'&branch='.$fa_branch - .'&barcode='.uri_escape_utf8($fa_barcode) - .'&stickyduedate='.$fa_stickyduedate - .'&duedatespec='.$fa_duedatespec + + my ($nonxml_stripped) = grep { $_->{type} eq 'nonxml_stripped' } @save_warnings; + if ( $nonxml_stripped && !$encode_error ) { + $record = Koha::Biblios->find($biblionumber)->metadata->record; + build_tabs( $template, $record, $dbh, $encoding, $input ); + $template->param( + biblionumber => $biblionumber, + popup => $mode, + frameworkcode => $frameworkcode, + itemtype => $frameworkcode, + borrowernumber => $loggedinuser, + tab => scalar $input->param('tab'), + NONXML_STRIPPED => $nonxml_stripped->{message}, ); + output_html_with_http_headers $input, $cookie, $template->output; exit; - } - else { - print $input->redirect( + } + if ( + !$encode_error + && ( $redirect eq "items" + || ( $mode ne "popup" && !$is_a_modif && $redirect ne "view" && $redirect ne "just_save" ) ) + ) + { + if ( $frameworkcode eq 'FA' ) { + print $input->redirect( '/cgi-bin/koha/cataloguing/additem.pl?' + . 'biblionumber=' + . $biblionumber + . '&frameworkcode=' + . $frameworkcode + . '&circborrowernumber=' + . $fa_circborrowernumber + . '&branch=' + . $fa_branch + . '&barcode=' + . uri_escape_utf8($fa_barcode) + . '&stickyduedate=' + . $fa_stickyduedate + . '&duedatespec=' + . $fa_duedatespec ); + exit; + } else { + print $input->redirect( "/cgi-bin/koha/cataloguing/additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid" - ); - exit; + ); + exit; } - } elsif(!$encode_error && (($is_a_modif || $redirect eq "view") && $redirect ne "just_save")){ + } elsif ( !$encode_error && ( ( $is_a_modif || $redirect eq "view" ) && $redirect ne "just_save" ) ) { my $defaultview = C4::Context->preference('IntranetBiblioDefaultView'); - my $views = { C4::Search::enabled_staff_search_views }; - if ($defaultview eq 'isbd' && $views->{can_view_ISBD}) { - print $input->redirect("/cgi-bin/koha/catalogue/ISBDdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); - } elsif ($defaultview eq 'marc' && $views->{can_view_MARC}) { - print $input->redirect("/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid"); - } elsif ($defaultview eq 'labeled_marc' && $views->{can_view_labeledMARC}) { - print $input->redirect("/cgi-bin/koha/catalogue/labeledMARCdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); + my $views = {C4::Search::enabled_staff_search_views}; + if ( $defaultview eq 'isbd' && $views->{can_view_ISBD} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/ISBDdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); + } elsif ( $defaultview eq 'marc' && $views->{can_view_MARC} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid" + ); + } elsif ( $defaultview eq 'labeled_marc' && $views->{can_view_labeledMARC} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/labeledMARCdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); } else { - print $input->redirect("/cgi-bin/koha/catalogue/detail.pl?biblionumber=$biblionumber&searchid=$searchid"); + print $input->redirect( + "/cgi-bin/koha/catalogue/detail.pl?biblionumber=$biblionumber&searchid=$searchid"); } exit; - } - elsif ( !$encode_error && ( $redirect eq "just_save" ) ) { + } elsif ( !$encode_error && ( $redirect eq "just_save" ) ) { my $tab = $input->param('current_tab'); print $input->redirect( "/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=$biblionumber&framework=$frameworkcode&tab=$tab&searchid=$searchid" diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl index a48f4bbe0f1..ba2fe290d32 100755 --- a/cataloguing/merge.pl +++ b/cataloguing/merge.pl @@ -92,8 +92,9 @@ if ( $op eq 'cud-merge' ) { # Modifying the reference record my $invalid_metadata; + my @merge_warnings; my $modded = try { - ModBiblio($record, $ref_biblionumber, $frameworkcode); + ModBiblio( $record, $ref_biblionumber, $frameworkcode, { warnings => \@merge_warnings } ); return 1; } catch { my $exception = $_; @@ -105,6 +106,8 @@ if ( $op eq 'cud-merge' ) { return 0; }; + my ($nonxml_stripped) = grep { $_->{type} eq 'nonxml_stripped' } @merge_warnings; + my $report_header = {}; if ($modded) { foreach my $biblionumber ( $ref_biblionumber, @biblionumbers ) { @@ -159,7 +162,8 @@ if ( $op eq 'cud-merge' ) { result => 1, report_records => \@report_records, report_header => $report_header, - ref_biblionumber => scalar $input->param('ref_biblionumber') + ref_biblionumber => scalar $input->param('ref_biblionumber'), + ( $nonxml_stripped ? ( NONXML_STRIPPED => $nonxml_stripped->{message} ) : () ), ); #------------------------- diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt index 809cc3abfa4..46d4827e9a8 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt @@ -805,6 +805,18 @@
[% INVALID_METADATA | html %]
[% END %] + [% IF ( NONXML_STRIPPED ) %] +
+

Record saved with modifications

+

Non-XML characters were automatically stripped from this record before saving. The record has been saved successfully, but please review it to ensure the data is correct. If data appears to be missing or incorrect, edit + and re-save the record.

+
[% NONXML_STRIPPED | html %]
+
+ [% END %] [% IF marc21_fixlen %]

WARNING

The following fixed-length control field(s) have an invalid length: [% marc21_fixlen.failed.join(',') | html %]. Please fix.

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt index a7d22b8be6e..a567e2c42b5 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt @@ -62,7 +62,7 @@ [% IF error.code == 'CANNOT_MOVE' %] The following items could not be moved from the old record to the new one: [% error.value | html %] [% ELSIF error.code == 'INVALID_METADATA' %] -

This record had encoding issues, non-xml characters have been stripped in order to allow editing. Please be cautious when saving that some data may have been removed from the record.

+

The merged record could not be saved because it contains characters that are not valid XML. The merge has been aborted.

[% error.value | html %]
[% ELSE %] [% error | html %] @@ -73,6 +73,15 @@ [% END %] [% ELSE %]
The merge was successful. View the merged record.
+ [% IF NONXML_STRIPPED %] +
+ Non-XML characters were automatically stripped from the merged record before saving. Please review the record to ensure + the data is correct. +
[% NONXML_STRIPPED | html %]
+
+ [% END %]

Report

diff --git a/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t index 6e5af530cb7..ad3138df690 100755 --- a/t/db_dependent/Koha/Biblio/Metadata.t +++ b/t/db_dependent/Koha/Biblio/Metadata.t @@ -18,7 +18,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 6; +use Test::More tests => 7; use Test::Exception; use Test::MockModule; use Test::Warn; @@ -28,6 +28,8 @@ use t::lib::Mocks; use C4::Biblio qw( AddBiblio ); use Koha::Database; +use MARC::Record; +use MARC::Field; BEGIN { use_ok('Koha::Biblio::Metadatas'); @@ -73,40 +75,44 @@ EOX lives_ok { $record->store } 'Valid MARCXML record stores successfully'; + is( $record->stripped_nonxml, undef, 'stripped_nonxml is undef for clean record' ); $schema->storage->txn_rollback; }; - subtest 'Invalid MARCXML handling' => sub { + subtest 'MARCXML with strippable non-XML characters' => sub { + plan tests => 3; $schema->storage->txn_begin; - my $invalid_marcxml = <<'EOX'; - - - 00925nam a22002411a 4500 - 1234567 - - This string will  break your record - - - -EOX + + # Build MARCXML containing a non-XML control character (chr(31) = Unit Separator) + my $bad_title = 'Title with' . chr(31) . ' bad character'; + my $clean_title = 'Title with bad character'; + + # Generate the MARCXML string with the embedded bad character + my $marc_record = MARC::Record->new; + $marc_record->append_fields( MARC::Field->new( '245', '', '', 'a' => $bad_title ) ); + my $marcxml_with_bad_char = $marc_record->as_xml_record('MARC21'); my $record = Koha::Biblio::Metadata->new( { format => 'marcxml', - metadata => $invalid_marcxml, + metadata => $marcxml_with_bad_char, biblionumber => $biblio->{biblionumber}, schema => 'MARC21', } ); - throws_ok { $record->store } - 'Koha::Exceptions::Metadata::Invalid', - 'Invalid MARCXML throws expected exception'; + lives_ok { $record->store } 'MARCXML with strippable characters stores successfully'; + + ok( $record->stripped_nonxml, 'stripped_nonxml is set after stripping' ); + + my $stored = Koha::Biblio::Metadatas->find( { biblionumber => $biblio->{biblionumber}, format => 'marcxml' } ); + is( + $stored->record->field('245')->subfield('a'), + $clean_title, + 'Stored record has control character removed' + ); - my $thrown = $@; - ok( $thrown->decoding_error, 'Exception contains decoding error message' ); - is( $thrown->biblionumber, $biblio->{biblionumber}, 'Exception contains correct biblionumber' ); $schema->storage->txn_rollback; }; -- 2.53.0