Bug 37478

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!

Created attachment 169599 [details]
File with invalid character in 003

Created attachment 169600 [details]
File with a missing subfiled indicator

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

(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.

(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.

(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.

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.

(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.

Adding Mason:
https://wiki.koha-community.org/wiki/Release_management#Dependency_changes

Created attachment 170601 [details] [review]
Bug 37478: (follow-up) Add MARC::Lint to cpanfile

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>

Created attachment 170704 [details] [review]
Bug 37478: (follow-up) Add MARC::Lint to cpanfile

Signed-off-by: David Nind <david@davidnind.com>

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

Does it mean that the record is imported if there are errors?

If so shouldn't the strict more be the default?

(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.

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.

Yes but then --strict is confusing IMO, would not --skip-bad-records (or something similar) be more appropriate?