https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42835 --- Comment #83 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Hi Luis (and all), First, a genuinely big thank you. This is an impressive piece of work. The nested design is the right one, the reactive reindexing is thorough (checkouts and returns, holds, transfers, stock rotation, library geolocation), the test coverage is extensive, and the commit messages explain the why very well. What follows is a QA review plus a proposal for getting this into community Koha. Everything below is fixable and none of it undermines the design. I would be glad to help you through the process. == How this was reviewed == Range 541cbd84a53..cc618963d67 of https://github.com/xercode/koha-item-index (branch bugXXXXX-nested-items): 38 commits, 47 files, +5625/-208 lines. I ran koha-qa.pl with --more-tests in KTD (Elasticsearch 9.4), reviewed it area by area (indexing, query/security, templates and preferences, tests, error handling), and ran it against the 250k biblio / 1.26M item catalogue from the benchmark. The review used AI-assisted tooling, and I reproduced the key findings by hand. The branch applies cleanly to main as of 16 October. With the item-limit forms the UI emits (items.itype:, mc-itype:, items.available:true, items.available_for_patron:true) every query I tried returned exactly the expected hit counts, in 110-210 ms cold. Thank you for the explanation of available versus available_for_patron in comment 81, it cleared up several things I had misread. == Blockers (need fixing before this can go in) == 1. OPAC hidden items can be matched through crafted limits. _extract_item_filters accepts any items.<field>:<value> limit, and _item_filter_clauses OR-merges filters on the same field, including the automatic OPAC hiding filters, which share that namespace. I reproduced it: with OpacHiddenItems set so that a record's only item is hidden, an anonymous OPAC search returns 0 hits normally and with a legitimate items.homebranch limit, but 1 hit with limit=items.hidden_in_opac_by_preferences:true (and likewise items.lost:1 against hidelostitems). Item details are still blanked at display time, but record presence, hit counts and facet counts leak, and the OPAC then redirects to opac-detail.pl?biblionumber= with an empty id. Suggested fix: allowlist the items.* fields a user limit may reference (the facet fields, available, available_for_patron, onrequest, geo, and the operand fields), and never merge the automatic hiding filters into user filter slots (build them as separate clauses). 2. No upgrade path. The items nested mapping is only added when an index is created. items_schema_version is written to documents but is never read, and the item facet aggregations are built as nested unconditionally. On an existing index, searches would error until rebuild_elasticsearch.pl -b -r is run, and a rebuild without -r cannot fix the dynamically mapped items field. Suggested fix: a query-side guard that falls back to the current behaviour when the mapping or schema version is missing, plus a clear warning in the atomicupdate and release notes. 3. Zebra. The new on-request and patron-availability facets render on Zebra installs, and Zebra cannot parse items.onrequest:* (CCL error). items.available_for_patron:true only works by accident, because the broad /available/ match in C4::Search::buildQuery turns it into the old availability limit. Suggested fix: guard the facets with the SearchEngine preference, or map the limits explicitly in buildQuery. == Important == * A plugin that dies in item_hidden_in_opac is treated as "not hidden", so items become visible (fails open). Please fail closed and log at error level. The same applies to item_available_for_patron. $plugin->get_metadata inside the error branch can itself escape the eval and kill the whole batch. * A library limit (branch:) combined with an items.homebranch facet is OR-merged rather than AND-ed (_library_limit_to_item_filter feeds the same equality slot), so the combination widens instead of narrowing. * StaffSearchResultsShowOnlyMatchingItems defaults to 1, which changes staff behaviour on upgrade. Koha prefers defaults that preserve existing behaviour. The pref text also mentions inner_hits; plain wording will be friendlier for librarians. * Elasticsearch defaults to a maximum of 10,000 nested objects per document, which contradicts the serials use case in index_config.yaml. Please set index.mapping.nested_objects.limit explicitly (or document the cap) and make sure bulk item errors are surfaced rather than a record silently dropping out of the index. * Changes to OpacHiddenItems, plugins and item-level_itypes leave the index stale until a reindex. This is documented in POD only. A warning on the preferences, or a reindex job as for geolocation, would help. * Every hold place, cancel, fill and confirm now enqueues a whole-record reindex. That is worth benchmarking on a hold-heavy system, and perhaps debouncing. * Several post-commit index_records calls (Hold, Transfer, StockRotationItem, AddReturn, ModItemTransfer) and Library::store are unguarded, so a queue or Elasticsearch failure turns into an error after the circulation write has already committed. Please wrap them in try/catch and log. The geolocation job also reports "finished" on failure, and "success" there means enqueued rather than indexed. * Perl-built user-visible strings (_describe_item_filters) are not translatable. * Barcode becomes an exact term and call number a *term* wildcard on keyword subfields, so matching becomes case sensitive where the old analysed fields were not. A lowercase normaliser would fix this. * The live-Elasticsearch test (NestedItems.t) never pings Elasticsearch before creating an index, has no Test::NoWarnings, and leaves its index behind if it dies. There are no tests yet for facet and count leakage under hidden items, escaping of limits and operands, the upgrade case, a plugin that dies, or SearchLimitLibrary set to both against a live index. * koha-qa.pl: QueryBuilder.pm has non-ASCII dashes in POD before any =encoding (use -- or add =encoding utf8); tidiness fails on C4/Circulation.pm, facets.inc, advsearch.tt, opac-facets.inc, opac-advsearch.tt and t/db_dependent/Circulation.t (perl misc/devel/tidy.pl <file>). * Bug numbers: every commit and bug_XXXXX.pl must carry the real bug number. The four fixup commits (exec bits, copyright notices, tidy, return undef) should be squashed into the commits they amend so that each commit passes QA on its own. One commit message (df956bb5a95) refers to internal phase/step names and an unrelated bug number. == Suggestions == * A leading-wildcard *term* on call numbers is expensive on large catalogues; consider an ngram or prefix approach. * max_inner_result_window of 50000 raises a protective limit 500 times; consider a lower window plus the existing graceful truncation. * Validate geo lat/lon/distance (non-numeric currently becomes 0,0 or an ES parse error). * The C4/Search.pm multibranchlimit fix for Zebra is useful but is a Zebra behaviour change; better as its own tiny bug. * The per-record Koha::Items count for the top-level available flag is now derivable from the item documents already built in memory. * Warn once per plugin per batch rather than once per item when a plugin dies during a full rebuild. * _item_marc_to_field runs on the result-decode path; a malformed marc string would kill the whole page, so a small guard would be cheap insurance. * Whitespace-only churn in Circulation.t and Items.t, and a duplicated id="available-items" in opac-advsearch.tt. == What is already solid == Trigger coverage for reactive reindexing; the atomicupdate (executable, idempotent, Koha::Installer::Output); the TT escaping; POD on the new subs; no SQL injection (everything is parameterised DBIC and a structured DSL); OpacHiddenItemsExceptions is honoured; truncated inner_hits degrade safely; and the commit messages with their test plans. == Suggested split == 38 commits and about 2,900 non-test lines is a lot for reviewers and testers. Several pieces are genuinely separate enhancements (the two new plugin hooks, the staff "show only matching items" feature, the on-request filter, geolocation indexing) and will move faster on their own. The reactive reindexing, on the other hand, belongs with the foundation: without it the nested items would go stale and the feature would not stand on its own. A proposed split, in dependency order (happy to adjust it with you): 1. Bug 42835 (this bug, repurposed): nested items in the biblio document, together with the reindexing that keeps them correct. Mapping and nested item documents with statuses, available and items_count, acquisition date and effective item type; the reindex triggers for transfers, holds, stock rotation and returns blocked by withdrawn items; the live-ES tests; plus the upgrade guard and the nested-object limit. Roughly commits 1, 2, 5 (without the geolocation job), 13, 15, 27, 28, 29, 30, 31 and the availability part of 6. 2. NEW: index library geolocation of items and reindex when it changes (the UpdateBiblioItemsGeolocation job), plus the fix that keeps the rest of the query when searching by geolocation. The geolocation part of 5, and 24. Depends on 1. 3. Bug 43151 (repurposed; it asks for filters to be applied to each item discretely): same-item filters and facets on nested items, including library limits and groups. Roughly commits 3, 4, 7, 10, 12, 14, 22, 23. Depends on 1. 4. NEW: advanced search item fields and operands on nested items (item type, collection, location, barcode, call number, acquisition date) and saved-filter routing. Roughly commits 16, 17, 25, 26. Depends on 3. This also addresses the long-standing complaint in bug 8589. 5. Bug 35052 (repurposed): OpacHiddenItems / OpacHiddenItemsHidesRecord at search time on nested items, with the field allowlist from blocker 1. Roughly commits 8, 9, 32. Depends on 3. 6. NEW: plugin hooks item_hidden_in_opac and item_available_for_patron (new in this branch; neither exists in current main), with the hidden_in_opac_custom and available_for_patron fields, and fail-closed handling. Roughly commits 18, 34 and the hook part of 6. Depends on 5. 7. NEW: staff search "show only matching items" (inner_hits, StaffSearchResultsShowOnlyMatchingItems and its atomicupdate). Roughly commit 19. Depends on 3. 8. NEW: on-request limit in OPAC and staff search. Roughly commit 11, with the Zebra guard. Depends on 3. 9. NEW: store each item's MARC in the nested items and stop embedding items when reading records to index. Roughly commits 35, 36, 37. This is a performance change with its own review surface (Koha::Item->as_marc_field, BiblioUtils), so it is better reviewed once the foundation is in. Depends on 1. 10. NEW (tiny): C4::Search multibranchlimit with an unknown group no longer dies on Zebra. Independent of everything else. The four fixup commits (exec bits, copyright notices, tidy, return undef) should be squashed into the commits they amend so that each commit passes QA on its own. One commit message (df956bb5a95) refers to internal phase/step names and an unrelated bug number. == Suggestions == * A leading-wildcard *term* on call numbers is expensive on large catalogues; consider an ngram or prefix approach. * max_inner_result_window of 50000 raises a protective limit 500 times; consider a lower window plus the existing graceful truncation. * Validate geo lat/lon/distance (non-numeric currently becomes 0,0 or an ES parse error). * The C4/Search.pm multibranchlimit fix for Zebra is useful but is a Zebra behaviour change; better as its own tiny bug. * The per-record Koha::Items count for the top-level available flag is now derivable from the item documents already built in memory. * Warn once per plugin per batch rather than once per item when a plugin dies during a full rebuild. * _item_marc_to_field runs on the result-decode path; a malformed marc string would kill the whole page, so a small guard would be cheap insurance. * Whitespace-only churn in Circulation.t and Items.t, and a duplicated id="available-items" in opac-advsearch.tt. == What is already solid == Trigger coverage for reactive reindexing; the atomicupdate (executable, idempotent, Koha::Installer::Output); the TT escaping; POD on the new subs; no SQL injection (everything is parameterised DBIC and a structured DSL); OpacHiddenItemsExceptions is honoured; truncated inner_hits degrade safely; and the commit messages with their test plans. == Suggested split == 38 commits and about 2,900 non-test lines is a lot for reviewers and testers. Several pieces are genuinely separate enhancements (the two new plugin hooks, the staff "show only matching items" feature, the on-request filter, geolocation indexing) and will move faster on their own. A proposed split, in dependency order (happy to adjust it with you): 1. Bug 42835 (this bug, repurposed): index items as nested objects in the biblio document. Mapping and nested item documents with statuses, available and items_count, acquisition date, effective item type, live-ES tests, plus the upgrade guard and nested-object limit. Roughly commits 1, 2, 13, 15, 31 and the availability part of 6. 2. NEW: reindex the record when item-related state changes. Transfers, holds, stock rotation and returns blocked by withdrawn items. Roughly commits 5 (without the geolocation job), 27, 28, 29, 30. Depends on 1. 3. NEW: index library geolocation of items and reindex when it changes (the UpdateBiblioItemsGeolocation job), plus the fix that keeps the rest of the query when searching by geolocation. Part of 5, and 24. Depends on 1. 4. Bug 43151 (repurposed; it asks for filters to be applied to each item discretely): same-item filters and facets on nested items, including library limits and groups. Roughly commits 3, 4, 7, 10, 12, 14, 22, 23. Depends on 1. 5. NEW: advanced search item fields and operands on nested items (item type, collection, location, barcode, call number, acquisition date) and saved-filter routing. Roughly commits 16, 17, 25, 26. Depends on 4. This also addresses the long-standing complaint in bug 8589. 6. Bug 35052 (repurposed): OpacHiddenItems / OpacHiddenItemsHidesRecord at search time on nested items, with the field allowlist from blocker 1. Roughly commits 8, 9, 32. Depends on 4. 7. NEW: plugin hooks item_hidden_in_opac and item_available_for_patron (new in this branch; neither exists in current main), with the hidden_in_opac_custom and available_for_patron fields, and fail-closed handling. Roughly commits 18, 34 and the hook part of 6. Depends on 6. 8. NEW: staff search "show only matching items" (inner_hits, StaffSearchResultsShowOnlyMatchingItems and its atomicupdate). Roughly commit 19. Depends on 4. 9. NEW: on-request limit in OPAC and staff search. Roughly commit 11, with the Zebra guard. Depends on 4. 10. NEW: store each item's MARC in the nested items and stop embedding items when reading records to index. Roughly commits 35, 36, 37. This is a performance change with its own review surface (Koha::Item->as_marc_field, BiblioUtils), so it is better reviewed once the foundation is in. Depends on 1. 11. NEW (tiny): C4::Search multibranchlimit with an unknown group no longer dies on Zebra. Independent of everything else. The four fixup commits (20, 21, 33, 38) get squashed into the commits they amend. Commits 5, 6, 9, 10 and 19 mix themes and will need splitting (git rebase -i and git add -p). If it would help, I am happy to prototype the split as a stack of branches for you to review and adopt. == How to submit this to the community == A walk-through of what the community expects, so that you are not guessing: 1. File one bug per piece in Bugzilla (product Koha, component "Searching - Elasticsearch" for most of them), each with a one-sentence summary, a description of the problem and the approach, and a test plan. Link them with "Depends on" in the order above. Re-using 42835, 43151 and 35052 where they fit keeps the history together. 2. Make each bug a small, self-contained patch series. One commit per logical change, subject "Bug NNNNN: Short summary", body explaining why. Never leave "Bug XXXXX" in commits, filenames or code comments. Put the test plan in the commit message of the first patch (or a comment) so testers can follow it step by step. 3. Attach patches with git bz (git bz attach), which sets the right patch flags and status "Needs Signoff". git bz create can also create the bug. 4. Before attaching, in KTD: run perl misc/devel/tidy.pl on touched files, koha-qa.pl -c <number of commits>, and the tests (prove) for everything you touched. Each commit should pass on its own, so squash fixups and keep the series bisectable. 5. Tests: use Test::NoWarnings, wrap DB tests in a transaction, skip cleanly when an optional service is not available, and add tests for security-relevant behaviour (hidden items, escaping) as well as the happy path. 6. DB and preferences: atomicupdate/bug_NNNNN.pl (executable, idempotent), matching entry in sysprefs.sql and the .pref file, and a default that preserves existing behaviour. Where an upgrade needs a reindex, say so in the "Text to go in the release notes" field on the bug. 7. Keep Zebra working: anything Elasticsearch-only needs to be guarded in templates and code. 8. Expect the sequence Needs Signoff, then a tester adds a Signed-off-by, then QA (Passed QA or Failed QA with comments), then the release manager pushes. Failed QA is a normal part of the process, not a rejection; respond to comments on the bug and re-attach. 9. Useful references: the wiki pages "Commit messages", "Coding guidelines" and "Submitting a patch", and the Koha manual's section on contributing. I am glad to review each bug as it arrives, help with the splitting, and test on a large dataset. Pedro and I will also look at what from our earlier series is worth carrying over on top (the availability definition for notforloan, damaged and withdrawn items, and the OPAC N+1 visibility batching). Thanks again for the quality of this work, I am looking forward to getting it in. -- You are receiving this mail because: You are watching all bug changes.