[Bug 43777] New: Koha::Biblio::Metadata->record should repair recoverable MARCXML faults on read
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Bug ID: 43777 Summary: Koha::Biblio::Metadata->record should repair recoverable MARCXML faults on read Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Bug 35104 stops MARCXML with non-XML characters or empty datafields from being saved, by repairing it in Koha::Biblio::Metadata->store() and recording what was fixed. Data that was already in the database before that (or that arrived by scripts or direct SQL outside Koha) can still be undecodable, and every place that reads it has to cope. The follow-up bugs for 35104 (43025, 43026, 43027, 43028, 43029, ...) are each re-implementing the same handling: eval { $metadata->record }, catch Koha::Exceptions::Metadata::Invalid, try repair_marcxml, fall back to record_strip_nonxml, then render the per-fault warnings in their own template. record_strip_nonxml also overlaps repair_marcxml, but only handles one of its two faults. Proposal: make the read path repair the same faults as store(). - Koha::Biblio::Metadata->record falls back to repair_marcxml when the stored MARCXML cannot be decoded, and returns the repaired MARC::Record. - The faults that were repaired are reported through object_messages, the same way store() reports them, so callers that want to tell the user can, and callers that do not care simply ignore them. record still returns a single value (no context-sensitive return). - Metadata that cannot be recovered still throws Koha::Exceptions::Metadata::Invalid. - The repair is not written back to the database; store() remains the only place that writes biblio_metadata_errors rows. - record_strip_nonxml is removed and its callers use record. - A shared template include renders the repair messages so the follow-up bugs do not each carry their own copy. Test plan: 1. Apply the patches on top of bug 35104. 2. prove t/db_dependent/Koha/Biblio/Metadata.t 3. Write MARCXML containing a control character (e.g. U+001F) or an empty datafield straight into biblio_metadata.metadata with SQL, then call $biblio->metadata->record and confirm a MARC::Record is returned and object_messages describes the repair. 4. Confirm unrecoverable MARCXML still throws Koha::Exceptions::Metadata::Invalid. 5. prove t/db_dependent/Biblio.t t/db_dependent/OAI/Server.t t/db_dependent/Exporter/Record.t Sponsored-by: OpenFifth -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk Depends on| |35104 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 [Bug 35104] We should warn when attempting to save MARC records that contain characters invalid in XML -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43025 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 [Bug 43025] Show metadata repair warnings in the cataloguing edit form -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43026 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43026 [Bug 43026] Show metadata repair warnings on the biblio detail and MARC detail pages -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43027 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43027 [Bug 43027] Handle undecodable metadata gracefully when merging bibliographic records -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43028 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43028 [Bug 43028] Isolate one undecodable record from aborting a batch import or revert -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43029 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43029 [Bug 43029] Guard maintenance scripts against biblios with corrupt metadata -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43776 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43776 [Bug 43776] TransformHtmlToMarc should repair and report invalid characters rather than silently stripping them -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207703&action=edit Bug 43777: Repair recoverable MARCXML faults in Koha::Biblio::Metadata->record Bug 35104 repairs non-XML characters and empty datafields when saving MARCXML. Metadata that was already corrupt before that, or that bypassed store(), still made ->record throw, so each caller had to catch Koha::Exceptions::Metadata::Invalid and re-implement its own fallback. ->record now falls back to repair_marcxml and returns the repaired MARC::Record. Each repaired fault is reported through object_messages, as store() does, so callers that want to tell the user can and the rest can ignore it. The repair is not written back to the database. Unrecoverable metadata still throws Invalid. record_strip_nonxml is now redundant (and only covered one of the two faults), so it is removed and its callers use ->record. Koha::Biblio->metadata previously built a new wrapper on every call, so messages reported by ->record were lost before they could be read. It now returns the same wrapper for as long as it wraps the same row. Test plan: 1. prove t/db_dependent/Koha/Biblio/Metadata.t 2. prove t/db_dependent/Koha/Biblio.t t/db_dependent/Biblio.t \ t/db_dependent/OAI/ t/db_dependent/Exporter/Record.t Sponsored-by: OpenFifth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43026 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43026 [Bug 43026] Show metadata repair warnings on the biblio detail and MARC detail pages -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43027 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43027 [Bug 43027] Handle undecodable metadata gracefully when merging bibliographic records -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43028 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43028 [Bug 43028] Isolate one undecodable record from aborting a batch import or revert -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43029 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43029 [Bug 43029] Guard maintenance scripts against biblios with corrupt metadata -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43776 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43776 [Bug 43776] TransformHtmlToMarc should repair and report invalid characters rather than silently stripping them -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- *** Bug 43643 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | Comma delimited| |OpenFifth list of Sponsors| | Status|Needs Signoff |Signed Off Sponsorship status|--- |Sponsored Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207703|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207737 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207737&action=edit Bug 43777: Repair recoverable MARCXML faults in Koha::Biblio::Metadata->record Bug 35104 repairs non-XML characters and empty datafields when saving MARCXML. Metadata that was already corrupt before that, or that bypassed store(), still made ->record throw, so each caller had to catch Koha::Exceptions::Metadata::Invalid and re-implement its own fallback. ->record now falls back to repair_marcxml and returns the repaired MARC::Record. Each repaired fault is reported through object_messages, as store() does, so callers that want to tell the user can and the rest can ignore it. The repair is not written back to the database. Unrecoverable metadata still throws Invalid. record_strip_nonxml is now redundant (and only covered one of the two faults), so it is removed and its callers use ->record. Koha::Biblio->metadata previously built a new wrapper on every call, so messages reported by ->record were lost before they could be read. It now returns the same wrapper for as long as it wraps the same row. Test plan: 1. prove t/db_dependent/Koha/Biblio/Metadata.t 2. prove t/db_dependent/Koha/Biblio.t t/db_dependent/Biblio.t \ t/db_dependent/OAI/ t/db_dependent/Exporter/Record.t Sponsored-by: OpenFifth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207737|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207772 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207772&action=edit Bug 43777: Tests for persisting the repair made by Koha::Biblio::Metadata->record Metadata that is repaired on read should be written back, with its error rows, so that later reads find clean metadata and the errors stay available (and dismissable) without repairing again. These tests fail until the following change. Test plan: 1. prove t/db_dependent/Koha/Biblio/Metadata.t Sponsored-by: OpenFifth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207773 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207773&action=edit Bug 43777: Repair recoverable MARCXML faults in Koha::Biblio::Metadata->record Bug 35104 repairs non-XML characters and empty datafields when saving MARCXML. Metadata that was already corrupt before that, or that bypassed store(), still made ->record throw, so each caller had to catch Koha::Exceptions::Metadata::Invalid and re-implement its own fallback. ->record now saves the repair by calling store(), which repairs the metadata, writes it back, records each fault as an error row and reports it through object_messages. The repaired MARC::Record is returned, and later reads find clean metadata, so the repair runs once per record while the errors stay available (and dismissable). If the write fails the repaired record is still returned. Unrecoverable metadata still throws Invalid. Note that the write bumps biblio_metadata.timestamp. record_strip_nonxml is now redundant (and only covered one of the two faults), so it is removed and its callers use ->record. Koha::Biblio->metadata previously built a new wrapper on every call, so messages reported by ->record were lost before they could be read. It now returns the same wrapper for as long as it wraps the same row. Test plan: Setup 1. Turn on the CataloguingLog system preference (Administration > System preferences > Logs), and set OAI-PMH to "Enable". 2. Pick three catalogue records with titles you can recognise, B with no items. Note their biblionumbers (A, B and C). 3. Damage them the way old data can be damaged, using the database (ktd --name "$KTD_INSTANCE" --dbshell): -- a stray control character in the title of A, B and C UPDATE biblio_metadata SET metadata = REPLACE(metadata, '<subfield code="a">', CONCAT('<subfield code="a">', CHAR(31))) WHERE biblionumber IN (A, B, C) AND format = 'marcxml'; -- an empty field on C as well UPDATE biblio_metadata SET metadata = REPLACE(metadata, '</record>', '<datafield tag="500" ind1=" " ind2=" "></datafield></record>') WHERE biblionumber = C AND format = 'marcxml'; Check that SELECT * FROM biblio_metadata_errors is empty for A, B and C. Editing a damaged record (used to go through record_strip_nonxml) 4. Open record A in the cataloguing editor (Edit > Edit record). => The record opens, the title is shown without the stray character, and a notice says the record was repaired. 5. Save the record without changing anything. => It saves with no error. 6. Tools > Log viewer, module Cataloguing: there is a MODIFY entry for A with the MARC before and after, and no error. 7. In the database, the stored metadata for A no longer contains the control character, and biblio_metadata_errors has a row for A (error_type nonxml_stripped, tag 245, subfield a). Deleting a damaged record 8. Delete record B (it has no items). In the Log viewer there is a DELETE entry for B that includes its MARC. Adding a record 9. Add a new, valid record. The Log viewer has an ADD entry with its MARC. OAI-PMH 10. Request record C: /cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml &identifier=<OAI identifier of C> => The record is returned, without the control character or the empty 500 field, and the request does not fail. Reading again 11. Open record C in the cataloguing editor, then again. => It opens both times. The database has error rows for C, and the stored metadata is already repaired, so nothing is repaired again. Known limit of this patch: the "repaired" notice in the editor is only shown on the read that did the repair; later bugs show it from the saved error rows. Regression check 12. Nothing in steps 4-11 produces an "Invalid metadata" error for these records, and the Log viewer has an entry for every step above. Automated 13. prove t/db_dependent/Koha/Biblio/Metadata.t 14. prove t/db_dependent/Koha/Biblio.t t/db_dependent/Biblio.t \ t/db_dependent/OAI/ t/db_dependent/Exporter/Record.t Sponsored-by: OpenFifth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org