[Bug 28230] New: Renewing/Checking out record with Ä or Ö letter in title can make Koha totally unfunctional
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Bug ID: 28230 Summary: Renewing/Checking out record with Ä or Ö letter in title can make Koha totally unfunctional Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Depends on: 22824 Koha can lock-up/stop working totally (unrelease manual DB LOCK on messages_queue table seems to cause this) in the following example situation (others aswell): - Have a biblio with "Ä" in title - Enable RenewalSendNotice - Enable "Item checkout and renewal" message preference for patron - Checkout an item from biblio "Ä" to patron - Renew the patrons loan and notice the following error:
{UNKNOWN}: YAML::XS::Load Error: The problem:
control characters are not allowed
was found at document: 0 at /kohadevbox/koha/C4/Circulation.pm line 3150
This seems to also cause a lock staying on message_queue table and locking other Koha database transactions from happening. I did some test code to debug this: #use YAML; use Encode; use Data::Dumper; use YAML::XS print Dumper(Dump(YAML::XS::Load(Encode::encode_utf8("---\r\n hello: 'heÄllo'")))); Basically here the YAML::XS version dies but if you replace that with YAML::Load it works. It might also have something to do with the Ä getting double encoded? The code that causes this problem is the
return YAML::XS::Load(Encode::encode_utf8($self->{metadata}));
line in C4::Message::metadata() function. This seems to be caused by "Bug 22824: Replace YAML::Syck with YAML::XS" (d6d01169509999e22). Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22824 [Bug 22824] Replace YAML::Syck with YAML::XS -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |blocker --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I haven't verified yet, but that would be a killer for any German installation... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com Keywords| |rel_21_05_candidate -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This works: use utf8; use Encode qw(encode encode_utf8); use Data::Dumper; use YAML::XS; binmode STDOUT, ':encoding(UTF-8)'; my $string = YAML::XS::Load(encode('UTF-8',"---\r\n hello: 'heÄllo'")); print STDOUT Dumper($string); $string = YAML::XS::Load(encode_utf8("---\r\n hello: 'heÄllo'")); print STDOUT Dumper($string); 1; -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #2)
This works:
I wanted to add that I totally reproduced this following the test plan, with the first record that comes with KTD (Russian). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Tomás Cohen Arazi from comment #2)
This works:
use utf8;
also adding use "utf8;" to the debug statement makes things work but I actually get different results with them: YAML::Load:
--- hello: heà llo
YAML::XS
--- hello: heÄllo
Anyway, if I'm not mistaken the use utf8 is just for the source code encoding, here we load the string from database. But good note this down. Maybe our input / biblio title is not properly utf8 encoded or something. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Joonas Kylmälä from comment #4)
also adding use "utf8;" to the debug statement makes things work but I actually get different results with them:
Meant to say the debug statement I posted in the first comment here. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Renewing/Checking out |Renewing/Checking out |record with Ä or Ö letter |record with AE or OE letter |in title can make Koha |in title can make Koha |totally unfunctional |totally unfunctional -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #6 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 120196 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120196&action=edit Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I was going to attach this patch: diff --git a/C4/Message.pm b/C4/Message.pm index 659ff96aeb2..d9bb945abb6 100644 --- a/C4/Message.pm +++ b/C4/Message.pm @@ -283,7 +283,7 @@ sub metadata { $self->content($self->render_metadata); return $data; } else { - return YAML::XS::Load(Encode::encode_utf8($self->{metadata})); + return YAML::XS::Load($self->{metadata}); } } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #8 from Andrew Nugged <nugged@gmail.com> --- So Joonas proposes to store octets in metadata by added missed "decoding to octets", and Jonathan proposes to store strings so to remove excessive in this case "encoding", I would agree to have more "just normal Perl strings around" and walk away as much as possible from "encode/decode", ... if this works (this might be uh :) ). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Jonathan Druart from comment #7)
I was going to attach this patch:
diff --git a/C4/Message.pm b/C4/Message.pm index 659ff96aeb2..d9bb945abb6 100644 --- a/C4/Message.pm +++ b/C4/Message.pm @@ -283,7 +283,7 @@ sub metadata { $self->content($self->render_metadata); return $data; } else { - return YAML::XS::Load(Encode::encode_utf8($self->{metadata})); + return YAML::XS::Load($self->{metadata}); } }
Why I didn't do this is because to me it looks to be incorrect in this scenario: 1. Load the object with find_last_message() -> metadata is now perl string 2. Call $message->append() -> it then calls my $metadata = $self->metadata; and we would be calling
+ return YAML::XS::Load($self->{metadata});
which means we are calling Load() with a Perl string and not UTF-8 octets which is wrong. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #10 from Andrew Nugged <nugged@gmail.com> --- right. Quote: USING YAML::XS WITH UNICODE Handling unicode properly in Perl can be a pain. YAML::XS only deals with streams of utf8 octets so seems this is that case when I want to have less encode/decode, but is impossible to avoid. Jonathan, then: Joonas did right -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- The
sub enqueue {
looks buggy as well.
$letter->{metadata} = Dump($metadata);
We should decode it to be a Perl string as well because in C4::Letters::EnqueueLetter which is called next and this is passed there it calls $sth->execute() with it. At least the other execute params I see are perl strings and not utf-8 octets so if it is not buggy it is unintuitive at least. but this enqueue bug is not related to this bug just wanted to mention because it is similar and might cause badly encoded emails. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120196|0 |1 is obsolete| | --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 120202 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120202&action=edit Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #13 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- I re-made the patch now so that the enqueue() function is free of this bug as well. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 120203 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120203&action=edit Bug 28230: Add tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120203|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 120204 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120204&action=edit Bug 28230: Add tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- And those tests prove you are right Joonas :) There is a warning displayed when the tests are run but I am suspecting an issue in t::lib::TestBuilder::builder_sample_biblio. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #16)
And those tests prove you are right Joonas :)
There is a warning displayed when the tests are run but I am suspecting an issue in t::lib::TestBuilder::builder_sample_biblio.
build_sample_biblio -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |joonas.kylmala@helsinki.fi |ity.org | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28234 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- See bug 28234. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120202|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120204|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120210 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120210&action=edit Bug 28230: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120211 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120211&action=edit Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Joonas Kylmälä from comment #13)
I re-made the patch now so that the enqueue() function is free of this bug as well.
+1 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120210|0 |1 is obsolete| | --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120227 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120227&action=edit Bug 28230: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120211|0 |1 is obsolete| | --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120228 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120228&action=edit Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Good catch guys, that's a nasty bug. Man I hate encoding issues.. All works as expected for me now.. passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120230 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120230&action=edit Bug 28230: Add execute bit to test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #27 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 22824 not in 20.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28230 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org