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. 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? Created attachment 173831 [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> Current status: Signed Off Created attachment 173832 [details] [review] Bug 37478: (follow-up) Add MARC::Lint to cpanfile Signed-off-by: David Nind <david@davidnind.com> Created attachment 173833 [details] [review] Bug 37478: Rename strict to skip_bad_records and add POD Option name changed, POD updated. The reason not to do this by default is that it is an extra record conversion to check the validity, so may slow down the import. Created attachment 173924 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173925 [details] [review] Bug 37478: (follow-up) Add MARC::Lint to cpanfile Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173926 [details] [review] Bug 37478: Rename strict to skip_bad_records and add POD Option name changed, POD updated. The reason not to do this by default is that it is an extra record conversion to check the validity, so may slow down the import. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> We have a new dependeny here - we need Mason to give his OK first. (In reply to Katrin Fischer from comment #25) > We have a new dependeny here - we need Mason to give his OK first. great news, libmarclint-perl package is available in debian repo so, all ok :) https://tracker.debian.org/pkg/libmarc-lint-perl Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.06 This script changed a lot since Bug 29440 so this does not apply easy on 23.11.x I propose to not backport for stability |
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