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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207772|0 |1 is obsolete| | Attachment #207773|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 #7 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 208933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208933&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> 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 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed CC| |jonathan.druart@gmail.com --- Comment #9 from Jonathan Druart <jonathan.druart@gmail.com> --- 1. Are you sure this is correct? We are modifying data without any feedback. 2. If we want this, there is way more to do, like remove all the code that deal with this problematic records. 3. t/db_dependent/selenium/regressions.t is failing now. 16:36:27 # at t/db_dependent/selenium/regressions.t line 176. 16:36:36 Error while executing command: no such element: Unable to locate element: //p[@class="biberror"] at /usr/share/perl5/Selenium/Remote/Driver.pm line 411. 4. Author: Martin Renvoize <martin.renvoize@openfifth.co.uk> Sponsored-by: OpenFifth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> I would expect more pairs of eyes on such fundamental change (and Claude does not count :D) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |adolfo.rodriguez@xercode.es --- Comment #11 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Following up on point 2: svc/bib is one of the places that still deals with problematic records the old way, and it was not adapted here. In fetch_bib (svc/bib, line 77), when metadata_record throws Koha::Exceptions::Metadata::Invalid, it still calls $biblio->metadata->record_strip_nonxml, which this bug removed (only Koha::Authority->record_strip_nonxml remains). So: - Recoverable records never reach that branch any more, because metadata_record already returns the repaired record. As a result the invalid-metadata header is never sent, and the advanced editor (koha-backend.js, GetRecord) no longer shows "Record had encoding issues, please switch to the basic editor to view details". OAI and addbiblio.pl were adapted to check object_messages, but svc/bib was not. - Unrecoverable records now die with a call to a method that no longer exists. They already failed before, so the practical result is the same, but the error is now a missing method. A possible fix in fetch_bib would be to drop the record_strip_nonxml call (returning a proper error for unrecoverable records), and to set invalid_metadata when $biblio->metadata->object_messages is not empty, as Koha::OAI::Server::Repository->get_biblio_marcxml does. Have a nice day. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thank you Jonathan and Adolfo for taking the time to look at this, it is genuinely appreciated, and Adolfo in particular for tracing the svc/bib leftover so precisely. A bit of context that might help for next time: this work was split into a tree of bugs at the request of QA folks (35104 -> 43777 -> 43025 -> its children) so that each piece could be reviewed in isolation. 43777 has been sitting in the queue asking for eyes for quite a while, so it is a little disheartening to receive the feedback after the push rather than when it was requested. For a change like this the whole tree really does need to be read together, as several of the concerns raised here (visibility of the repairs, removal of the old handling) are deliberately delivered by the dependent bugs. If anyone has concerns about the direction, the best moment to raise them is while the bugs are in Needs Signoff / Needs QA, and I will happily wait for them. That said, the points raised are fair, and some parts really were missing from the pushed work. Taking them in turn: 1. Feedback when data is repaired: delivered by bug 43025 (edit form) and bug 43026 (detail / MARC detail pages), both now pushed. Still to come: the advanced cataloguing editor (bug 43642) and TransformHtmlToMarc (bug 43776). 2. Removing the old handling: Adolfo is right that svc/bib was missed (record_strip_nonxml is called but no longer exists, and the invalid-metadata header is never sent for repaired records). Filed as bug 44069. 3. t/db_dependent/selenium/regressions.t: the test still expects the old biberror message for a record that is now repaired on read. Filed as bug 44070. 4. Number of reviewers: fair point, and I would be glad of more eyes on the rest of the tree. Both new bugs depend on 43025 and I am working on them now, together with the remaining open bugs in the tree that do not have patches yet (43021, 43022, 43023, 43024, 43642, 43776) and rebasing those that do (43020, 43028). I would really appreciate testing and sign-offs as they arrive. Thanks again! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43777 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Update: bug 44073 now has a patch attached and is at Needs Signoff (it applies on top of bug 43776, which it depends on). -- You are receiving this mail because: You are watching all bug changes.