Manage Guestbook APIs to Edit Guestbooks and retrieve Guestbook Respo…#12395
Manage Guestbook APIs to Edit Guestbooks and retrieve Guestbook Respo…#12395stevenwinship wants to merge 13 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
606dc5f to
fbe7b2e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test Results398 tests +1 382 ✅ ±0 37m 20s ⏱️ + 12m 9s For more details on these failures, see this check. Results for commit bc94b4c. ± Comparison against base commit 0a1817f. ♻️ This comment has been updated with latest results. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Claude Code review follows-- PR #12395 Review: Manage Guestbook APIs to Edit Guestbooks and Retrieve Guestbook Responses Author: stevenwinship | Branch: 12386-manage-guestbook-apis → develop | Milestone: 6.11 Overview This PR adds two new API endpoints:
It also improves JSON serialization with null-safety checks and better formatting. Issues Found
File: GuestbookResponseServiceBean.java TypedQuery query = em.createQuery( The guestbookId is concatenated directly into the JPQL string. While it's a Long (mitigating classic SQL injection), this is still a bad pattern — it bypasses the query cache and is Note: This is a pre-existing issue (the old code had it too), but the refactoring was an opportunity to fix it.
File: Guestbooks.java, updateGuestbook() method The updateGuestbook method does not perform an explicit permission check before executing the command. It relies on the UpdateGuestbookCommand to enforce permissions internally. Compare this
File: JsonPrinter.java guestbookResponseObject.add("dataset", gbResponse.getDataset().getDisplayName()); If getDataset(), getDataFile(), or getEventType() returns null, this throws NPE. The existing code protects getAuthenticatedUser(), getEmail(), etc. with null checks — these fields should be
File: doc/release-notes/12386-manage-guestbook-apis.md and native-api.rst The curl example for the responses endpoint has ?limit10&offset=0 — missing the = sign after limit:
File: Guestbooks.java import edu.harvard.iq.dataverse.*; The explicit imports were replaced with a wildcard import. This is generally discouraged as it can cause ambiguity and makes dependencies less clear. The codebase style appears to prefer
File: Guestbooks.java, getResponses() method if (next < totalResponseCount) { The totalResponseCount comes from findCountByGuestbookId(guestbook.getId(), dataverse.getId()) which filters by dataverse, while the actual response list comes from Code Quality & Style
Suggestions
Summary The PR delivers useful functionality and follows the project's existing patterns well. The main concerns are: (1) the JPQL concatenation should use a parameter, (2) potential NPEs in The pr addresses the immediate issue. The question is whether we should follow up on the issues Claude raises, especially the High and Medium concerns. I lean toward doing that |
rtreacy
left a comment
There was a problem hiding this comment.
per the discussion this morning, moving issues that go beyond current fix to another issue and approving
|
📦 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: The user needs the ability to edit a Guestbook after creation (JSF allows this). Also an API is needed to retrieve the Guestbook Responses with more detailed Guestbook metadata and Guestbook Response metadata than the original API produced.
Which issue(s) this PR closes:#12386
Special notes for your reviewer:
Suggestions on how to test this:
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: