Bug 37478 - bulkmarcimport.pl can die on bad records
Summary: bulkmarcimport.pl can die on bad records
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords: dependency
Depends on:
Blocks:
 
Reported: 2024-07-25 16:37 UTC by Nick Clemens (kidclamp)
Modified: 2024-08-28 09:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds a -st strict option to misc/migration_tools/bulkmarcimport.pl. This can be used to catch any parsing errors - if errors are found, the record is checked to identify any problems, outputs warnings, and then skips the record.
Version(s) released in:
Circulation function:


Attachments
Bug 37478: Add strict mode to bulkmarcimport (2.88 KB, patch)
2024-07-25 16:44 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
File with invalid character in 003 (720 bytes, application/marc)
2024-07-25 16:45 UTC, Nick Clemens (kidclamp)
Details
File with a missing subfiled indicator (3.82 KB, application/marc)
2024-07-25 16:45 UTC, Nick Clemens (kidclamp)
Details
Bug 37478: (follow-up) Add MARC::Lint to cpanfile (688 bytes, patch)
2024-08-22 11:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37478: Add strict mode to bulkmarcimport (2.93 KB, patch)
2024-08-26 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 37478: (follow-up) Add MARC::Lint to cpanfile (734 bytes, patch)
2024-08-26 10:43 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-07-25 16:37:10 UTC
bulkmarcimport.pl will die on xml parsing errors when trying to convert the saved metadata (marcxml) back into a MARC::Record object

Two cases we have seen are an invalid characters:
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="003">aPGcC</controlfield>

and invalid/missing subfield:
:91: parser error : attributes construct error
    <subfield code=""">Among nineteenth-century women's rights reformers, Elizab


It turns out MARC::Record can read the incoming file - we can convert it to xml to save as metadata, but only on trying to create the record out of the xml do we die.

This may point to a bug in MARC::Record
Comment 1 Nick Clemens (kidclamp) 2024-07-25 16:44:13 UTC
Created attachment 169598 [details] [review]
Bug 37478: Add strict mode to bulkmarcimport

This patch adds a conversion from MARC -> XML -> MARC to catch any parsing errors.
If errors are found, we then lint the record to catch any problems, output the
warnings, and skip the record.

To test:
1 - Download the sample records from this bug report
2 - perl misc/migration_tools/bulkmarcimport.pl -b --file=520_nosubfield.mrc -v
    fails!
3 - perl misc/migration_tools/bulkmarcimport.pl -b --file=003_subfielda.mrc -v
    fails!
4 - Apply patch
5 - Repeat 2 & 3 - no change
6 - Add -st switch to the commands:
    perl misc/migration_tools/bulkmarcimport.pl -b --file=520_nosubfield.mrc -v -st
    perl misc/migration_tools/bulkmarcimport.pl -b --file=003_subfielda.mrc -v -st
7 - The records are now skipped, and the script completes
8 - Confirm the warnings generated are useful
9 - Sign off!
Comment 2 Nick Clemens (kidclamp) 2024-07-25 16:45:33 UTC
Created attachment 169599 [details]
File with invalid character in 003
Comment 3 Nick Clemens (kidclamp) 2024-07-25 16:45:56 UTC
Created attachment 169600 [details]
File with a missing subfiled indicator
Comment 4 Nick Clemens (kidclamp) 2024-07-25 17:24:21 UTC
The reason to not simply use Lint here was a matter of how sensitive it is. Lint doesn't distinguish between an error that breaks parsing vs a marc rules error like wrong indicator value or repeated field that should be singular. We worried it would be too strict
Comment 5 David Cook 2024-07-25 23:45:54 UTC
(In reply to Nick Clemens (kidclamp) from comment #0)
> It turns out MARC::Record can read the incoming file - we can convert it to
> xml to save as metadata, but only on trying to create the record out of the
> xml do we die.
> 
> This may point to a bug in MARC::Record

This sounds very familiar. I remember talking to someone (maybe even you heh) or commenting on a bug somewhere about invalid XML being produced by MARC::Record...

I recall that MARC::File::XML has some shockingly bad code when it comes to creating XML. If you look at the source code on MetaCPAN, you'll see that it actually recreates the record as a string, and it only escapes &, <, and >. 

So yeah I'd certainly say a bug in MARC::File::XML.
Comment 6 David Cook 2024-07-25 23:48:47 UTC
(In reply to David Cook from comment #5)
> So yeah I'd certainly say a bug in MARC::File::XML.

I'll raise 2 issues in Galen's marc-perl git repo for the attributes issue and the invalid characters.
Comment 7 David Cook 2024-07-25 23:51:59 UTC
(In reply to Nick Clemens (kidclamp) from comment #0)
> Two cases we have seen are an invalid characters:
> :9: parser error : PCDATA invalid Char value 31
>   <controlfield tag="003">aPGcC</controlfield>
> 
> and invalid/missing subfield:
> :91: parser error : attributes construct error
>     <subfield code=""">Among nineteenth-century women's rights reformers,
> Elizab

