Bug 17088

Summary: Bad MARC XML can halt export_records.pl
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Command-line UtilitiesAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: brendan, f.demians, julian.maurice, m.de.rooy, robin
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17196
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 17088 - Bad MARC XML can halt export_records.pl
Bug 17088 - Bad MARC XML can halt export_records.pl
Bug 17088: [Follow-up] Adjust POD in Logger.pm
Bug 17088: [Follow-up] Use Logger for failed exports
Bug 17088 - Bad MARC XML can halt export_records.pl
Bug 17088: [Follow-up] Adjust POD in Logger.pm
Bug 17088: [Follow-up] Use Logger for failed exports
Bug 17088: Add tests to cover the changes

Description Kyle M Hall 2016-08-09 13:07:17 UTC
If export_records.pl encounters a record that fails to load correctly as a MARC::Record the export process will fail and only the records encountered up to that point will be exported.
Comment 1 Kyle M Hall 2016-08-09 13:12:32 UTC
Created attachment 54202 [details] [review]
Bug 17088 - Bad MARC XML can halt export_records.pl

If export_records.pl encounters a record that fails to load correctly as
a MARC::Record the export process will fail and only the records
encountered up to that point will be exported.

Test Plan:
1) Pick a record in your database, and replace the biblioitems.marcxml field with some random characters
2) Export your records using the command:
   misc/export_records.pl  --format xml --record-type bibs --filename /tmp/biblios.xml
3) Note you get an error and the script ends prematurely
4) Apply this patch
5) Repeat step 2
6) Note the full record set is exported ( except for your bad record )
7) Note an error is still generated for your bad record ( useful for
    tracking down bad records ).
Comment 2 Marcel de Rooy 2016-09-16 09:31:23 UTC
Testing this now.
Comment 3 Marcel de Rooy 2016-09-16 11:44:08 UTC
Created attachment 55609 [details] [review]
Bug 17088 - Bad MARC XML can halt export_records.pl

If export_records.pl encounters a record that fails to load correctly as
a MARC::Record the export process will fail and only the records
encountered up to that point will be exported.

Test Plan:
1) Pick a record in your database, and replace the biblioitems.marcxml field with some random characters
2) Export your records using the command:
   misc/export_records.pl  --format xml --record-type bibs --filename /tmp/biblios.xml
3) Note you get an error and the script ends prematurely
4) Apply this patch
5) Repeat step 2
6) Note the full record set is exported ( except for your bad record )
7) Note an error is still generated for your bad record ( useful for
    tracking down bad records ).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2016-09-16 11:44:11 UTC
Created attachment 55610 [details] [review]
Bug 17088: [Follow-up] Adjust POD in Logger.pm

Not important enough to go on its own.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2016-09-16 11:44:15 UTC
Created attachment 55611 [details] [review]
Bug 17088: [Follow-up] Use Logger for failed exports

Fixes a TODO for logging unsupported record_type in _get_record_for_export.
Also logs a warning when the record_type parameter is not supplied at all in sub export.
Replaces a warn by a log message about an invalid record for format iso2709.
Also adds a log message about an invalid record for format xml.
Adds a trivial unit test for passing no record_type to export.

Test plan:
See also first patch.
Run t/db_dependent/Exporter/Record.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested the log messages for iso2709 and xml by manipulating
the record_type parameter with:
    $params->{record_type}='xx';
before calling _get_record_for_export in Record.pm.
Comment 6 Marcel de Rooy 2016-09-16 11:45:47 UTC
Kyle: Added follow-up for logging. If you prefer to let them go on their own, feel free to move them. They are quite simple however.
Comment 7 Jonathan Druart 2016-10-04 13:50:06 UTC
Created attachment 56032 [details] [review]
Bug 17088 - Bad MARC XML can halt export_records.pl

If export_records.pl encounters a record that fails to load correctly as
a MARC::Record the export process will fail and only the records
encountered up to that point will be exported.

Test Plan:
1) Pick a record in your database, and replace the biblioitems.marcxml field with some random characters
2) Export your records using the command:
   misc/export_records.pl  --format xml --record-type bibs --filename /tmp/biblios.xml
3) Note you get an error and the script ends prematurely
4) Apply this patch
5) Repeat step 2
6) Note the full record set is exported ( except for your bad record )
7) Note an error is still generated for your bad record ( useful for
    tracking down bad records ).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2016-10-04 13:50:10 UTC
Created attachment 56033 [details] [review]
Bug 17088: [Follow-up] Adjust POD in Logger.pm

Not important enough to go on its own.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2016-10-04 13:50:14 UTC
Created attachment 56034 [details] [review]
Bug 17088: [Follow-up] Use Logger for failed exports

Fixes a TODO for logging unsupported record_type in _get_record_for_export.
Also logs a warning when the record_type parameter is not supplied at all in sub export.
Replaces a warn by a log message about an invalid record for format iso2709.
Also adds a log message about an invalid record for format xml.
Adds a trivial unit test for passing no record_type to export.

Test plan:
See also first patch.
Run t/db_dependent/Exporter/Record.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested the log messages for iso2709 and xml by manipulating
the record_type parameter with:
    $params->{record_type}='xx';
before calling _get_record_for_export in Record.pm.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2016-10-04 13:50:21 UTC
Created attachment 56035 [details] [review]
Bug 17088: Add tests to cover the changes

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Brendan Gallagher 2016-10-10 12:27:09 UTC
Pushed to Master - Should be in the November Release 16.11 - Thanks!
Comment 12 Frédéric Demians 2016-10-22 10:39:29 UTC
Pushed in 16.05. Will be in 16.05.05.
Comment 13 Julian Maurice 2016-10-24 09:44:39 UTC
Pushed to 3.22.x, will be in 3.22.12