Fix pagination stats for Guestbook Responses - #12557
Conversation
This comment has been minimized.
This comment has been minimized.
14343b0 to
ac9c11b
Compare
This comment has been minimized.
This comment has been minimized.
rtreacy
left a comment
There was a problem hiding this comment.
Claude
Overview
Fixes a bug (#12523) where GET /api/guestbooks/{id}/responses returned pagination totals (totalResponses, usage count) that undercounted responses/usages from datasets in child dataverses when a guestbook defined on a parent dataverse is applied to a dataset further down the hierarchy. Core fix is a 2-line change in Guestbooks.java, plus a new integration test and a small test-utility fix to avoid guestbook-name collisions.
Root cause analysis (traced through the code)
- Guestbooks.getResponses() sets dataverse = guestbook.getDataverse() — always the guestbook's owning dataverse, never a descendant.
- The old code called findCountUsages(guestbookId, dataverse.getId()) / findCountByGuestbookId(guestbookId, dataverse.getId()), whose non-null-dataverseId branches filter with obj.owner_id = dataverseId — i.e., only datasets directly owned by the guestbook's home dataverse. Datasets in child/grandchild dataverses using the same (inherited) guestbook were silently excluded from the totals.
- Meanwhile, the actual list of returned responses (findAllByGuestbookId) was already filtered by guestbook.id alone, with no dataverse scoping — so the list already included the full hierarchy while the pagination totals didn't. That's exactly the reported symptom (list/total mismatch).
- Passing null for dataverseId routes both count methods into their already-existing "guestbookId only, no dataverse restriction" branches, bringing the totals back in line with the list query. This is a minimal, correct, surgical fix.
- Confirmed this change is scoped only to this one call site (Guestbooks.java:190-193); other callers of the same two methods (ManageGuestbooksPage.java, GuestbookResponsesPage.java, Guestbooks.java:93-94) are untouched and keep their existing dataverse-scoped semantics, so no unintended behavior change elsewhere.
Test coverage
- New FilesIT.testDownloadFileWithParentGuestbookResponse correctly reproduces the bug: creates a parent dataverse + child dataverse, creates a dataset in the child, creates a guestbook in the parent and explicitly assigns it to the child's dataset (updateDatasetGuestbook), then asserts responseListSize == totalCountFromJson after a download. This is exactly the right regression test for the fix.
- Minor: the test creates a second "user with no permission" (apiToken/username at line ~64-68) that's never referenced afterward — looks like leftover copy-paste from the neighboring testDownloadFileWithGuestbookResponse test. Harmless but dead code; worth trimming.
- The guestbook-test.json name placeholder ({@} → UUID via createRandomGuestbook) is a reasonable supporting fix, presumably needed to avoid a guestbook-name collision now that tests create more guestbooks per run. Checked the one other direct consumer of that JSON file's literal name (GuestbooksIT.testGuestbook's string-replace of "my test guestbook" → "my modified test guestbook") — still works correctly since it's a substring replace, not exact-match, so no regression there.
Other notes
- No API/security concerns: the endpoint's permission check (EditDataverse on the guestbook's owning dataverse) is unchanged; removing the dataverse scoping only affects which rows are counted, not who can call the endpoint. Since a guestbook can only be assigned to datasets in its own dataverse or descendants by design, an unscoped count is exactly the right scope for "this guestbook, wherever it's used."
- Release note included and accurately describes the fix.
Summary
This is a well-targeted, minimal bug fix with a solid regression test. Only nit is the unused second user in the new test — not blocking.
ac9c11b to
10100c3
Compare
This comment has been minimized.
This comment has been minimized.
10100c3 to
30c4475
Compare
This comment has been minimized.
This comment has been minimized.
rtreacy
left a comment
There was a problem hiding this comment.
Appoved as per Claude comments
30c4475 to
1b05a02
Compare
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |


What this PR does / why we need it: Pagination totals reported in API /api/guestbooks/{id}/responses were not including all stats from the full hierarchy of parent collections
Which issue(s) this PR closes:#12523
Special notes for your reviewer:
Suggestions on how to test this: See Issue. Also see FilesIT testDownloadFileWithParentGuestbookResponse
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: included
Additional documentation: