There are some UTF-8 encoding problems when using marc-in-json that have diacritics in them, I encountered this when using the '/api/v1/public/biblios/' API route. To recreate: -Add some diacritics to a MARC field, I am using the 538$a and added the note 'Tést nöte'. -Look at the what the API returns, I used this: curl --location --request GET 'http://localhost:8080/api/v1/public/biblios/144' \ --header 'Accept: application/marc-in-json' The 538$a note comes out looking like this: Tést nöte
Created attachment 122188 [details] [review] Bug 28604: Regression tests This patch introduces regression tests for the encoding issue with MiJ output. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122189 [details] [review] Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output This patch fixes a double-encoding issue with MiJ output. Mojolicious' *text* renderer encodes the passed information according to the request context. [1] MARC::Record::MiJ->to_mij, conveniently encodes the string before output [2]. This causes double encoding. So the solution to this situation, is to use the *data* renderer, which doesn't perform any encoding [3]. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests contain diacritics and fail! 3. Have a record with diacritics 4. Try the API routes for fetching a biblio: $ curl --location --request GET 'http://localhost:8080/api/v1/public/biblios/144' \ --header 'Accept: application/marc-in-json' (replace the record id with the one you've chosen) => FAIL: Boo, double encoding 5. Bonus point: you can try it on the non-public route, but you need more configuration boilerplate (basic auth, permissions). If you look at the fix, you will understand the tests cover it and no need to complicate yourself. 6. Apply this patch 7. Repeat 2 => SUCCESS: Tests pass! 8. Repeat 4 (and maybe 5) => SUCCESS: No double encoding! Yay! 9. Sign off :-D [1] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-text [2] https://metacpan.org/dist/MARC-File-MiJ/source/lib/MARC/Record/MiJ.pm#L111 [3] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-data Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122190 [details] [review] Bug 28604: Regression tests This patch introduces regression tests for the encoding issue with MiJ output. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 122191 [details] [review] Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output This patch fixes a double-encoding issue with MiJ output. Mojolicious' *text* renderer encodes the passed information according to the request context. [1] MARC::Record::MiJ->to_mij, conveniently encodes the string before output [2]. This causes double encoding. So the solution to this situation, is to use the *data* renderer, which doesn't perform any encoding [3]. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests contain diacritics and fail! 3. Have a record with diacritics 4. Try the API routes for fetching a biblio: $ curl --location --request GET 'http://localhost:8080/api/v1/public/biblios/144' \ --header 'Accept: application/marc-in-json' (replace the record id with the one you've chosen) => FAIL: Boo, double encoding 5. Bonus point: you can try it on the non-public route, but you need more configuration boilerplate (basic auth, permissions). If you look at the fix, you will understand the tests cover it and no need to complicate yourself. 6. Apply this patch 7. Repeat 2 => SUCCESS: Tests pass! 8. Repeat 4 (and maybe 5) => SUCCESS: No double encoding! Yay! 9. Sign off :-D [1] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-text [2] https://metacpan.org/dist/MARC-File-MiJ/source/lib/MARC/Record/MiJ.pm#L111 [3] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-data Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 122292 [details] [review] Bug 28604: Regression tests This patch introduces regression tests for the encoding issue with MiJ output. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122293 [details] [review] Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output This patch fixes a double-encoding issue with MiJ output. Mojolicious' *text* renderer encodes the passed information according to the request context. [1] MARC::Record::MiJ->to_mij, conveniently encodes the string before output [2]. This causes double encoding. So the solution to this situation, is to use the *data* renderer, which doesn't perform any encoding [3]. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests contain diacritics and fail! 3. Have a record with diacritics 4. Try the API routes for fetching a biblio: $ curl --location --request GET 'http://localhost:8080/api/v1/public/biblios/144' \ --header 'Accept: application/marc-in-json' (replace the record id with the one you've chosen) => FAIL: Boo, double encoding 5. Bonus point: you can try it on the non-public route, but you need more configuration boilerplate (basic auth, permissions). If you look at the fix, you will understand the tests cover it and no need to complicate yourself. 6. Apply this patch 7. Repeat 2 => SUCCESS: Tests pass! 8. Repeat 4 (and maybe 5) => SUCCESS: No double encoding! Yay! 9. Sign off :-D [1] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-text [2] https://metacpan.org/dist/MARC-File-MiJ/source/lib/MARC/Record/MiJ.pm#L111 [3] https://metacpan.org/release/MRAMBERG/Convos-0.5/view/local/lib/perl5/Mojolicious/Guides/Rendering.pod#Rendering-data Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works well, QA script is happy, Tests pass. Passing QA
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.02
Pushed to 20.11.x for 20.11.09
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
(In reply to Victor Grousset/tuxayo from comment #11) > Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. Can you please backport it to the 20.05.x branch? Thanks :-)
Backported: Pushed to 20.05.x branch for 20.05.15
(sorry the spam, I couldn't find a quiet way to go from "closed" to "pushed to oldoldstable")
Should this be backported to 19.11.x for 19.11.21? Or if it contains string changes, backported to 19.11.22? As we are currently in a string freeze.
Should this be backported to 19.11.x for 19.11.22?
(In reply to wainuiwitikapark from comment #16) > Should this be backported to 19.11.x for 19.11.22? If it applies, backport it. It has no string changes
Backported to 19.11.x for 19.11.22
On 19.11.x Test Result (2 failures / ±0) t_db_dependent_api_v1_biblios_t.get() tests (703) t_db_dependent_api_v1_biblios_t.get_public() tests (703) I am getting these failtures Can someone please write a patch for that
I think it's okay to just revert it. The severity is only "normal" and it was requested for 20.05.x I read comment 17 as an advice about the string freeze and not as a need for 19.11 Could be wrong though. If it turns out to be needed and is fixed for 19.11 , then it could still be backported next month.
(In reply to wainuiwitikapark from comment #19) > On 19.11.x > > Test Result (2 failures / ±0) > > t_db_dependent_api_v1_biblios_t.get() tests (703) > t_db_dependent_api_v1_biblios_t.get_public() tests (703) > > I am getting these failtures > > Can someone please write a patch for that I tried to understand what's going on yesterday, and couldn't figure why it is failing, I agree with Victor, that this can safely be reverted to keep jenkins happy, and if someone requests it, we can come back to it.
Ok so reverted these patches - so not backported to 19.11.x