Bug 17088 - Bad MARC XML can halt export_records.pl
Summary: Bad MARC XML can halt export_records.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-09 13:07 UTC by Kyle M Hall
Modified: 2018-06-04 20:10 UTC (History)
5 users (show)

See Also:
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 (1.66 KB, patch)
2016-08-09 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17088 - Bad MARC XML can halt export_records.pl (1.76 KB, patch)
2016-09-16 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17088: [Follow-up] Adjust POD in Logger.pm (905 bytes, patch)
2016-09-16 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17088: [Follow-up] Use Logger for failed exports (4.25 KB, patch)
2016-09-16 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17088 - Bad MARC XML can halt export_records.pl (1.86 KB, patch)
2016-10-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17088: [Follow-up] Adjust POD in Logger.pm (1007 bytes, patch)
2016-10-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17088: [Follow-up] Use Logger for failed exports (4.35 KB, patch)
2016-10-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17088: Add tests to cover the changes (4.79 KB, patch)
2016-10-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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