From 323050df7c5807d5c507ed99cf233a6a7bc8a0e4 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sat, 28 Feb 2026 21:50:59 +0000 Subject: [PATCH] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth --- t/db_dependent/Biblio.t | 35 ++++++++++------- t/db_dependent/Exporter/Record.t | 6 +-- t/db_dependent/Koha/Biblio/Metadata.t | 55 +++++++++++++++++---------- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index bca89014a23..5de58268210 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -1204,29 +1204,38 @@ subtest 'GetFrameworkCode' => sub { }; -subtest 'ModBiblio on invalid record' => sub { - plan tests => 3; +subtest 'ModBiblio on record with strippable non-XML characters' => sub { + plan tests => 4; t::lib::Mocks::mock_preference( "CataloguingLog", 1 ); - # We create a record with an onvalid control character in the MARC + # We create a record with an invalid control character (chr(31)) in the MARC my $record = MARC::Record->new(); - my $field = MARC::Field->new( '650', '', '', 'a' => '00aD000015937' ); + my $field = MARC::Field->new( '650', '', '', 'a' => '00' . chr(31) . 'aD000015937' ); $record->append_fields($field); my ($biblionumber) = C4::Biblio::AddBiblio( $record, '' ); - warning_like { C4::Biblio::ModBiblio( $record, $biblionumber, '' ); } - qr/parser error : PCDATA invalid Char value 31/, - 'Modding the biblio warns about the encoding issues'; - my $action_logs = - Koha::ActionLogs->search( { object => $biblionumber, module => 'Cataloguing', action => 'MODIFY' } ); - is( $action_logs->count, 1, "Modification of biblio was successful and recorded" ); - my $action_log = $action_logs->next; + # New behaviour: strippable non-XML characters are silently fixed; the + # stripping event is reported via the warnings arrayref, not Perl warn() + my @warnings; + C4::Biblio::ModBiblio( $record, $biblionumber, '', { warnings => \@warnings } ); + like( + $warnings[0]{message}, qr/650\$a: invalid char value 31 \(U\+001F\) at position \d+/, + 'Non-XML character stripping reported via warnings arrayref with field and char context' + ); + + my $metadata = Koha::Biblios->find($biblionumber)->metadata; + my @nonxml_errors = $metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->as_list; + ok( scalar @nonxml_errors, 'nonxml_stripped errors persisted in biblio_metadata_errors after ModBiblio' ); like( - $action_log->info, qr/parser error : PCDATA invalid Char value 31/, - "Metadata issue successfully logged in action logs" + $nonxml_errors[0]->message, qr/650\$a: invalid char value 31 \(U\+001F\) at position \d+/, + 'error message identifies field, char value, and position' ); + + my $action_logs = + Koha::ActionLogs->search( { object => $biblionumber, module => 'Cataloguing', action => 'MODIFY' } ); + is( $action_logs->count, 1, "Modification of biblio was successfully recorded in action logs" ); }; subtest 'UpdateTotalIssues on Invalid record' => sub { diff --git a/t/db_dependent/Exporter/Record.t b/t/db_dependent/Exporter/Record.t index c77c2afb73a..6d3696d2aa3 100755 --- a/t/db_dependent/Exporter/Record.t +++ b/t/db_dependent/Exporter/Record.t @@ -77,10 +77,8 @@ if ( $marcflavour eq 'UNIMARC' ) { $homebranch_subfield_code = 'a'; } -my $bad_biblio = Koha::Biblio->new()->store(); -Koha::Biblio::Metadata->new( - { biblionumber => $bad_biblio->id, format => 'marcxml', metadata => 'something wrong', schema => $marcflavour } ) - ->store(); +my $bad_biblio = $builder->build_sample_biblio; +$bad_biblio->metadata->_result->update( { metadata => 'something wrong' } ); my $bad_biblionumber = $bad_biblio->id; my $item_1_1 = $builder->build_sample_item( diff --git a/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t index 6e5af530cb7..7b0a6af48a8 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,53 @@ EOX lives_ok { $record->store } 'Valid MARCXML record stores successfully'; + is( + $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->count, + 0, 'No nonxml_stripped error for clean record' + ); $schema->storage->txn_rollback; }; - subtest 'Invalid MARCXML handling' => sub { + subtest 'MARCXML with strippable non-XML characters' => sub { + plan tests => 4; $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'; + + my @errors = $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->as_list; + ok( scalar @errors, 'nonxml_stripped error(s) recorded after stripping' ); + like( + $errors[0]->message, + qr/245\$a: invalid char value 31 \(U\+001F\) at position \d+/, + 'error message identifies field, char value, and position' + ); + + 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