Of course, parser errors are often how we find out that we have bad metadata... oh well. I'll raise the issues anyway.
Comment 8 David Nind 2024-07-31 20:57:56 UTC
Getting this error after applying the patch for step 5 - assuming this means the Perl MARC::Lint module is not installed and needs adding (however that is done):

perl misc/migration_tools/bulkmarcimport.pl -b --file=003_subfielda.mrc -v
Can't locate MARC/Lint.pm in @INC (you may need to install the MARC::Lint module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.36.0 /usr/local/share/perl/5.36.0 /usr/lib/x86_64-linux-gnu/perl5/5.36 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.36 /usr/share/perl/5.36 /usr/local/lib/site_perl) at misc/migration_tools/bulkmarcimport.pl line 12.
BEGIN failed--compilation aborted at misc/migration_tools/bulkmarcimport.pl line 12.
Comment 9 David Cook 2024-07-31 23:07:34 UTC
(In reply to David Nind from comment #8)
> Getting this error after applying the patch for step 5 - assuming this means
> the Perl MARC::Lint module is not installed and needs adding (however that
> is done):

Bingo! Marking Failed QA as we'll need it on the cpanfile at least I believe.
Comment 10 Katrin Fischer 2024-08-01 07:34:59 UTC
Adding Mason:
https://wiki.koha-community.org/wiki/Release_management#Dependency_changes
Comment 11 Nick Clemens (kidclamp) 2024-08-22 11:27:18 UTC
Created attachment 170601 [details] [review]
Bug 37478: (follow-up) Add MARC::Lint to cpanfile
Comment 12 David Nind 2024-08-26 10:43:49 UTC
Created attachment 170703 [details] [review]
Bug 37478: Add strict mode to bulkmarcimport

This patch adds a conversion from MARC -> XML -> MARC to catch any parsing errors.
If errors are found, we then lint the record to catch any problems, output the
warnings, and skip the record.

To test:
1 - Download the sample records from this bug report
2 - perl misc/migration_tools/bulkmarcimport.pl -b --file=520_nosubfield.mrc -v
    fails!
3 - perl misc/migration_tools/bulkmarcimport.pl -b --file=003_subfielda.mrc -v
    fails!
4 - Apply patch
5 - Repeat 2 & 3 - no change
6 - Add -st switch to the commands:
    perl misc/migration_tools/bulkmarcimport.pl -b --file=520_nosubfield.mrc -v -st
    perl misc/migration_tools/bulkmarcimport.pl -b --file=003_subfielda.mrc -v -st
7 - The records are now skipped, and the script completes
8 - Confirm the warnings generated are useful
9 - Sign off!

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2024-08-26 10:43:52 UTC
Created attachment 170704 [details] [review]
Bug 37478: (follow-up) Add MARC::Lint to cpanfile

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2024-08-26 10:53:43 UTC
Testing notes (using KTD):

1. After applying the patch, and to get the new Perl module to install, I:
   1.1 Shutdown KTD
   1.2 Edited my .env file so that: INSTALL_MISSING_FROM_CPANFILE=yes
   1.3 Restarted KTD
Comment 15 Jonathan Druart 2024-08-27 11:29:49 UTC
Does it mean that the record is imported if there are errors?

If so shouldn't the strict more be the default?
Comment 16 Jonathan Druart 2024-08-27 11:31:30 UTC
(In reply to Jonathan Druart from comment #15)
> Does it mean that the record is imported if there are errors?
> 
> If so shouldn't the strict more be the default?

Ha, it dies, sorry.

Then isn't it less... strict? I feel like I am missing something, just ignore me if I am not making sense.
Comment 17 David Nind 2024-08-27 23:25:46 UTC
My understanding (limited as it is) is that it skips "bad" records if you use the new --strict option, instead of dying/failing.

Which I guess means that your import will work, except for the "bad" records.
Comment 18 Jonathan Druart 2024-08-28 09:11:40 UTC
Yes but then --strict is confusing IMO, would not --skip-bad-records (or something similar) be more appropriate?