Bugzilla – Attachment 194226 Details for
Bug 35104
We should warn when attempting to save MARC records that contain characters invalid in XML
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 35104: Update and extend tests to reflect graceful non-XML stripping
323050d.patch (text/plain), 8.04 KB, created by
Martin Renvoize (ashimema)
on 2026-03-01 16:23:54 UTC
(
hide
)
Description:
Bug 35104: Update and extend tests to reflect graceful non-XML stripping
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-01 16:23:54 UTC
Size:
8.04 KB
patch
obsolete
>From 323050df7c5807d5c507ed99cf233a6a7bc8a0e4 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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'; >-<?xml version="1.0" encoding="UTF-8"?> >-<record xmlns="http://www.loc.gov/MARC21/slim"> >- <leader>00925nam a22002411a 4500</leader> >- <controlfield tag="001">1234567</controlfield> >- <datafield tag="245" ind1="1" ind2="0"> >- <subfield code="a">This string will break your record</subfield> >- </datafield> >- <!-- Invalid XML structure --> >-</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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 35104
:
157625
|
157627
|
157628
|
157642
|
159615
|
159620
|
159621
|
159630
|
159810
|
159811
|
159812
|
159813
|
166431
|
173145
|
173146
|
176820
|
176821
|
176850
|
176851
|
176852
|
176860
|
193985
|
193986
|
193987
|
193988
|
193989
|
193990
|
193991
|
193992
|
193993
|
193994
|
193995
|
193996
|
193997
|
193998
|
193999
|
194000
|
194001
|
194004
|
194005
|
194006
|
194007
|
194008
|
194009
|
194010
|
194011
|
194012
|
194013
|
194152
|
194153
|
194154
|
194155
|
194156
|
194157
|
194158
|
194159
|
194160
|
194161
|
194162
|
194163
|
194164
|
194165
|
194166
|
194167
|
194168
|
194169
|
194170
|
194171
|
194172
|
194213
|
194214
|
194215
|
194216
|
194217
|
194218
|
194219
|
194220
|
194221
|
194222
|
194223
|
194224
|
194225
| 194226 |
194227