Bug 32336

Summary: MARCXML output of REST API may be badly encoded (UNIMARC)
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: REST APIAssignee: Julian Maurice <julian.maurice>
Status: Pushed to stable --- QA Contact:
Severity: major    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34514
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34467
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.05
Bug Depends on:    
Bug Blocks: 33329, 34515    
Attachments: Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)
Bug 32336: Add unit test for UNIMARC encoding problems in REST API
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)
Bug 32336: Add unit test for UNIMARC encoding problems in REST API
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)
Bug 32336: (QA follow-up) Use $metadata->schema

Description Julian Maurice 2022-11-23 14:41:23 UTC
In Koha/REST/V1/Biblios.pm (methods get and get_public), MARC::Record::as_xml_record is called without a $format parameter, which means it relies on the package variable $MARC::File::XML::RecordFormat to be correctly set, which may or may not be the case depending on what the current starman/mojo worker has done before.
We should pass the format explicitly to avoid problems.

This is only a problem for UNIMARC, because $MARC::File::XML::RecordFormat defaults to 'USMARC'.
Comment 1 Julian Maurice 2022-11-23 15:14:26 UTC
Created attachment 144207 [details] [review]
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)

Test plan:
1. Be sure to test on a UNIMARC instance
2. Enable system preferences RESTPublicAPI and RESTPublicAnonymousRequests
3. Create a biblio with some diacritics
4. Restart koha and do not interact with OPAC or the staff interface
   (this may change $MARC::File::XML::RecordFormat)
5. Query the public API to retrieve the created biblio in MARCXML. For
   instance with cURL:

   curl -H 'Accept: application/marcxml+xml' \
    http://koha.local/api/v1/public/biblios/<biblionumber>

   You should see encoding issues.
6. Apply the patch and restart Koha.
7. Repeat step 5.
   You should see no encoding issues.
8. You should also test /api/v1/biblios/<biblionumber> (this one
   requires authentication, so cURL might not be the most practical
   tool; use your favorite tool for this)
Comment 2 Tomás Cohen Arazi 2022-11-23 15:36:14 UTC
Please add tests
Comment 3 Julian Maurice 2023-02-17 13:52:13 UTC
Created attachment 146846 [details] [review]
Bug 32336: Add unit test for UNIMARC encoding problems in REST API
Comment 4 Julian Maurice 2023-02-17 13:52:15 UTC
Created attachment 146847 [details] [review]
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)

Test plan:
1. Be sure to test on a UNIMARC instance
2. Enable system preferences RESTPublicAPI and RESTPublicAnonymousRequests
3. Create a biblio with some diacritics
4. Restart koha and do not interact with OPAC or the staff interface
   (this may change $MARC::File::XML::RecordFormat)
5. Query the public API to retrieve the created biblio in MARCXML. For
   instance with cURL:

   curl -H 'Accept: application/marcxml+xml' \
    http://koha.local/api/v1/public/biblios/<biblionumber>

   You should see encoding issues.
6. Apply the patch and restart Koha.
7. Repeat step 5.
   You should see no encoding issues.
8. You should also test /api/v1/biblios/<biblionumber> (this one
   requires authentication, so cURL might not be the most practical
   tool; use your favorite tool for this)
Comment 5 Jonathan Druart 2023-03-23 14:28:02 UTC
(In reply to Julian Maurice from comment #4)
> 8. You should also test /api/v1/biblios/<biblionumber> (this one
>    requires authentication, so cURL might not be the most practical
>    tool; use your favorite tool for this)

FYI you can use -u koha:koha ;)

curl -v -s -u koha:koha --request GET 'http://localhost:8081/api/v1/biblios/1' --header 'Accept: application/marcxml+xml'
Comment 6 Jonathan Druart 2023-03-23 14:28:22 UTC
Created attachment 148614 [details] [review]
Bug 32336: Add unit test for UNIMARC encoding problems in REST API

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2023-03-23 14:28:24 UTC
Created attachment 148615 [details] [review]
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC)

Test plan:
1. Be sure to test on a UNIMARC instance
2. Enable system preferences RESTPublicAPI and RESTPublicAnonymousRequests
3. Create a biblio with some diacritics
4. Restart koha and do not interact with OPAC or the staff interface
   (this may change $MARC::File::XML::RecordFormat)
5. Query the public API to retrieve the created biblio in MARCXML. For
   instance with cURL:

   curl -H 'Accept: application/marcxml+xml' \
    http://koha.local/api/v1/public/biblios/<biblionumber>

   You should see encoding issues.
6. Apply the patch and restart Koha.
7. Repeat step 5.
   You should see no encoding issues.
8. You should also test /api/v1/biblios/<biblionumber> (this one
   requires authentication, so cURL might not be the most practical
   tool; use your favorite tool for this)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Tomás Cohen Arazi 2023-03-23 22:25:25 UTC
I think we need some more here: we should really be using

my $metadata = $biblio->metadata;
my $schema   = $biblio->schema // C4::Context->preference("marcflavour");

Should we also file a new bug for list()?
Comment 9 Tomás Cohen Arazi 2023-03-23 23:18:48 UTC
Created attachment 148628 [details] [review]
Bug 32336: (QA follow-up) Use $metadata->schema

When we worked on Koha::Biblio::Metadata, we agreed
biblio_metadata.schema would be the sourceof truth when it comes to the
record schema.

This patch acknowledges that, while retaining the fallback to the
syspref.

To test:
1. Apply this patch
2. Run:
   $ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass, behavior is unchanged.
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2023-03-23 23:19:31 UTC
If you agree with my follow-up please set it back to PQA, otherwise let's discuss it properly.

Best regards.
Comment 11 Julian Maurice 2023-03-30 13:34:28 UTC
Patch makes sense. Back to Passed QA
Thanks for the follow-up Tomas
Comment 12 Tomás Cohen Arazi 2023-03-31 10:04:15 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 13 Matt Blenkinsop 2023-04-11 11:20:00 UTC
Nice work everyone!

Pushed to stable for 22.11.x