Bug 28604

Summary: Bad encoding when using marc-in-json
Product: Koha Reporter: Lucas Gass <lucas>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: fridolin.somers, kyle, martin.renvoize, tomascohen, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02,20.11.09,20.05.15
Attachments: Bug 28604: Regression tests
Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output
Bug 28604: Regression tests
Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output
Bug 28604: Regression tests
Bug 28604: Prevent double encoding of MARC::Record::MiJ->to_mij output

Description Lucas Gass 2021-06-18 20:33:46 UTC
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
Comment 1 Tomás Cohen Arazi 2021-06-18 21:47:31 UTC
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>
Comment 2 Tomás Cohen Arazi 2021-06-18 21:47:34 UTC
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>
Comment 3 Lucas Gass 2021-06-18 22:09:39 UTC
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>
Comment 4 Lucas Gass 2021-06-18 22:09:43 UTC
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>
Comment 5 Martin Renvoize 2021-06-22 15:34:22 UTC
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>
Comment 6 Martin Renvoize 2021-06-22 15:34:25 UTC
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>
Comment 7 Martin Renvoize 2021-06-22 15:34:49 UTC
All works well, QA script is happy, Tests pass.

Passing QA
Comment 8 Jonathan Druart 2021-06-23 13:07:06 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 9 Kyle M Hall 2021-07-09 14:31:03 UTC
Pushed to 21.05.x for 21.05.02
Comment 10 Fridolin Somers 2021-08-02 20:32:31 UTC
Pushed to 20.11.x for 20.11.09
Comment 11 Victor Grousset/tuxayo 2021-08-15 19:08:20 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
Comment 12 Tomás Cohen Arazi 2021-08-15 19:12:09 UTC
(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 :-)
Comment 13 Victor Grousset/tuxayo 2021-08-16 18:40:24 UTC
Backported: Pushed to 20.05.x branch for 20.05.15
Comment 14 Victor Grousset/tuxayo 2021-08-16 18:42:00 UTC
(sorry the spam, I couldn't find a quiet way to go from "closed" to "pushed to oldoldstable")
Comment 15 wainuiwitikapark 2021-08-17 00:32:21 UTC
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.
Comment 16 wainuiwitikapark 2021-09-07 01:46:58 UTC
Should this be backported to 19.11.x for 19.11.22?
Comment 17 Tomás Cohen Arazi 2021-09-07 01:48:20 UTC
(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
Comment 18 wainuiwitikapark 2021-09-07 04:12:29 UTC
Backported to 19.11.x for 19.11.22
Comment 19 wainuiwitikapark 2021-09-19 23:47:10 UTC
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
Comment 20 Victor Grousset/tuxayo 2021-09-21 12:17:03 UTC
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.
Comment 21 Tomás Cohen Arazi 2021-09-21 12:21:27 UTC
(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.
Comment 22 wainuiwitikapark 2021-09-22 02:26:05 UTC
Ok so reverted these patches - so not backported to 19.11.x