[Bug 43606] New: Add x-koha-embed: items support to GET /biblios (list), respecting OPAC/public item visibility
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Bug ID: 43606 Summary: Add x-koha-embed: items support to GET /biblios (list), respecting OPAC/public item visibility Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: tomascohen@gmail.com CC: tomascohen@gmail.com Target Milestone: --- The REST API endpoint GET /biblios (operationId listBiblio) can return records as MARCXML, MARC-in-JSON, USMARC and plain text via content negotiation. However, the serialized records never include item data (952 fields), even when the caller would like them embedded. Currently: * The swagger definition for GET /biblios (api/v1/swagger/paths/biblios.yaml) does not declare an x-koha-embed header parameter. Because OpenAPI input validation is strict, sending "x-koha-embed: items" against this endpoint results in an HTTP 400 response. * The controller Koha::REST::V1::Biblios::list serializes with $biblios->print_collection('marcxml'), passing no embed option. * Koha::Objects::Record::Collections::print_collection has a positional signature ( $self, $format ) and serializes each element using $element->record, i.e. the raw stored MARC with no items. As a result, clients that need biblios with items must fall back to fetching items one biblio at a time (GET /biblios/{biblio_id}/items), which is an N+1 access pattern and is significantly slower for bulk/list use cases. Proposed enhancement: Add the ability to embed items into the records returned by the list endpoints, driven by the existing x-koha-embed request header, e.g.: x-koha-embed: items Item visibility MUST be respected according to the interface the request is served on: * Staff/intranet route (GET /biblios): embed all items (subject to the existing catalogue permission already required by the endpoint). * Public route (GET /public/biblios and related public list endpoints): embed only items visible in the OPAC, honouring OpacHiddenItems and patron category override_hidden_items, consistent with how get_public, get_items_public and Items::list_public already scope items via filter_by_visible_in_opac. The building block already exists: Koha::Biblio::metadata_record already supports: $biblio->metadata_record({ embed_items => 1, interface => 'opac' | 'intranet', patron => $patron, }); which applies the EmbedItems record processor and, for the opac interface, scopes items through filter_by_visible_in_opac (plus the ViewPolicy filter). Suggested implementation outline: 1. Give print_collection a hashref-based signature that accepts embed_items (and interface/patron), choosing per element between $element->record and $element->metadata_record({ embed_items => 1, interface => ..., patron => ... }). 2. In the list controllers, read the embed list from the request (the koha.embed stash populated when x-koha-embed is sent) and pass embed_items through to print_collection. The public list controller must pass interface => 'opac' and the current patron so OPAC visibility rules apply; the staff controller passes interface => 'intranet'. 3. Declare the x-koha-embed header parameter with an "items" enum value on the listBiblio operation (and the equivalent public list operation) in the swagger spec, so the header is accepted instead of rejected with HTTP 400. This keeps serialization logic in the collection class, reuses the existing item-visibility scoping, and makes it possible to retrieve a page of biblios with their items in a single request rather than one request per record. Backwards compatibility: default behaviour (no x-koha-embed header) is unchanged and continues to return records without items. -- 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=43606 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- 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=43606 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|tomascohen@gmail.com |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206608 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206608&action=edit Bug 43606: Make print_collection accept a hashref This patch changes Koha::Objects::Record::Collections->print_collection to take a hashref instead of a positional format string, so it can be extended with new options without further signature changes. All callers are converted accordingly: - Koha::REST::V1::Biblios - Koha::REST::V1::DeletedBiblios - Koha::REST::V1::Authorities There are no functional changes; the generated output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/deleted_biblios.t \ t/db_dependent/api/v1/authorities.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206609&action=edit Bug 43606: Add x-koha-embed: items support to GET /biblios This patch adds support for embedding item data (952 fields) into the records returned by the GET /biblios list endpoint, driven by the 'x-koha-embed: items' request header. It works for all record formats served by the endpoint (marcxml, marc-in-json, marc and text/plain). Previously the list endpoint had no 'x-koha-embed' parameter declared, so sending the header resulted in a 400 response, and callers that needed items had to fetch them one biblio at a time (GET /biblios/{id}/items), an N+1 access pattern. Changes: - Declare the 'x-koha-embed' header parameter (enum: items) on the listBiblio operation in the swagger spec - Add embed_items/interface/patron options to print_collection. When embed_items is set, each element's record is obtained through metadata_record so items are embedded and scoped by interface - Read the embed from the stash and pass embed_items and interface => 'intranet' from the biblios list controller The items relation is already whitelisted for prefetching on Koha::Biblios, so the framework prefetches it from the embed header and no separate item query is issued per biblio. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! 3. Restart Plack and try it out: k$ koha-plack --restart kohadev Request GET /api/v1/biblios with: - Accept: application/marcxml+xml and no x-koha-embed header => no 952 fields - Accept: application/marcxml+xml and x-koha-embed: items => one 952 field per item 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206610&action=edit Bug 43606: Avoid redundant queries when embedding items When embedding items into a record, Koha::Filter::MARC::EmbedItems calls Koha::Item->as_marc_field on every item. Each call resolved the MARC structure on its own and, to get the framework code, accessed $item->biblio, which materialized the biblio object from the database once per biblio being processed. This patch avoids that redundant work: - Koha::Item->as_marc_field now accepts an optional 'tagslib' parameter (a MARC structure as returned by C4::Biblio::GetMarcStructure). When passed, it is used instead of resolving the structure internally - Koha::Filter::MARC::EmbedItems resolves the structure once per record, using the frameworkcode already provided by metadata_record in its options (so no biblio object is materialized), and passes it to each as_marc_field call There are no functional changes; the embedded output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t t/db_dependent/Koha/Item.pm => SUCCESS: Tests pass! 3. Confirm the query reduction: k$ DBIC_TRACE=1 perl -e ' use Koha::Biblios; Koha::Biblios->search( undef, { prefetch => [qw(biblioitem metadata items)], rows => 5 } ) ->print_collection( { format => q{marcxml}, embed_items => 1, interface => q{intranet} } ); ' 2>&1 | grep -c "FROM \`biblio\` .*biblionumber" => SUCCESS: 0 (no biblio object is materialized while embedding) 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, | |lisette@bywatersolutions.co | |m, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #4 from Lisette Scheer <lisette@bywatersolutions.com> --- Worked great! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206608|0 |1 is obsolete| | Attachment #206609|0 |1 is obsolete| | Attachment #206610|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=43606 --- Comment #5 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 206943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206943&action=edit Bug 43606: Make print_collection accept a hashref This patch changes Koha::Objects::Record::Collections->print_collection to take a hashref instead of a positional format string, so it can be extended with new options without further signature changes. All callers are converted accordingly: - Koha::REST::V1::Biblios - Koha::REST::V1::DeletedBiblios - Koha::REST::V1::Authorities There are no functional changes; the generated output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/deleted_biblios.t \ t/db_dependent/api/v1/authorities.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #6 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 206944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206944&action=edit Bug 43606: Add x-koha-embed: items support to GET /biblios This patch adds support for embedding item data (952 fields) into the records returned by the GET /biblios list endpoint, driven by the 'x-koha-embed: items' request header. It works for all record formats served by the endpoint (marcxml, marc-in-json, marc and text/plain). Previously the list endpoint had no 'x-koha-embed' parameter declared, so sending the header resulted in a 400 response, and callers that needed items had to fetch them one biblio at a time (GET /biblios/{id}/items), an N+1 access pattern. Changes: - Declare the 'x-koha-embed' header parameter (enum: items) on the listBiblio operation in the swagger spec - Add embed_items/interface/patron options to print_collection. When embed_items is set, each element's record is obtained through metadata_record so items are embedded and scoped by interface - Read the embed from the stash and pass embed_items and interface => 'intranet' from the biblios list controller The items relation is already whitelisted for prefetching on Koha::Biblios, so the framework prefetches it from the embed header and no separate item query is issued per biblio. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! 3. Restart Plack and try it out: k$ koha-plack --restart kohadev Request GET /api/v1/biblios with: - Accept: application/marcxml+xml and no x-koha-embed header => no 952 fields - Accept: application/marcxml+xml and x-koha-embed: items => one 952 field per item 4. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #7 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 206945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206945&action=edit Bug 43606: Avoid redundant queries when embedding items When embedding items into a record, Koha::Filter::MARC::EmbedItems calls Koha::Item->as_marc_field on every item. Each call resolved the MARC structure on its own and, to get the framework code, accessed $item->biblio, which materialized the biblio object from the database once per biblio being processed. This patch avoids that redundant work: - Koha::Item->as_marc_field now accepts an optional 'tagslib' parameter (a MARC structure as returned by C4::Biblio::GetMarcStructure). When passed, it is used instead of resolving the structure internally - Koha::Filter::MARC::EmbedItems resolves the structure once per record, using the frameworkcode already provided by metadata_record in its options (so no biblio object is materialized), and passes it to each as_marc_field call There are no functional changes; the embedded output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t t/db_dependent/Koha/Item.pm => SUCCESS: Tests pass! 3. Confirm the query reduction: k$ DBIC_TRACE=1 perl -e ' use Koha::Biblios; Koha::Biblios->search( undef, { prefetch => [qw(biblioitem metadata items)], rows => 5 } ) ->print_collection( { format => q{marcxml}, embed_items => 1, interface => q{intranet} } ); ' 2>&1 | grep -c "FROM \`biblio\` .*biblionumber" => SUCCESS: 0 (no biblio object is materialized while embedding) 4. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206943|0 |1 is obsolete| | Attachment #206944|0 |1 is obsolete| | Attachment #206945|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=43606 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208674 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208674&action=edit Bug 43606: Make print_collection accept a hashref This patch changes Koha::Objects::Record::Collections->print_collection to take a hashref instead of a positional format string, so it can be extended with new options without further signature changes. All callers are converted accordingly: - Koha::REST::V1::Biblios - Koha::REST::V1::DeletedBiblios - Koha::REST::V1::Authorities There are no functional changes; the generated output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/deleted_biblios.t \ t/db_dependent/api/v1/authorities.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208675 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208675&action=edit Bug 43606: Add x-koha-embed: items support to GET /biblios This patch adds support for embedding item data (952 fields) into the records returned by the GET /biblios list endpoint, driven by the 'x-koha-embed: items' request header. It works for all record formats served by the endpoint (marcxml, marc-in-json, marc and text/plain). Previously the list endpoint had no 'x-koha-embed' parameter declared, so sending the header resulted in a 400 response, and callers that needed items had to fetch them one biblio at a time (GET /biblios/{id}/items), an N+1 access pattern. Changes: - Declare the 'x-koha-embed' header parameter (enum: items) on the listBiblio operation in the swagger spec - Add embed_items/interface/patron options to print_collection. When embed_items is set, each element's record is obtained through metadata_record so items are embedded and scoped by interface - Read the embed from the stash and pass embed_items and interface => 'intranet' from the biblios list controller The items relation is already whitelisted for prefetching on Koha::Biblios, so the framework prefetches it from the embed header and no separate item query is issued per biblio. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/Koha/Objects/Record/Collections.t \ t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! 3. Restart Plack and try it out: k$ koha-plack --restart kohadev Request GET /api/v1/biblios with: - Accept: application/marcxml+xml and no x-koha-embed header => no 952 fields - Accept: application/marcxml+xml and x-koha-embed: items => one 952 field per item 4. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208676 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208676&action=edit Bug 43606: Avoid redundant queries when embedding items When embedding items into a record, Koha::Filter::MARC::EmbedItems calls Koha::Item->as_marc_field on every item. Each call resolved the MARC structure on its own and, to get the framework code, accessed $item->biblio, which materialized the biblio object from the database once per biblio being processed. This patch avoids that redundant work: - Koha::Item->as_marc_field now accepts an optional 'tagslib' parameter (a MARC structure as returned by C4::Biblio::GetMarcStructure). When passed, it is used instead of resolving the structure internally - Koha::Filter::MARC::EmbedItems resolves the structure once per record, using the frameworkcode already provided by metadata_record in its options (so no biblio object is materialized), and passes it to each as_marc_field call There are no functional changes; the embedded output is identical. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t t/db_dependent/Koha/Item.pm => SUCCESS: Tests pass! 3. Confirm the query reduction: k$ DBIC_TRACE=1 perl -e ' use Koha::Biblios; Koha::Biblios->search( undef, { prefetch => [qw(biblioitem metadata items)], rows => 5 } ) ->print_collection( { format => q{marcxml}, embed_items => 1, interface => q{intranet} } ); ' 2>&1 | grep -c "FROM \`biblio\` .*biblionumber" => SUCCESS: 0 (no biblio object is materialized while embedding) 4. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208677 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208677&action=edit Bug 43606: (QA follow-up) Prefetch items when embedding in GET /biblios When x-koha-embed: items is sent, Koha::Biblio->metadata_record fetches the items of each biblio separately, so the list endpoint still issued one items query per biblio (N+1). This patch prefetches the items relationship when the embed is requested, so the items are loaded in the same query as the biblios. It also builds the print_collection parameters (embed_items, interface) once and reuses them across the output formats, instead of repeating the hash in every branch. There are no functional changes to the output. Benchmark (200 biblios x 5 items each, marcxml, embed_items + intranet, one process per run, local KTD, identical output in both cases): | Rows | Variant | Queries | Time | Peak RSS | |------|----------------|---------|--------|----------| | 50 | no prefetch | 51 | 0.236s | 273.3 MB | | 50 | prefetch items | 1 | 0.182s | 273.5 MB | | 200 | no prefetch | 201 | 0.775s | 309.5 MB | | 200 | prefetch items | 1 | 0.533s | 310.0 MB | Queries go from N+1 to 1, time drops by about 25-30%, and memory is unchanged (+0.1%). Test plan: 1. Apply this patch and rebuild/restart (yarn build; restart_all) 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! 3. Compare the query count with and without the items prefetch using DBIC_TRACE=1 against GET /api/v1/biblios with 'x-koha-embed: items' and Accept: application/marcxml+xml 4. Sign off :-D Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43606 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208678 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208678&action=edit Bug 43606: (QA follow-up) Test x-koha-embed: items for all record formats The existing test only exercised application/marcxml+xml. This patch adds coverage for the other formats served by GET /biblios: application/marc-in-json, application/marc and text/plain. Each checks that one 952 field is embedded per item, and that none are embedded when the header is not sent. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! 3. Sign off :-D Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org