Bug 32336 - MARCXML output of REST API may be badly encoded (UNIMARC)
Summary: MARCXML output of REST API may be badly encoded (UNIMARC)
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Julian Maurice
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 33329 34515
  Show dependency treegraph
 
Reported: 2022-11-23 14:41 UTC by Julian Maurice
Modified: 2023-10-10 06:31 UTC (History)
3 users (show)

See Also:
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


Attachments
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC) (2.09 KB, patch)
2022-11-23 15:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32336: Add unit test for UNIMARC encoding problems in REST API (2.04 KB, patch)
2023-02-17 13:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC) (2.09 KB, patch)
2023-02-17 13:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32336: Add unit test for UNIMARC encoding problems in REST API (2.12 KB, patch)
2023-03-23 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32336: Fix encoding of MARCXML output for REST API (UNIMARC) (2.16 KB, patch)
2023-03-23 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32336: (QA follow-up) Use $metadata->schema (2.55 KB, patch)
2023-03-23 23:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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