Bug 34549 adds some defensive coding to prevent incorrectly encoded MARC data getting into the database.. but is does so by silently stripping bad characters from the record. It would be nice to highlight that this is taking place, or preferably before it takes place so the end user understands what's happening.
When I was writing bug 34549, I thought a little bit about how this might be done. Whether it was better to do it by Javascript on the page, an AJAX call, or do it on page submission server side...
But first a test... I change the Library of Congress Z39.50 from utf8 to "ISO_8859-1" (note changing to MARC-8 didn't seem to make a difference), and then I search "bibliothecaire" for the Title. I quickly notice obvious encoding problems like "Fonction peÌdagogique du documentaliste-bibliotheÌcaire" So I go to import that one... and actually I don't have any problems. I get a record with the following title: "Fonction peÌdagogique du documentaliste-bibliotheÌcaire : journeÌe acadeÌmique des documentalistes-bibliotheÌcaires, le 7 feÌvrier 1977" I suppose it's because Ì and are valid UTF-8 characters. If I change the Z39.50 back to "utf8" then I get: "Fonction pédagogique du documentaliste-bibliothécaire :" I'm not sure why é would be interpreted as Ì though. If I download as MARCXML, I see the title is "Fonction pédagogique du documentaliste-bibliothécaire :" I guess I'm not confident how many conversions are happening and at which points but this still looks odd...
As per my comment at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34549#c19 technically the problem we're trying to solve in bug 34549 is preventing the saving of characters that cannot be rendered in XML. Picking up on encoding problems is a lot trickier because bytes can be valid in multiple different encodings. The machine can still output a valid character, so it doesn't realize that the source data is encoded incorrectly.
Created attachment 157625 [details] Text file containing control characters
I've written a Javascript based check which *mostly* gets us there. It checks the input[text] and textarea fields for bytes that aren't allowed in XML and it raises an alert if it finds any. I haven't figured out a way to get the "Go to field" to work though, as I need the tab where the HTML element is found, but we aren't recording that data anywhere at the moment. We have a "tabindex", but it seems hard-coded to 0 or 1 for some reason... I would attach my patch here on Bugzilla, but "git-bz" isn't working for me right now... I've run out of steam on this one anyway, but hopefully I can share my code and someone else can take it away...
Created attachment 157627 [details] [review] Bug 34549: Alert when inserting text invalid in XML into bib record
git-bz still not working for me, so just used "git format-patch -1 HEAD" and uploaded manually...
Created attachment 157628 [details] [review] Bug 35104: Alert when inserting text invalid in XML into bib record This change adds a Javascript based alert when a user tries to save data via the bib record editor that is invalid in XML. Test plan: 0. Apply patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode= 2. Copy the data from "Text file containing control characters" 3. Paste the data into 245$a and 500$a 4. Click "Save" 5. Note the alerts at the top of the web page and that the record does not save 6. Resolve the problems 7. Click "Save" 8. Note that the record saves
Created attachment 157642 [details] [review] Bug 35104: Alert when inserting text invalid in XML into bib record This change adds a Javascript based alert when a user tries to save data via the bib record editor that is invalid in XML. Test plan: 0. Apply patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode= 2. Copy the data from "Text file containing control characters" 3. Paste the data into 245$a and 500$a 4. Click "Save" 5. Note the alerts at the top of the web page and that the record does not save 6. Resolve the problems 7. Click "Save" 8. Note that the record saves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works really nicely, as such I'm signing off... Two little points however.. 1) I wonder if we could/should merge these errors into the existing validation alert rather than having it's own alert box? 2) I did consider whether we should go belt & braces and catch on the server too.. however I think this is a great improvement without that anyway.
(In reply to David Cook from comment #3) > As per my comment at > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34549#c19 > technically the problem we're trying to solve in bug 34549 is preventing the > saving of characters that cannot be rendered in XML. > > Picking up on encoding problems is a lot trickier because bytes can be valid > in multiple different encodings. The machine can still output a valid > character, so it doesn't realize that the source data is encoded incorrectly. You're totally right there.. it's really hard to spot encoding issues becuase of this :(
While I appreciate the sign off, it's not quite ready yet. It doesn't calculate the tab currently for the "Go to field" link. It only works by accident for things like the 245 due to "tabindex" weirdness.
(In reply to Martin Renvoize from comment #10) > This works really nicely, as such I'm signing off... > > Two little points however.. > > 1) I wonder if we could/should merge these errors into the existing > validation alert rather than having it's own alert box? That was my original intention but that would involve more refactoring. Could be worth doing though. > 2) I did consider whether we should go belt & braces and catch on the server > too.. however I think this is a great improvement without that anyway. I was pondering that too. Probably worth doing.
I had an interesting experience that I thought I'd share here since it's relevant. One of my librarians was copying and pasting text from a PDF into Koha. When they did it, it generated a broken MARC record. When I did it, it worked fine. That's when I discovered that Chrome, Firefox, and Edge all treat PDF text differently. Consider the following phrase "like-minded people" which is actually broken against 2 lines on the hyphen in the PDF. If you copy from Chrome, it removes the hyphen so it becomes "likeminded people". If you copy from Firefox, it copies the hyphen and a line break so it becomes something like "like-\nminded people". If you copy from Edge, it mangles the hyphen and turns it into a "Start of text" control character, which will break Koha. -- While we work through our official solution, I came up with a little Javascript function which is run during the "paste" event in the MARC editor. It displays a "confirm" box which contains an explanatory warning, offers a tip on understanding the problem, and then provides an option to try to "fix" the record (by stripping out the bad characters). I'm just running that in their "IntranetUserJS" for now, but I thought I'd share this information. I think having a warning at "save time" is very useful especially for imported records. This "on paste" warning might also be useful though since it's at the time that they're entering the data, so it might be easier for them to notice the problem. -- In any case, it was great to find an example from the wild to test a fix against...
Wow, that's an impressive find. Man this stuff ends up in fun "exciting" places.
(In reply to Martin Renvoize from comment #15) > Wow, that's an impressive find. Man this stuff ends up in fun "exciting" > places. Thanks! I did feel pretty good working that one out! During this process, I've been thinking there might be an alternative to bug 34549 as well. Instead of erasing the invalid characters, surely we could just escape them. So I'm going to use my discovery from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104#c14 to see if I can do an alternate version of bug 34549 where we escape the bad characters using numeric character references or an entity. So the STX character from that example would be converted to something like  instead of just erased. Of course, it would be good to try it out with a variety of examples and not just this sort of data corruption example. But maybe it's also one of those ones where we can just do our best and see how it evolves over time...
(In reply to David Cook from comment #16) > So the STX character from that example would be converted to something like >  instead of just erased. I just tried a client-side replace where the STX character gets converted into  but it looks like it gets converted into &#2; Something I find interesting is that in the database we'll store "David’s awesome record" but then the MARCXML export to the browser will encode the UTF-8 encoded character ’ as ’ I'm going to poke around in this a bit more...
(In reply to David Cook from comment #17) > I'm going to poke around in this a bit more... The TransformHtmlToMarc doesn't seem to affect it... If I do $record->as_formatted then I see: likeminded If I do $record->as_xml then I see: &#2; Looking at https://metacpan.org/dist/MARC-File-XML/source/lib/MARC/File/XML.pm#L378 there is an escape function that escapes ampersands and angle brackets. In theory, maybe MARC::File::XML should escape any invalid characters using character references or remove them since they're invalid. But MARC::File::XML's escaping means it's impossible for us to pre-escape any invalid characters. It feels like MARC::File::XML is essentially holding us hostage. We need to clean our input data (in whatever format) before it reaches MARC::File::XML, which seems a bit silly, since it's the XML format which has the restrictions... That being said... the XML 1.0 spec is pretty forgiving. After review, it's really just excluding *some* ASCII control characters, Unicode surrogates, U+FFFE, and U+FFFF. That's a really small number of characters and none of them are printable characters.
(In reply to David Cook from comment #18) > That being said... the XML 1.0 spec is pretty forgiving. After review, it's > really just excluding *some* ASCII control characters, Unicode surrogates, > U+FFFE, and U+FFFF. That's a really small number of characters and none of > them are printable characters. After review, for a UTF-8 encoded document, bug 34549 would only strip non-printable characters. I might have another crack at trying to get a Latin-1 encoded document into Koha... as I know that I've had Latin-1 encoded data in Koha before (although it's very possible that it came through side-loading using non-Koha tools).
We've got an interesting assortment of fixes at the moment... If you already have a record with invalid characters, you can open it using bug 34014 which will scrub your record clean within the editor, but it will also show you the parser errors, so you could go and repair your record, in theory. With bug 34549, you can try to save invalid characters, and it will silently wipe them out. With bug 35104, I've been looking at a client-side warning, but it doesn't tell you exactly which characters are a problem. Maybe we'd be better off re-using the mechanism from bug 34014 here in bug 35104. That would remove the need for a client-side check, it would be more consistent, and then we could undo bug 34549...
Created attachment 159615 [details] [review] Bug 35104: [Alternative] Throw exception on store of invalid marcxml This patch adds an exception into ModBilioMarc to prevent erronious marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source.
I think we could perhaps go a level lower and put this exception into Koha::Biblio::Metadata->store maybe instead? We'll also need to account for the exception at various places with a try/catch block as detailed in the commit message.. but I wanted to start getting feedback on the proposed approach before committing to continuing down this path.
Created attachment 159620 [details] [review] Bug 35104: [Alternative] Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source.
Created attachment 159621 [details] [review] Bug 35104: Initial attempt at catching the failure on new records Whilst I can see the warning output from store, I can't seem to actually catch any exceptions.. confused
Thanks for working on this, Martin. I'll try to take a look in a bit.
I've applied the last 2 patches and restarted Koha... Bug 35104: [Alternative] Throw exception on store of invalid marcxml Bug 35104: Initial attempt at catching the failure on new records And then I added the text from "Text file containing control characters" into a test record -- I'm getting an error trace page. It looks like you made a little mistake when catching the exception and it's causing you to rethrow it. I'll fix that up in a sec... -- However, with the fix, we're still having a problem rendering the page. But I'll let folk take it from there...
Created attachment 159630 [details] [review] Bug 35104: Fix typo when catching exception
I have to run and I'm away tomorrow morning, but hopefully this gets you a bit further.
Awesome.. I knew it was something silly when catching the exception! That gives me enough to get going again :), thanks David
(In reply to Martin Renvoize from comment #29) > Awesome.. I knew it was something silly when catching the exception! > > That gives me enough to get going again :), thanks David Yay teamwork! :D
Created attachment 159810 [details] [review] Bug 35104: [Alternative] Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source.
Created attachment 159811 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again.
Created attachment 159812 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source.
Created attachment 159813 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again.
IMO this is too low level, we should not call MARC::Record::new_from_xml everytime we store. It will work ofc but what about perf? What if I do want invalid marcxml? :D
(In reply to Jonathan Druart from comment #35) > IMO this is too low level, we should not call MARC::Record::new_from_xml > everytime we store. > > It will work ofc but what about perf? I think that we should validate each time we store. However... maybe we could use XML::LibXML directly instead of MARC::Record to reduce some overhead. I don't know if it makes an actual difference in terms of perf though. And the nice thing about using MARC::Record is we know if it will break on subsequent usage. Perhaps more importantly, these patches don't add error handling for every instance of Koha::Biblio::Metadata->store(). We might be breaking Staged MARC imports here and not realizing it. So maybe we should add a Koha::Biblio::Metadata->validate() and just call it from the controller script for now? > What if I do want invalid marcxml? :D :P
Created attachment 166431 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: baptiste <baptiste.bayche@inlibro.com>
Thanks for the follow-up here Baptiste.. it's great to see someone is still interested in this. I'll continue to add such catches elsewhere as it seems people do actually want to help pursue it.
We should have at least one unit test for this. I was thinking that maybe we should move the code into a validate() function and call that from store() but it doesn't matter too much, as we can unit test it in store() just fine too. Martin, if you want to add that unit test, I could look at QAing this. Invalid metadata is a pet peeve of mine, so I'd love to see this (and related changes) get in.
Getting sha1 errors. Can you post a branch on github?
I too get the sha1 error: Applying: Bug 35104: Throw exception on store of invalid marcxml Applying: Bug 35104: Catch the failure on new and edited records error: sha1 information is lacking or useless (C4/Biblio.pm). error: could not build fake ancestor Patch failed at 0001 Bug 35104: Catch the failure on new and edited records -- Interestingly, the first patch applied but not the second one. If you dig into it, you can see that the first index referenced here doesn't exist in the upstream git: diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 0d6feb8aed..eb79c397a5 100644 Since there is over 3 hours between commits, the second one must've been based off a local index state for C4/Biblio.pm. Funny enough, the other file changed does have an index from the upstream: a11711afd3 Just needs a rebase.
Created attachment 173145 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: baptiste <baptiste.bayche@inlibro.com>
Created attachment 173146 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: baptiste <baptiste.bayche@inlibro.com>
Rebased.. I started working on other cases but ran out of time for this.. there's a lot of calls to ModBiblio in Koha that would benefit from try/catch treatment after this and propagating the errors to screen or gracefully skipping over them.
Created attachment 176820 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: baptiste <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176821 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: baptiste <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Isn't there risk to generate internal server error outside of the addbiblio.pl ?
(In reply to Baptiste Wojtkowski (bwoj) from comment #46) > Created attachment 176821 [details] [review] [review] > Bug 35104: Catch the failure on new and edited records > > We now catch the error and display it to the end user so they can fix > the issues before attempting to save again. > > Signed-off-by: baptiste <baptiste.bayche@inlibro.com> > Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Can we fix baptiste (1) sign-off line to save me a mailmap mapping please? :)
Created attachment 176850 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176851 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176852 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176860 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data.
So.. we'll have other failing tests.. t/db_dependant/Biblio.t for example.. this is because we're actually testing that putting bad characters into the database should succeed and with this patchset it doesn't.. given that's the idea of this patchset.. to prevent bad data ever making it's way into the database I'm not sure how to proceed. We can continue to add tests for bad data to ensure we capture it and deal with it when reading data out (should someone decided to put it in directly using database queries).. but should we continue to allow ModBiblio to "press on regardless".. personally I think not.
Is SO the correct state ?
Created attachment 193985 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 193986 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 193987 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 193988 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data.
Created attachment 193989 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records.
Created attachment 193990 [details] [review] Bug 35104: Persist nonxml_stripped on biblio_metadata, show on detail pages Add a nonxml_stripped column to biblio_metadata so that the automatic stripping of non-XML characters during store() survives beyond the current request. The column is set to the XML parse error message when stripping was needed, and cleared to NULL when the record is saved cleanly, so the warning disappears once the cataloguer has reviewed and re-saved the record. Show a warning alert on the biblio detail page and MARC detail page whenever nonxml_stripped is set, with a direct link to the edit form. Fix Koha::Import::Record::get_marc_record() to fall back to StripNonXmlChars when the raw staged MARCXML cannot be parsed, so that the import preview (showmarc.pl?importid=...) no longer returns a 404 for records that contain non-XML characters.
Created attachment 193991 [details] [review] Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file.
Created attachment 193992 [details] [review] Bug 35104: Update Biblio.t to reflect graceful non-XML stripping behaviour The ModBiblio subtest was written for the old exception-throwing behaviour where invalid MARC would emit a Perl warn() and the error would appear in the action log. With the graceful stripping introduced by this bug: - StripNonXmlChars recovers the record silently (no Perl warn()) - The stripping event is reported via the warnings arrayref in $options - nonxml_stripped is persisted on biblio_metadata - ModBiblio still succeeds and logs the modification to action logs Update the three assertions accordingly.
Created attachment 193993 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 193994 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 193995 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 193996 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 193997 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. Sponsored-by: OpenFifth
Created attachment 193998 [details] [review] Bug 35104: Persist nonxml_stripped on biblio_metadata, show on detail pages Add a nonxml_stripped column to biblio_metadata so that the automatic stripping of non-XML characters during store() survives beyond the current request. The column is set to the XML parse error message when stripping was needed, and cleared to NULL when the record is saved cleanly, so the warning disappears once the cataloguer has reviewed and re-saved the record. Show a warning alert on the biblio detail page and MARC detail page whenever nonxml_stripped is set, with a direct link to the edit form. Fix Koha::Import::Record::get_marc_record() to fall back to StripNonXmlChars when the raw staged MARCXML cannot be parsed, so that the import preview (showmarc.pl?importid=...) no longer returns a 404 for records that contain non-XML characters. Sponsored-by: OpenFifth
Created attachment 193999 [details] [review] Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. Sponsored-by: OpenFifth
Created attachment 194000 [details] [review] Bug 35104: Update Biblio.t to reflect graceful non-XML stripping behaviour The ModBiblio subtest was written for the old exception-throwing behaviour where invalid MARC would emit a Perl warn() and the error would appear in the action log. With the graceful stripping introduced by this bug: - StripNonXmlChars recovers the record silently (no Perl warn()) - The stripping event is reported via the warnings arrayref in $options - nonxml_stripped is persisted on biblio_metadata - ModBiblio still succeeds and logs the modification to action logs Update the three assertions accordingly. Sponsored-by: OpenFifth
Created attachment 194001 [details] [review] Bug 35104: DBIC Scheme Update Patch from commit 7762ba6
Created attachment 194004 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194005 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194006 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194007 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 194008 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. Sponsored-by: OpenFifth
Created attachment 194009 [details] [review] Bug 35104: Persist nonxml_stripped on biblio_metadata, show on detail pages Add a nonxml_stripped column to biblio_metadata so that the automatic stripping of non-XML characters during store() survives beyond the current request. The column is set to the XML parse error message when stripping was needed, and cleared to NULL when the record is saved cleanly, so the warning disappears once the cataloguer has reviewed and re-saved the record. Show a warning alert on the biblio detail page and MARC detail page whenever nonxml_stripped is set, with a direct link to the edit form. Fix Koha::Import::Record::get_marc_record() to fall back to StripNonXmlChars when the raw staged MARCXML cannot be parsed, so that the import preview (showmarc.pl?importid=...) no longer returns a 404 for records that contain non-XML characters. Sponsored-by: OpenFifth
Created attachment 194010 [details] [review] Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. Sponsored-by: OpenFifth
Created attachment 194011 [details] [review] Bug 35104: Update Biblio.t to reflect graceful non-XML stripping behaviour The ModBiblio subtest was written for the old exception-throwing behaviour where invalid MARC would emit a Perl warn() and the error would appear in the action log. With the graceful stripping introduced by this bug: - StripNonXmlChars recovers the record silently (no Perl warn()) - The stripping event is reported via the warnings arrayref in $options - nonxml_stripped is persisted on biblio_metadata - ModBiblio still succeeds and logs the modification to action logs Update the three assertions accordingly. Sponsored-by: OpenFifth
Created attachment 194012 [details] [review] Bug 35104: DBIC Scheme Update Sponsored-by: OpenFifth
Created attachment 194013 [details] [review] Bug 35104: Update Exporter/Record.t to bypass store() validation The test created a bad biblio_metadata record by calling store() with invalid MARCXML, which now throws Koha::Exceptions::Metadata::Invalid instead of storing silently. Use _result->update() to inject the bad data directly into the DB, bypassing the store() validation layer, as done elsewhere in the test suite for testing behaviour with corrupt metadata.
A good way to test this is to attempt to import the file from bug 41927 in every way you can think of and confirm a) it imports and b) it shows the error in the UI for the biblio view.
*** Bug 41927 has been marked as a duplicate of this bug. ***
I'm keen to move this one along but can we get a consolidated test plan?
In cataloguing/addbiblio.pl why is "elsif ($encode_error)" at the bottom? Wouldn't it make more sense to put that near the top of the if/elsif and that way you could avoid adding all the "!$encode_error" condition to the other elsif statements?
Hmm I see the merit of having "nonxml_stripped" as a DB column but it's a bit weird to have it in the "biblio_metadata" table since the metadata in that table doesn't have to be XML theoretically. But then I don't know about making another DB table like "biblio_metadata_errors"...
Created attachment 194152 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194153 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194154 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194155 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 194156 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. Sponsored-by: OpenFifth
Created attachment 194157 [details] [review] Bug 35104: Persist nonxml_stripped on biblio_metadata, show on detail pages Add a nonxml_stripped column to biblio_metadata so that the automatic stripping of non-XML characters during store() survives beyond the current request. The column is set to the XML parse error message when stripping was needed, and cleared to NULL when the record is saved cleanly, so the warning disappears once the cataloguer has reviewed and re-saved the record. Show a warning alert on the biblio detail page and MARC detail page whenever nonxml_stripped is set, with a direct link to the edit form. Fix Koha::Import::Record::get_marc_record() to fall back to StripNonXmlChars when the raw staged MARCXML cannot be parsed, so that the import preview (showmarc.pl?importid=...) no longer returns a 404 for records that contain non-XML characters. Sponsored-by: OpenFifth
Created attachment 194158 [details] [review] Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. Sponsored-by: OpenFifth
Created attachment 194159 [details] [review] Bug 35104: Update Biblio.t to reflect graceful non-XML stripping behaviour The ModBiblio subtest was written for the old exception-throwing behaviour where invalid MARC would emit a Perl warn() and the error would appear in the action log. With the graceful stripping introduced by this bug: - StripNonXmlChars recovers the record silently (no Perl warn()) - The stripping event is reported via the warnings arrayref in $options - nonxml_stripped is persisted on biblio_metadata - ModBiblio still succeeds and logs the modification to action logs Update the three assertions accordingly. Sponsored-by: OpenFifth
Created attachment 194160 [details] [review] Bug 35104: DBIC Scheme Update Sponsored-by: OpenFifth
Created attachment 194161 [details] [review] Bug 35104: Update Exporter/Record.t to bypass store() validation The test created a bad biblio_metadata record by calling store() with invalid MARCXML, which now throws Koha::Exceptions::Metadata::Invalid instead of storing silently. Use _result->update() to inject the bad data directly into the DB, bypassing the store() validation layer, as done elsewhere in the test suite for testing behaviour with corrupt metadata.
Created attachment 194162 [details] [review] Bug 35104: Automated Schema Update Patch from commit 0ca3840
Created attachment 194163 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Patch from commit 92f76df
Created attachment 194164 [details] [review] Bug 35104: Replace nonxml_stripped column with biblio_metadata_errors table Instead of storing a single nonxml_stripped flag directly on biblio_metadata, introduce a normalised biblio_metadata_errors table that can hold multiple typed error rows per metadata record. - Add biblio_metadata_errors table (id, metadata_id FK, error_type, message, created_on) with CASCADE delete from biblio_metadata - Remove the nonxml_stripped column from biblio_metadata - Add Koha::Biblio::Metadata::Error / Errors object classes - Refactor Koha::Biblio::Metadata::store() to write/clear a nonxml_stripped row in biblio_metadata_errors rather than the old column - Add metadata_errors() accessor on Koha::Biblio::Metadata - Update C4::Biblio, catalogue/detail.pl, catalogue/MARCdetail.pl and tests to query biblio_metadata_errors instead of the old column Sponsored-by: OpenFifth
Created attachment 194165 [details] [review] Bug 35104: Improve nonxml_stripped error context to cover all affected fields The previous implementation stored only the first XML parser error message (e.g. "PCDATA invalid Char value 31 at line X, column Y"), which had two problems: - The XML parser stops at the first bad character, so records with multiple affected fields only reported one - The message gave no indication of which MARC field/subfield to fix Add _nonxml_affected_fields(), which scans the raw MARC XML string for every datafield subfield and control field that contains at least one character illegal in XML 1.0 (the same set stripped by StripNonXmlChars), and returns a deduplicated list of field references (e.g. ["245$a", "520$b"]). The stored error message now reads "Non-XML characters stripped from: 245$a" so cataloguers can immediately see which fields need attention, while the logger warn includes the field context alongside the original XML error for technical debugging. Sponsored-by: OpenFifth
Created attachment 194166 [details] [review] Bug 35104: Record each stripped character as its own error with field and position context Previously a single error row was written with a deduplicated list of affected field references. This was an improvement over the raw XML parser error (which only reported the first bad character), but still lost the per-character detail the parser provided. Replace _nonxml_affected_fields() with _find_nonxml_chars(), which scans every datafield subfield and control field in the original MARC XML and returns one hashref per bad-character occurrence, carrying: - field: human-readable field reference (e.g. "245$a") - char_ord: decimal ordinal of the illegal character - position: 1-based character offset within the field value store() now writes one biblio_metadata_errors row per occurrence with a message in the form: 245$a: invalid char value 31 (U+001F) at position 10 This combines field/subfield identification (introduced in the previous commit) with the character value and in-field location that the XML parser previously reported only for the first bad character. The logger warn retains the original XML parser error string alongside the deduplicated field list, so operators still see the full technical detail. A fallback to storing the raw parser error is kept for the unlikely case where the regex scan finds no matches despite stripping being needed. catalogue/detail.pl and catalogue/MARCdetail.pl now join all per-character messages with newlines so the existing <pre> alert block shows all errors. Sponsored-by: OpenFifth
Created attachment 194167 [details] [review] Bug 35104: Add context snippet with caret pointer to nonxml_stripped error messages Each per-character error message now includes a two-line snippet showing up to 30 characters of surrounding context, with the offending character replaced by a [U+XXXX] marker and a caret (^) aligned beneath it: 245$a: invalid char value 31 (U+001F) at position 10 Hello[U+001F]World ^ Adds _context_snippet() helper and refactors _find_nonxml_chars() to use index-based iteration so the char array can be passed directly to the snippet helper without re-splitting.
Created attachment 194168 [details] [review] Bug 35104: Use Unicode Control Pictures in nonxml_stripped context snippets Instead of a [U+XXXX] marker, the context snippet now shows a visible glyph in place of the stripped character: C0 controls (U+0000-U+001F) -> Unicode Control Pictures (U+2400-U+241F) DEL (U+007F) -> U+2421 SYMBOL FOR DELETE (?) other non-XML characters -> U+FFFD REPLACEMENT CHARACTER (?) The first line of the error message is unchanged, continuing to identify the character by its decimal value and U+XXXX code: 336$a: invalid char value 8 (U+0008) at position 12 t $xtac arr?\ $btxt V $2rdac ontent ^
Created attachment 194169 [details] [review] Bug 35104: Import StripNonXmlChars in C4::Biblio The function was called at line 2353 but missing from the use C4::Charset import list, causing an "Undefined subroutine" error when saving a record via the editor.
Created attachment 194170 [details] [review] Bug 35104: Remove silent pre-emptive StripNonXmlChars call from TransformHtmlToMarc The call was only applied to the first subfield of a newly-created MARC field (the elsif branch), not to subsequent subfields added to an existing field, making the stripping incomplete and inconsistent. It was also never imported, causing an "Undefined subroutine" crash on any save via the MARC editor. Non-XML character stripping is now handled entirely by Koha::Biblio::Metadata::store(), which operates on the complete MARC XML, records each stripped character as a biblio_metadata_errors row, and logs a warning - giving cataloguers and administrators full visibility.
Created attachment 194171 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields Two related changes: 1. Metadata::store() no longer auto-clears nonxml_stripped errors on a clean re-save. Errors are review flags that require deliberate human resolution; they are only replaced when fresh stripping occurs on the same record. 2. addbiblio.pl loads any existing nonxml_stripped errors for a biblio being edited and passes them to the template. The template renders them in an alert banner above the editor with "Go to field" quick-links that navigate to the correct tab and scroll to the first occurrence of the affected subfield. A custom .nonxml-linkfield JS handler resolves the tab and element at click time using the tag/subfield data attributes, avoiding the need to know the random element-ID suffix server-side.
Created attachment 194172 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Adds a REST DELETE endpoint and UI button so cataloguers can explicitly dismiss nonxml_stripped data-quality warnings without needing to re-save the record. - DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} deletes the specific error row; requires edit_catalogue permission. - Koha::REST::V1::Biblios::MetadataErrors controller handles the request, verifying the error belongs to the given biblio before deleting. - The edit form's warning banner now shows a 'Mark resolved' button next to each error. Clicking it fires an AJAX DELETE; on success the row is removed from the DOM and the entire banner is hidden when no errors remain.
Created attachment 194213 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194214 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194215 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 194216 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 194217 [details] [review] Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. Sponsored-by: OpenFifth
Created attachment 194218 [details] [review] Bug 35104: Automated Schema Update Patch from commit 0f43be8
Created attachment 194219 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth
Created attachment 194220 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. Sponsored-by: OpenFifth
Created attachment 194221 [details] [review] Bug 35104: Strip non-XML characters gracefully with per-field error tracking Rewrite Metadata::store to strip non-XML characters automatically rather than throwing a Koha::Exceptions::Metadata::Invalid exception. When stripping is needed: * Every bad character is located individually via _find_nonxml_chars(), which scans the raw MARCXML and records the field reference (e.g. 336$a), character ordinal, 1-based position within the subfield value, and a two-line context snippet generated by _context_snippet(). * Context snippets show up to 30 characters either side of the bad character, replacing it with the appropriate Unicode Control Picture (U+2400-U+241F for C0 controls, U+2421 for DEL, U+FFFD otherwise) so the location is visible even in plain text. A second line carries a caret (^) aligned beneath the replacement glyph. * Each occurrence is stored as a separate row in biblio_metadata_errors with error_type 'nonxml_stripped'. On a clean re-save the existing error rows are deliberately left in place: they are review flags requiring explicit human resolution and must not be silently cleared. Only a save that triggers fresh stripping replaces the existing set. * A new stripped_on_last_store() method lets callers distinguish between "stripping just happened" and "pre-existing errors are present", so the UI can avoid spuriously re-displaying a save-time warning when the record is simply re-saved without changes. If the MARCXML cannot be recovered at all even after stripping, the existing Koha::Exceptions::Metadata::Invalid exception is still thrown. Sponsored-by: OpenFifth
Created attachment 194222 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth
Created attachment 194223 [details] [review] Bug 35104: Display non-XML stripping warnings on biblio detail pages When a bibliographic record has nonxml_stripped errors recorded in biblio_metadata_errors, display a warning banner on both the normal detail page (catalogue/detail.pl) and the MARC detail page (catalogue/MARCdetail.pl) so that staff are aware the stored record may differ from the original data. Sponsored-by: OpenFifth
Created attachment 194224 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. Sponsored-by: OpenFifth
Created attachment 194225 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Extend the data quality warnings banner in the MARC editor with a per-error 'Mark resolved' button. Clicking it calls a new REST endpoint (DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id}) which removes the corresponding biblio_metadata_errors row. The button is then hidden client-side to give immediate feedback. The new Koha::REST::V1::Biblios::MetadataErrors controller handles the delete, verifying that the requested error belongs to the requested biblio before removing it. Sponsored-by: OpenFifth
Created attachment 194226 [details] [review] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth
Created attachment 194227 [details] Rogue record to trigger errors
Please find attached to the patchset a rogue record that will trigger the xml character stripping as refined here. Test plan 1) Use the record attached, loading it into the system via 'stage + commit' or 'bulkmarkimport' or any other means you see fit 2) Note that the record loads, warnings may be displayed on the command line however. 3) Navigate to the new record, the page should load but show a message at the top with details that the record was modified on import and how it was modified. 4) Edit the record and mark the issue as resolved 5) The warning message should no longer appear
Some initial testing. Maybe we should ask some cataloguers for same sample records that should cause issues? First attempted test -------------------- I attempted to test, but getting this error in several catalog areas in the staff interface after importing the sample record: The method Koha::Biblio::Metadata->metadata_errors is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Object.pm line 1042 Areas include: - Accessing a record from search results - Trying to directly access a record - View and edit actions for manage stated MARC records This only happens after importing the sample record. I did a reset_all, then updatedatabase, dbic, yarn build, and a restart_all, and didn't get this error. Imported record --------------- Viewing the imported record in the staff interface: Record was automatically modified Non-XML characters were stripped from this record when it was last saved. Please review and re-save the record to confirm the data is correct. 336$a: invalid char value 8 (U+0008) at position 12 t $xtac arr␈\ $btxt V $2rdac ontent ^ Resolved error and updated other MARC issues, can view and edit fine. Testing notes ------------- Testing notes so far (using KTD): 1. Apply the patch 2. Run the database update: updatedatabase 3. Run DBIC: dbic (not sure if this is required) 4. Test importing the sample record using Cataloging > Stage record for import and Manage staged record 5. Run tests prove t/db_dependent/Koha/Biblio/Metadata.t t/db_dependent/Biblio.t t/db_dependent/Exporter/Record.t 6. Bonus tests: - Merge a record - Import record from the command line - Z39.50 import
Hi David, thanks for testing. I'm struggling to replicate the issue your seeing.. I started with a ktd that started on main, then I checked out the branch and ran through updatedatabase followed by a restart_all. I imported my record using stage then import. Searching worked for me, direct navigation worked etc. I then tried a reset_all and uploaded the rogue record again.. all continue to work for me. Happy to discus on Mattermost if it's easier?
I didn't have the same problem as David. David, did you restart_all? Everything worked fine for me except the qa tool: [SKIP] Koha/Biblio/Metadata/Error.pm SKIP pod_coverage No POD exists [FAIL] t/db_dependent/Koha/Biblio/Metadata.t FAIL forbidden_patterns forbidden pattern: Use https:// instead of http:// for URLs (line 58) [FAIL] t/db_dependent/api/v1/biblios_metadata_errors.t FAIL file_permissions File must have the exec flag
(In reply to Lisette Scheer from comment #126) > > I didn't have the same problem as David. David, did you restart_all? > > Everything worked fine for me except the qa tool: > [SKIP] Koha/Biblio/Metadata/Error.pm > SKIP pod_coverage > No POD exists False negative.. new files end up with the POD failure even when there's POD for some reason. > [FAIL] t/db_dependent/Koha/Biblio/Metadata.t > FAIL forbidden_patterns > forbidden pattern: Use https:// instead of http:// for URLs False negative.. it's http inside test data inside the standard xslt > (line 58) > [FAIL] t/db_dependent/api/v1/biblios_metadata_errors.t > FAIL file_permissions > File must have the exec flag Could have sworn I'd fixed this already.. follow-up attached anyways.
Created attachment 195433 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 195434 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 195435 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 195436 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 195437 [details] [review] Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. Sponsored-by: OpenFifth
Created attachment 195438 [details] [review] Bug 35104: Automated Schema Update Patch from commit ed58435
Created attachment 195439 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth
Created attachment 195440 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. Sponsored-by: OpenFifth
Created attachment 195441 [details] [review] Bug 35104: Strip non-XML characters gracefully with per-field error tracking Rewrite Metadata::store to strip non-XML characters automatically rather than throwing a Koha::Exceptions::Metadata::Invalid exception. When stripping is needed: * Every bad character is located individually via _find_nonxml_chars(), which scans the raw MARCXML and records the field reference (e.g. 336$a), character ordinal, 1-based position within the subfield value, and a two-line context snippet generated by _context_snippet(). * Context snippets show up to 30 characters either side of the bad character, replacing it with the appropriate Unicode Control Picture (U+2400-U+241F for C0 controls, U+2421 for DEL, U+FFFD otherwise) so the location is visible even in plain text. A second line carries a caret (^) aligned beneath the replacement glyph. * Each occurrence is stored as a separate row in biblio_metadata_errors with error_type 'nonxml_stripped'. On a clean re-save the existing error rows are deliberately left in place: they are review flags requiring explicit human resolution and must not be silently cleared. Only a save that triggers fresh stripping replaces the existing set. * A new stripped_on_last_store() method lets callers distinguish between "stripping just happened" and "pre-existing errors are present", so the UI can avoid spuriously re-displaying a save-time warning when the record is simply re-saved without changes. If the MARCXML cannot be recovered at all even after stripping, the existing Koha::Exceptions::Metadata::Invalid exception is still thrown. Sponsored-by: OpenFifth
Created attachment 195442 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth
Created attachment 195443 [details] [review] Bug 35104: Display non-XML stripping warnings on biblio detail pages When a bibliographic record has nonxml_stripped errors recorded in biblio_metadata_errors, display a warning banner on both the normal detail page (catalogue/detail.pl) and the MARC detail page (catalogue/MARCdetail.pl) so that staff are aware the stored record may differ from the original data. Sponsored-by: OpenFifth
Created attachment 195444 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. Sponsored-by: OpenFifth
Created attachment 195445 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Extend the data quality warnings banner in the MARC editor with a per-error 'Mark resolved' button. Clicking it calls a new REST endpoint (DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id}) which removes the corresponding biblio_metadata_errors row. The button is then hidden client-side to give immediate feedback. The new Koha::REST::V1::Biblios::MetadataErrors controller handles the delete, verifying that the requested error belongs to the requested biblio before removing it. Sponsored-by: OpenFifth
Created attachment 195446 [details] [review] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth
Created attachment 195447 [details] [review] Bug 35104: (follow-up) Tests should be executable Patch from commit 62f73a1
Created attachment 199928 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199929 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199930 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199931 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 199932 [details] [review] Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. Sponsored-by: OpenFifth
Created attachment 199933 [details] [review] Bug 35104: Automated Schema Update
Created attachment 199934 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth
Created attachment 199935 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. Sponsored-by: OpenFifth
Created attachment 199936 [details] [review] Bug 35104: Strip non-XML characters gracefully with per-field error tracking Rewrite Metadata::store to strip non-XML characters automatically rather than throwing a Koha::Exceptions::Metadata::Invalid exception. When stripping is needed: * Every bad character is located individually via _find_nonxml_chars(), which scans the raw MARCXML and records the field reference (e.g. 336$a), character ordinal, 1-based position within the subfield value, and a two-line context snippet generated by _context_snippet(). * Context snippets show up to 30 characters either side of the bad character, replacing it with the appropriate Unicode Control Picture (U+2400-U+241F for C0 controls, U+2421 for DEL, U+FFFD otherwise) so the location is visible even in plain text. A second line carries a caret (^) aligned beneath the replacement glyph. * Each occurrence is stored as a separate row in biblio_metadata_errors with error_type 'nonxml_stripped'. On a clean re-save the existing error rows are deliberately left in place: they are review flags requiring explicit human resolution and must not be silently cleared. Only a save that triggers fresh stripping replaces the existing set. * A new stripped_on_last_store() method lets callers distinguish between "stripping just happened" and "pre-existing errors are present", so the UI can avoid spuriously re-displaying a save-time warning when the record is simply re-saved without changes. If the MARCXML cannot be recovered at all even after stripping, the existing Koha::Exceptions::Metadata::Invalid exception is still thrown. Sponsored-by: OpenFifth
Created attachment 199937 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth
Created attachment 199938 [details] [review] Bug 35104: Display non-XML stripping warnings on biblio detail pages When a bibliographic record has nonxml_stripped errors recorded in biblio_metadata_errors, display a warning banner on both the normal detail page (catalogue/detail.pl) and the MARC detail page (catalogue/MARCdetail.pl) so that staff are aware the stored record may differ from the original data. Sponsored-by: OpenFifth
Created attachment 199939 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. Sponsored-by: OpenFifth
Created attachment 199940 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Extend the data quality warnings banner in the MARC editor with a per-error 'Mark resolved' button. Clicking it calls a new REST endpoint (DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id}) which removes the corresponding biblio_metadata_errors row. The button is then hidden client-side to give immediate feedback. The new Koha::REST::V1::Biblios::MetadataErrors controller handles the delete, verifying that the requested error belongs to the requested biblio before removing it. Sponsored-by: OpenFifth
Created attachment 199941 [details] [review] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth
Created attachment 199942 [details] [review] Bug 35104: (follow-up) Tests should be executable
Created attachment 199953 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199954 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199955 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199956 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 199957 [details] [review] Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. Sponsored-by: OpenFifth
Created attachment 199958 [details] [review] Bug 35104: Automated Schema Update
Created attachment 199959 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth
Created attachment 199960 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. Sponsored-by: OpenFifth
Created attachment 199961 [details] [review] Bug 35104: Strip non-XML characters gracefully with per-field error tracking Rewrite Metadata::store to strip non-XML characters automatically rather than throwing a Koha::Exceptions::Metadata::Invalid exception. When stripping is needed: * Every bad character is located individually via _find_nonxml_chars(), which scans the raw MARCXML and records the field reference (e.g. 336$a), character ordinal, 1-based position within the subfield value, and a two-line context snippet generated by _context_snippet(). * Context snippets show up to 30 characters either side of the bad character, replacing it with the appropriate Unicode Control Picture (U+2400-U+241F for C0 controls, U+2421 for DEL, U+FFFD otherwise) so the location is visible even in plain text. A second line carries a caret (^) aligned beneath the replacement glyph. * Each occurrence is stored as a separate row in biblio_metadata_errors with error_type 'nonxml_stripped'. On a clean re-save the existing error rows are deliberately left in place: they are review flags requiring explicit human resolution and must not be silently cleared. Only a save that triggers fresh stripping replaces the existing set. * A new stripped_on_last_store() method lets callers distinguish between "stripping just happened" and "pre-existing errors are present", so the UI can avoid spuriously re-displaying a save-time warning when the record is simply re-saved without changes. If the MARCXML cannot be recovered at all even after stripping, the existing Koha::Exceptions::Metadata::Invalid exception is still thrown. Sponsored-by: OpenFifth
Created attachment 199962 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth
Created attachment 199963 [details] [review] Bug 35104: Display non-XML stripping warnings on biblio detail pages When a bibliographic record has nonxml_stripped errors recorded in biblio_metadata_errors, display a warning banner on both the normal detail page (catalogue/detail.pl) and the MARC detail page (catalogue/MARCdetail.pl) so that staff are aware the stored record may differ from the original data. Sponsored-by: OpenFifth
Created attachment 199964 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. Sponsored-by: OpenFifth
Created attachment 199965 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Extend the data quality warnings banner in the MARC editor with a per-error 'Mark resolved' button. Clicking it calls a new REST endpoint (DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id}) which removes the corresponding biblio_metadata_errors row. The button is then hidden client-side to give immediate feedback. The new Koha::REST::V1::Biblios::MetadataErrors controller handles the delete, verifying that the requested error belongs to the requested biblio before removing it. Sponsored-by: OpenFifth
Created attachment 199966 [details] [review] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth
Created attachment 199967 [details] [review] Bug 35104: (follow-up) Tests should be executable
Created attachment 199968 [details] [review] Bug 35104: (follow-up) QA fixes - Use say_success() in atomicupdate instead of bare say $out - Log a warning in Koha::Import::Record::get_marc_record when both XML parse attempts fail, preserving the original error context - Add plan tests to the three inner subtests that were missing them - Eliminate double DB query (count + as_list) in catalogue/detail.pl and catalogue/MARCdetail.pl - Report all nonxml_stripped occurrences in the ModBiblioMarc warning message, not just the first
Created attachment 199969 [details] [review] Bug 35104: (follow-up) Fix QA issues - Run perltidy on C4/Biblio.pm and Koha/Import/Record.pm - Replace raw MARCXML heredoc in test with MARC::Record->as_xml_record to avoid forbidden http:// URL pattern - Update POD example in Koha::Biblio::Metadata to avoid codespell false positive on word fragment
Created attachment 199971 [details] [review] Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199972 [details] [review] Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199973 [details] [review] Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 199974 [details] [review] Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. Sponsored-by: OpenFifth
Created attachment 199975 [details] [review] Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. Sponsored-by: OpenFifth
Created attachment 199976 [details] [review] Bug 35104: Automated Schema Update
Created attachment 199977 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth
Created attachment 199978 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. Sponsored-by: OpenFifth
Created attachment 199979 [details] [review] Bug 35104: Strip non-XML characters gracefully with per-field error tracking Rewrite Metadata::store to strip non-XML characters automatically rather than throwing a Koha::Exceptions::Metadata::Invalid exception. When stripping is needed: * Every bad character is located individually via _find_nonxml_chars(), which scans the raw MARCXML and records the field reference (e.g. 336$a), character ordinal, 1-based position within the subfield value, and a two-line context snippet generated by _context_snippet(). * Context snippets show up to 30 characters either side of the bad character, replacing it with the appropriate Unicode Control Picture (U+2400-U+241F for C0 controls, U+2421 for DEL, U+FFFD otherwise) so the location is visible even in plain text. A second line carries a caret (^) aligned beneath the replacement glyph. * Each occurrence is stored as a separate row in biblio_metadata_errors with error_type 'nonxml_stripped'. On a clean re-save the existing error rows are deliberately left in place: they are review flags requiring explicit human resolution and must not be silently cleared. Only a save that triggers fresh stripping replaces the existing set. * A new stripped_on_last_store() method lets callers distinguish between "stripping just happened" and "pre-existing errors are present", so the UI can avoid spuriously re-displaying a save-time warning when the record is simply re-saved without changes. If the MARCXML cannot be recovered at all even after stripping, the existing Koha::Exceptions::Metadata::Invalid exception is still thrown. Sponsored-by: OpenFifth
Created attachment 199980 [details] [review] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth
Created attachment 199981 [details] [review] Bug 35104: Display non-XML stripping warnings on biblio detail pages When a bibliographic record has nonxml_stripped errors recorded in biblio_metadata_errors, display a warning banner on both the normal detail page (catalogue/detail.pl) and the MARC detail page (catalogue/MARCdetail.pl) so that staff are aware the stored record may differ from the original data. Sponsored-by: OpenFifth
Created attachment 199982 [details] [review] Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. Sponsored-by: OpenFifth
Created attachment 199983 [details] [review] Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Extend the data quality warnings banner in the MARC editor with a per-error 'Mark resolved' button. Clicking it calls a new REST endpoint (DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id}) which removes the corresponding biblio_metadata_errors row. The button is then hidden client-side to give immediate feedback. The new Koha::REST::V1::Biblios::MetadataErrors controller handles the delete, verifying that the requested error belongs to the requested biblio before removing it. Sponsored-by: OpenFifth
Created attachment 199984 [details] [review] Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. Sponsored-by: OpenFifth
Created attachment 199985 [details] [review] Bug 35104: (follow-up) Tests should be executable
Created attachment 199986 [details] [review] Bug 35104: (follow-up) QA fixes - Use say_success() in atomicupdate instead of bare say $out - Log a warning in Koha::Import::Record::get_marc_record when both XML parse attempts fail, preserving the original error context - Add plan tests to the three inner subtests that were missing them - Eliminate double DB query (count + as_list) in catalogue/detail.pl and catalogue/MARCdetail.pl - Report all nonxml_stripped occurrences in the ModBiblioMarc warning message, not just the first
Created attachment 199987 [details] [review] Bug 35104: (follow-up) Fix QA issues - Run perltidy on C4/Biblio.pm and Koha/Import/Record.pm - Replace raw MARCXML heredoc in test with MARC::Record->as_xml_record to avoid forbidden http:// URL pattern - Update POD example in Koha::Biblio::Metadata to avoid codespell false positive on word fragment
Created attachment 199988 [details] [review] Bug 35104: (follow-up) Simplify encode_error control flow in addbiblio.pl Handle the encode_error and nonxml_stripped cases with early exits immediately after the try/catch block. This removes the need for !$encode_error guards on all the success-path branches, as suggested by David Cook in comment #85.
Oops I didn't realise I was the QA contact on this. Looks like I'll have to take a look next week...
Could we get a test plan here, Martin? I tried exporting record 369 - but we can't do that because if bad xml characters, so then I copied the marcxml from the DB and saved it into a file Before the patches: I can stage and import the record - I cannot view or edit the record After the patches: I can stage the record, import fails with no visible error in staff side,logs show: Invalid data, cannot decode metadata object (biblio_metadata.id=, biblionumber=441, format=marcxml, schema=MARC21, decoding_error='Field 942 must have at least one subfield at /usr/share/perl5/MARC/File/XML.pm line 483.') at /kohadevbox/koha/Koha/BackgroundJob/MARCImportCommitBatch.pm line 93. Use of uninitialized value in sprintf at /kohadevbox/koha/Koha/Exceptions/Metadata.pm line 41. I also tried editing the existing record 369 - with or without the patches I require bug 42512 Before the patches: I get the warning about the record on details I can edit and fix After the patches: I get the warning about the record on details I get a warning about the record on edit I can edit and fix I think importing needs to provide the error to the user if it is going to fail on bad encoding
Test plan ========= Prerequisites: - Bug 42512 applied (needed to edit records whose metadata cannot be fully decoded). - A record with MARCXML containing invalid/non-recoverable content. Two useful variants: a) Non-XML characters: a control character embedded in a field value (this is the original, "characters invalid in XML" scenario the bug targets). b) A structurally invalid field: a <datafield> element with no <subfield> children at all, e.g. `<datafield tag="500" ind1=" " ind2=" "></datafield>`. MARC::File::XML refuses to parse this ("Field NNN must have at least one subfield"). Legacy/externally-sourced records can contain this even though Koha's own live code cannot produce it directly. You can inject either into an existing record's stored metadata via the DB to set up test data: UPDATE biblio_metadata SET metadata = REPLACE(metadata, '</record>', '<datafield tag="500" ind1=" " ind2=" "></datafield></record>') WHERE biblionumber = <some biblionumber>; 1) Single record view/edit - non-XML characters (existing behaviour) - Save a record containing an invalid-for-XML character in a field. - Confirm the character is stripped automatically, an error is recorded and shown on the detail/edit pages with a quick-link to the affected field, and a "Mark resolved" button is available to dismiss it once corrected. 2) Single record view/edit - empty datafield (new recovery) - Corrupt biblio_metadata for an existing record with an empty <datafield> as above. - View the record's detail page: a "Record was automatically modified" warning is shown, listing the field that was removed (e.g. "500: field removed, it had no subfields"). - Edit the record (cataloguing/addbiblio.pl): the same warning appears in a "Data quality warnings" banner. Unlike the non-XML-character case, there is no "Go to field" link (the whole field was removed, so there's nothing left in the record to jump to) - only the message and a "Mark resolved" button. - Confirm a record with BOTH an empty datafield and a non-XML character in different fields shows both warnings together, and both can be independently marked resolved. 3) Batch import of an unrecoverable record - Stage a MARCXML file containing one record with a non-recoverable metadata problem, together with one or more normal, valid records in the same batch (Tools > Stage MARC records for import). - Complete staging, then commit the batch (Tools > Manage staged MARC records > Import this batch). - Confirm: - The background job finishes (does not silently fail with no detail). - The job's report shows a "Number of records with errors" count of 1, alongside the normal added/updated/ignored counts. - The valid record(s) in the same batch are still imported successfully. - On "Manage staged MARC records", the bad record's row shows a red "Error" status; hovering over it shows the decoding error message. 4) Reverting a batch with an unrecoverable record - Import a batch that overlays (replaces) an existing record, then corrupt that record's matched biblio's metadata via the DB so a subsequent revert cannot decode the pre-overlay copy stored in marcxml_old (or use a batch where the original record already had this issue). - Revert the batch (Tools > Manage staged MARC records > Revert this batch). - Confirm the revert job finishes rather than failing outright, the affected record is left with an "error" status, and other records in the batch are still reverted normally. 5) CLI import (misc/commit_file.pl) - Stage the same batch as in (3), then run: misc/commit_file.pl --batch-number <id> - Confirm the printed summary includes a "Number of records with errors" line, and the command completes rather than dying. 6) Maintenance scripts - Run misc/maintenance/touch_all_biblios.pl and misc/maintenance/sanitize_records.pl against a range of biblios that includes one with unrecoverable metadata. - Confirm each script reports the error for that one biblionumber and continues processing the rest, rather than dying partway through. 7) Regression check - Repeat a normal batch import and a normal single-record edit (no encoding problems) and confirm behaviour, counts, and displayed messages are unchanged from before this patch. Automated tests: - t/db_dependent/ImportBatch.t - t/db_dependent/Koha/BackgroundJob/MARCImportCommitBatch.t - t/db_dependent/Koha/BackgroundJob/MARCImportRevertBatch.t - t/db_dependent/Koha/Biblio/Metadata.t - t/db_dependent/Biblio.t - t/db_dependent/api/v1/biblios_metadata_errors.t
Created attachment 201672 [details] [review] Bug 35104: (follow-up) Surface invalid metadata errors during batch import QA testing showed that importing a staged record whose MARCXML cannot be decoded (and cannot be recovered by non-XML character stripping) now throws Koha::Exceptions::Metadata::Invalid from Koha::Biblio::Metadata::store, but C4::ImportBatch::BatchCommitRecords had no handling for it. The exception propagated out of the whole batch commit job, silently marking the entire job as failed with no way for staff to see which record or why - only visible in the server logs. BatchCommitRecords now catches this specific exception per-record, rolls back that record's changes, flags it with status 'error' and records the decoding error in import_records.import_error, and continues committing the rest of the batch. The new count is reported back through Koha::BackgroundJob::MARCImportCommitBatch's job report and misc/commit_file.pl's summary, and the per-record error message is now shown in the "Manage staged MARC records" table. Also fixes a "Use of uninitialized value in sprintf" warning in Koha::Exceptions::Metadata::full_message, hit every time this exception is raised for metadata that has not yet been stored (no id yet).
Created attachment 201673 [details] [review] Bug 35104: (follow-up) Isolate metadata decode failures in other batch loops Proactively applies the same fix as the previous follow-up to other places that loop over multiple records and can hit an undecodable MARC record along the way: - C4::ImportBatch::_batchCommitItems: an item whose MARCXML cannot be decoded now uses the existing import_items error/status columns (as already done for duplicate barcodes) instead of dying and aborting the rest of the record's items. - C4::ImportBatch::BatchRevertRecords: a record whose marcxml_old cannot be decoded during a 'restore' revert is now flagged with status 'error' and counted in num_errors instead of aborting the whole revert job. Koha::BackgroundJob::MARCImportRevertBatch's progress count is updated to include num_errors. - misc/maintenance/touch_all_biblios.pl and misc/maintenance/sanitize_records.pl: both call $biblio->metadata->record without catching the exception it throws on undecodable metadata (sanitize_records.pl even had a dead "unless ($record)" check, since record() throws rather than returning false). One corrupt record would silently stop either script partway through a potentially large run. Both now report the error for that biblionumber and continue with the rest.
Created attachment 201674 [details] [review] Bug 35104: (follow-up) Recover records with an empty datafield, generalize warning UI MARC::File::XML refuses to parse a <datafield> with no <subfield> children at all ("Field NNN must have at least one subfield"), which happens with legacy data saved before Koha validated MARCXML on write. Previously this was only caught by the "truly unrecoverable" branch of Koha::Biblio::Metadata::store, throwing Koha::Exceptions::Metadata::Invalid. store() now detects this specific fault the same way it already detects non-XML characters, and recovers by removing the empty datafield (its data cannot be reconstructed, so a 'empty_datafield_stripped' error row is written to biblio_metadata_errors for a cataloguer to review, exactly like the existing 'nonxml_stripped' rows). Both faults are detected independently, so a record with both is repaired in one pass and both error types are recorded. The nonxml_stripped-specific plumbing that surfaces these rows to staff (detail/MARCdetail warning banners, the addbiblio.pl edit-form banner with quick-links and "Mark resolved", merge.pl, and bulkmarcimport.pl's log output) is generalized to show any recorded metadata error rather than hardcoding one error_type, so future recoverable-fault types need no further UI plumbing. The "go to field" quick-link is only offered for faults that leave the field in place to link to (nonxml_stripped); empty_datafield_stripped removes the whole field, so there's nothing left to link to. C4::Biblio::ModBiblioMarc's warnings-array forwarding is likewise generalized: Koha::Biblio::Metadata::store now puts an already-formatted message string in the object_message payload (instead of a raw occurrence array plus type-specific formatting logic living in C4::Biblio.pm), so ModBiblioMarc can forward any warning-type message without knowing its shape. The final message text sent to callers is unchanged, verified by the existing nonxml_stripped tests in t/db_dependent/Biblio.t passing unmodified.
37 files changed, 1731 insertions(+), 145 deletions(-) That's a bit intimidating to test in one go... I'll give up QA on this one, and see what other bugs I can QA more readily...
Created attachment 201710 [details] [review] Bug 35104: Add biblio_metadata_errors table for per-fault error tracking Adds biblio_metadata_errors to record individual faults (non-XML characters, empty datafields, ...) found and recovered when saving a bibliographic record's MARCXML, so they can be surfaced to cataloguers and explicitly resolved once reviewed.
Created attachment 201711 [details] [review] Bug 35104: Automated Schema Update
Created attachment 201712 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result
Created attachment 201713 [details] [review] Bug 35104: Guard Koha::Exceptions::Metadata::Invalid::full_message against undef fields full_message() interpolated id/biblionumber/format/schema/decoding_error directly into a sprintf without checking definedness, producing Perl warnings whenever one of them is unset (e.g. a new, not-yet-stored metadata object with no id yet).
Created attachment 201714 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Thin Koha::Object(s) wrappers around biblio_metadata_errors. error_type is restricted to a known set of constants, validated on store(), so a typo'd or unknown value fails loudly rather than writing an unrecognised value into the database. Errors::by_type() gives callers a named filter instead of hand-rolling the same search().
Created attachment 201715 [details] [review] Bug 35104: Recover MARCXML that fails to decode, or throw when unrecoverable Koha::Biblio::Metadata->store() now validates that MARCXML round-trips through MARC::Record before saving it, rather than saving whatever was given and letting corruption surface later as an opaque failure somewhere downstream. Two faults are recovered automatically, and either or both may apply to the same record: - non-XML characters (recorded as nonxml_stripped, recovered via StripNonXmlChars) - datafields with no subfields at all, which MARC::File::XML refuses to parse (recorded as empty_datafield_stripped, recovered by removing the empty datafield) Each recovered fault is logged, reported via object_messages for the caller to act on, and recorded as a biblio_metadata_errors row (tag and subfield included where identifiable) for later review - except where an identical row already exists for the same metadata_id, so a source record that stays dirty across repeated saves (e.g. re-harvested from an external repository) doesn't pile up duplicate rows for the same fault. Existing rows are otherwise left alone on a clean save, since they're review flags requiring explicit resolution, not a cache to invalidate. If the MARCXML cannot be recovered at all, Koha::Exceptions::Metadata::Invalid is thrown. The recovery logic is exposed as a repair_marcxml class method so other callers that parse MARCXML ahead of a store() (see following bugs) can reuse it instead of re-implementing it.
Created attachment 201716 [details] [review] Bug 35104: Catch invalid metadata and surface repair warnings in AddBiblio/ModBiblio AddBiblio/ModBiblio/ModBiblioMarc gain an optional warnings arrayref option: any repair messages recorded by Koha::Biblio::Metadata->store() (see previous commit) are appended to it via object_messages, so a caller can show the cataloguer what was automatically fixed instead of the fix happening invisibly. AddBiblio also now distinguishes Koha::Exceptions::Metadata::Invalid from other errors in its catch block and rethrows it, so a caller can tell "this record is unrecoverable" apart from an unexpected internal error and react accordingly (see following bugs for callers doing so). The inline StripNonXmlChars calls in TransformHtmlToMarc are removed, since stripping is now handled centrally and consistently by Koha::Biblio::Metadata->store() rather than ad-hoc at every call site.
Created attachment 201717 [details] [review] Bug 35104: Update Exporter/Record.t fixture for the new store() validation This test built a "bad" biblio by storing invalid MARCXML directly via Koha::Biblio::Metadata->store(), which now rejects it. Simulate legacy corrupt data already in the database by writing it straight to the DBIC row, bypassing the new validation, instead.
Created attachment 201719 [details] [review] Bug 35104: Add biblio_metadata_errors table for per-fault error tracking Adds biblio_metadata_errors to record individual faults (non-XML characters, empty datafields, ...) found and recovered when saving a bibliographic record's MARCXML, so they can be surfaced to cataloguers and explicitly resolved once reviewed. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201720 [details] [review] Bug 35104: Automated Schema Update Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201721 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201722 [details] [review] Bug 35104: Guard Koha::Exceptions::Metadata::Invalid::full_message against undef fields full_message() interpolated id/biblionumber/format/schema/decoding_error directly into a sprintf without checking definedness, producing Perl warnings whenever one of them is unset (e.g. a new, not-yet-stored metadata object with no id yet). Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201723 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Thin Koha::Object(s) wrappers around biblio_metadata_errors. error_type is restricted to a known set of constants, validated on store(), so a typo'd or unknown value fails loudly rather than writing an unrecognised value into the database. Errors::by_type() gives callers a named filter instead of hand-rolling the same search(). Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201724 [details] [review] Bug 35104: Recover MARCXML that fails to decode, or throw when unrecoverable Koha::Biblio::Metadata->store() now validates that MARCXML round-trips through MARC::Record before saving it, rather than saving whatever was given and letting corruption surface later as an opaque failure somewhere downstream. Two faults are recovered automatically, and either or both may apply to the same record: - non-XML characters (recorded as nonxml_stripped, recovered via StripNonXmlChars) - datafields with no subfields at all, which MARC::File::XML refuses to parse (recorded as empty_datafield_stripped, recovered by removing the empty datafield) Each recovered fault is logged, reported via object_messages for the caller to act on, and recorded as a biblio_metadata_errors row (tag and subfield included where identifiable) for later review - except where an identical row already exists for the same metadata_id, so a source record that stays dirty across repeated saves (e.g. re-harvested from an external repository) doesn't pile up duplicate rows for the same fault. Existing rows are otherwise left alone on a clean save, since they're review flags requiring explicit resolution, not a cache to invalidate. If the MARCXML cannot be recovered at all, Koha::Exceptions::Metadata::Invalid is thrown. The recovery logic is exposed as a repair_marcxml class method so other callers that parse MARCXML ahead of a store() (see following bugs) can reuse it instead of re-implementing it. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201725 [details] [review] Bug 35104: Catch invalid metadata and surface repair warnings in AddBiblio/ModBiblio AddBiblio/ModBiblio/ModBiblioMarc gain an optional warnings arrayref option: any repair messages recorded by Koha::Biblio::Metadata->store() (see previous commit) are appended to it via object_messages, so a caller can show the cataloguer what was automatically fixed instead of the fix happening invisibly. AddBiblio also now distinguishes Koha::Exceptions::Metadata::Invalid from other errors in its catch block and rethrows it, so a caller can tell "this record is unrecoverable" apart from an unexpected internal error and react accordingly (see following bugs for callers doing so). The inline StripNonXmlChars calls in TransformHtmlToMarc are removed, since stripping is now handled centrally and consistently by Koha::Biblio::Metadata->store() rather than ad-hoc at every call site. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
Created attachment 201726 [details] [review] Bug 35104: Update Exporter/Record.t fixture for the new store() validation This test built a "bad" biblio by storing invalid MARCXML directly via Koha::Biblio::Metadata->store(), which now rejects it. Simulate legacy corrupt data already in the database by writing it straight to the DBIC row, bypassing the new validation, instead. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth
I split this into multiple bugs to make testing and QA simpler.. the final state when all bugs are applied is the same as it was before I did the split so I kept the original signoff attributions.
*** Bug 42413 has been marked as a duplicate of this bug. ***
Created attachment 203259 [details] [review] Bug 35104: Add biblio_metadata_errors table for per-fault error tracking Adds biblio_metadata_errors to record individual faults (non-XML characters, empty datafields, ...) found and recovered when saving a bibliographic record's MARCXML, so they can be surfaced to cataloguers and explicitly resolved once reviewed. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203260 [details] [review] Bug 35104: Automated Schema Update Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203261 [details] [review] Bug 35104: Add koha_object_class to BiblioMetadataError schema result Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203262 [details] [review] Bug 35104: Guard Koha::Exceptions::Metadata::Invalid::full_message against undef fields full_message() interpolated id/biblionumber/format/schema/decoding_error directly into a sprintf without checking definedness, producing Perl warnings whenever one of them is unset (e.g. a new, not-yet-stored metadata object with no id yet). Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203263 [details] [review] Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Thin Koha::Object(s) wrappers around biblio_metadata_errors. error_type is restricted to a known set of constants, validated on store(), so a typo'd or unknown value fails loudly rather than writing an unrecognised value into the database. Errors::by_type() gives callers a named filter instead of hand-rolling the same search(). Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203264 [details] [review] Bug 35104: Recover MARCXML that fails to decode, or throw when unrecoverable Koha::Biblio::Metadata->store() now validates that MARCXML round-trips through MARC::Record before saving it, rather than saving whatever was given and letting corruption surface later as an opaque failure somewhere downstream. Two faults are recovered automatically, and either or both may apply to the same record: - non-XML characters (recorded as nonxml_stripped, recovered via StripNonXmlChars) - datafields with no subfields at all, which MARC::File::XML refuses to parse (recorded as empty_datafield_stripped, recovered by removing the empty datafield) Each recovered fault is logged, reported via object_messages for the caller to act on, and recorded as a biblio_metadata_errors row (tag and subfield included where identifiable) for later review - except where an identical row already exists for the same metadata_id, so a source record that stays dirty across repeated saves (e.g. re-harvested from an external repository) doesn't pile up duplicate rows for the same fault. Existing rows are otherwise left alone on a clean save, since they're review flags requiring explicit resolution, not a cache to invalidate. If the MARCXML cannot be recovered at all, Koha::Exceptions::Metadata::Invalid is thrown. The recovery logic is exposed as a repair_marcxml class method so other callers that parse MARCXML ahead of a store() (see following bugs) can reuse it instead of re-implementing it. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203265 [details] [review] Bug 35104: Catch invalid metadata and surface repair warnings in AddBiblio/ModBiblio AddBiblio/ModBiblio/ModBiblioMarc gain an optional warnings arrayref option: any repair messages recorded by Koha::Biblio::Metadata->store() (see previous commit) are appended to it via object_messages, so a caller can show the cataloguer what was automatically fixed instead of the fix happening invisibly. AddBiblio also now distinguishes Koha::Exceptions::Metadata::Invalid from other errors in its catch block and rethrows it, so a caller can tell "this record is unrecoverable" apart from an unexpected internal error and react accordingly (see following bugs for callers doing so). The inline StripNonXmlChars calls in TransformHtmlToMarc are removed, since stripping is now handled centrally and consistently by Koha::Biblio::Metadata->store() rather than ad-hoc at every call site. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 203266 [details] [review] Bug 35104: Update Exporter/Record.t fixture for the new store() validation This test built a "bad" biblio by storing invalid MARCXML directly via Koha::Biblio::Metadata->store(), which now rejects it. Simulate legacy corrupt data already in the database by writing it straight to the DBIC row, bypassing the new validation, instead. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Initially sub _find_nonxml_chars sub _find_empty_datafields sub _context_snippet Seemed like additional work as MARC::Record::new_from_xml returns an error, however, it will only return the first error, and not all
*** Bug 42528 has been marked as a duplicate of this bug. ***
Thanks everyone! Pushed to main for 26.11!
(In reply to Pedro Amorim (ammopt) from comment #228) > Thanks everyone! Pushed to main for 26.11! Oh great! Thanks to everyone for working on this one. My apologies for not being able to contribute as much as I wanted to here.
This is causing the following test to fail: prove t/db_dependent/Biblio/TransformHtmlToMarc.t
This is causing the following tests to fail: prove t/db_dependent/Biblio/TransformHtmlToMarc.t prove t/db_dependent/selenium/regressions.t
Created attachment 203782 [details] [review] Bug 35104: (RM follow-up) Fix regressions.t for Metadata's new auto-repair Koha::Biblio::Metadata->store() now repairs recoverable MARCXML faults before saving, so this test's invalid data no longer reaches the DB and detail.pl never hits a decoding error. Write the metadata directly via the DBIC row instead, bypassing store()'s repair, so the test still exercises detail.pl's own defensive handling of corrupt metadata. Test plan: prove t/db_dependent/selenium/regressions.t Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 203802 [details] [review] Bug 35104: (RM follow-up) Restore StripNonXmlChars in TransformHtmlToMarc Removed by 6726b1a2544 on the assumption store() strips centrally - but TransformHtmlToMarc never calls store(), so callers like addbiblio.pl (FindDuplicate, duplicate redisplay) see unstripped control characters again, the same failure Bug 34549 fixed. repair_marcxml covers more faults but works on full MARCXML and can throw - too big a change here; left as a follow-up. Test plan: prove t/db_dependent/Biblio/TransformHtmlToMarc.t Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>