[CIRCSTORE-647] Add Request Anonymization Endpoint

[brycekbargar]

I took the code from the Anonymizing Loans codepaths and used it to implement Anonymization for requests. Reusing the loan implementation carries over at least one weirdness where we don't actually check if an id can be anonymized, we just don't anonymize it and tell the user we did. There's probably more that I'm not aware of.

I'm picking up this work because the initial developers have other commitments now. This PR is I think implementing this ticket: https://folio-org.atlassian.net/browse/CIRCSTORE-647 .

[roman-barannyk]

Summary

• I loaded the PR-reviewer ability and fetched the changed files from PR [CIRCSTORE-647] Add Request Anonymization Endpoint #581 (Add Request Anonymization Endpoint). I looked through the Java implementation, tests, and small test-support changes and ran a focused code review below.
• Purpose: add POST /anonymize-storage-requests to anonymize a list of request IDs (re-using the loans anonymization approach).
• Scope: 6 files changed, ~553 lines added (implementation + tests + test helpers + NEWS).
• High-level risk: Medium — new DB-updating endpoint that currently reports IDs as anonymized even when they are not actually changed; tests cover many cases but the implementation has a few correctness and robustness issues to address.

Core changes

• New API implementation: src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java
  • Validates UUIDs, groups valid/invalid IDs, builds a combined SQL UPDATE that removes requester/proxy fields for closed requests, executes it, and responds with anonymizedRequests equal to the valid UUID list.
• Tests: src/test/java/org/folio/rest/api/AnonymizeRequestsApiTest.java plus test support:
  • Request matchers, InterfaceUrls entries, and builder helpers added to exercise the new endpoint and assert behavior.
• Small test-support changes: RequestRequestBuilder extended (from(JsonObject), closed()/closed(reason)), InterfaceUrls for request storage & anonymize URL, plus RequestMatchers for anonymized state.

Other notable changes

• NEWS.md: added line for CIRCSTORE-647.
• Tests added to ensure:
  • can anonymize closed requests (and repeated calls are idempotent),
  • open requests are left unchanged (but the endpoint currently still returns their IDs as anonymized — carried over from loan implementation),
  • invalid UUIDs are reported in NotAnonymizedRequests with reason "invalidRequestIds", and empty list returns 422.

Merge readiness and risk assessment

• Observations (non-blocking):
  • Tests cover expected behavior and the new test helpers are consistent with other suite patterns.
  • Logging is present for invalid UUIDs and anonymization count.
  • CI status unknown to me (I didn't run CI locally).
• Risks:
  • Functional correctness: the API returns every valid UUID in anonymizedRequests even if the SQL did not update any rows (e.g., open or already-anonymized requests). That may mislead API consumers.
  • Potential NPE / robustness issues around null lists in the response builder.
  • SQL-building approach is string-concatenation; while UUIDs are validated, constructing long IN lists may be fragile or inefficient at scale.

Critical issues to address (actionable, mapped to specific places in the diff)

• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — postAnonymizeStorageRequests: validate request.getRequestIds() for null/empty before using it and return 422 or an appropriate error; currently requestIds is used directly and could NPE if null.
• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — addToNotAnonymizedRequests: guard against response.getNotAnonymizedRequests() being null or ensure the response model always provides a non-null list (initialize if necessary) before calling .add(...); otherwise this can NPE in runtime tests or production.
• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — postAnonymizeStorageRequests: do not return the full validIds list as "anonymizedRequests" unless those rows were actually updated; use the SQL update result (number of rows updated) or run a SELECT to determine which IDs were changed, and report not-anonymized IDs with a suitable reason (e.g., "notAnonymizable" or "notClosed"). This is a behavioral bug: tests currently assert and document the old loans behavior, but the API semantics are surprising and likely incorrect for callers.
• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — createAnonymizationSQL / executeSql: avoid constructing an IN-list by string concatenation at scale; use a parameterized/prepared statement or a temporary table/unnest to pass the UUID list to Postgres safely and avoid hitting SQL length limits. Even though UUIDs are validated, parameterization improves safety and performance.

Possible improvements (actionable suggestions tied to diff locations)

• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — createAnonymizationSQL: consider casting the array in the SQL where you remove keys (e.g., jsonb - ARRAY[...]::text[]) only if required by Postgres version, and add tests for the SQL expression in different Postgres versions used by the project to ensure portability.
• src/main/java/org/folio/rest/impl/AnonymizeStorageRequestsAPI.java — logging: include more context in the success/failure logs (e.g., tenant, number of matched rows, SQL error class) to make production troubleshooting easier.
• src/test/java/org/folio/rest/api/AnonymizeRequestsApiTest.java — add an assertion that the API’s reported anonymizedRequests equals the set of actually-anonymized ids (once behavior is fixed) — this will prevent regressions where the API reports changed rows but DB didn't change.
• src/test/java/org/folio/rest/support/builders/RequestRequestBuilder.java — from(JsonObject): consider defensive checks when reading nested objects (requester/proxy) to avoid surprise NPEs if fields are missing in future tests.

Explanation and suggested code-level fixes

• To avoid returning IDs that were not actually anonymized:
  • Option A (preferred): After running the UPDATE, run a SELECT on the request table filtering for ids in the input list and where requesterId/proxyUserId/requester/proxy IS NULL — that gives the actual anonymized IDs; compute notAnonymized = validIds - actuallyAnonymized and include those with reason "notAnonymizable" (or "notClosed"). Use the RowSet returned by the SELECT to build the list. This avoids attempting to infer success purely from input.
  • Option B: Use the UPDATE ... RETURNING id to get the list of IDs that were updated. Postgres supports RETURNING; change the UPDATE to add "RETURNING id" and use postgresClient to execute and collect returned ids. That is efficient and clear.
• To avoid SQL concatenation and support parameterization:
  • Use PostgresClient’s prepared execution or create a temporary table/unnest to pass an array of UUIDs as a parameter and join against it. Example pattern: WITH ids AS (SELECT unnest($1::uuid[]) AS id) UPDATE ... WHERE request.id IN (SELECT id FROM ids) RETURNING id.
• To prevent NPE on response list:
  • Ensure AnonymizeStorageRequestsResponse has a non-null notAnonymizedRequests list, or create it before use:
    • if (response.getNotAnonymizedRequests() == null) response.setNotAnonymizedRequests(new ArrayList<>());
    • Or change addToNotAnonymizedRequests to response.withNotAnonymizedRequests(...).

Minor / stylistic

• RequestRequestBuilder: making PatronSummary static is good. The new from(JsonObject) method is long — consider splitting into helper methods for readability.
• Tests: The comments in onlyAnonymizesOpenRequests note the inherited odd behavior; if you plan to keep the old behavior for backwards compatibility, add a clear comment in the API JavaDoc describing the behavior (that the response lists valid IDs even if not changed). Preferably fix the behavior and update tests to assert actual anonymization.

[brycekbargar]

@julianladisch @roman-barannyk

Hello, is there any further feedback for this PR? Thank you.

[alexanderkurash]

@brycekbargar I've re-created this PR to make the checks pass (#590), but test coverage is still very low at<2% (new code coverage threshold is 80%).

Here's AI analysis of this PR, seems to be correct:

The key bit is in pom.xml (line 540): Surefire excludes org/folio/rest/api/**/*Test.java, then runs StorageTestSuite.java (line 55). That suite explicitly lists test classes, and it includes AnonymizeLoansApiTest.class but not the new AnonymizeRequestsApiTest.class at lines 56-87.
So CI coverage likely never executes AnonymizeRequestsApiTest.java (line 32), which explains why AnonymizeStorageRequestsAPI.java (line 37) shows as uncovered.

[brycekbargar]

The key bit is in pom.xml (line 540): Surefire excludes org/folio/rest/api/**/*Test.java, then runs StorageTestSuite.java (line 55). That suite explicitly lists test classes, and it includes AnonymizeLoansApiTest.class but not the new AnonymizeRequestsApiTest.class at lines 56-87. So CI coverage likely never executes AnonymizeRequestsApiTest.java (line 32), which explains why AnonymizeStorageRequestsAPI.java (line 37) shows as uncovered.

@alexanderkurash Thank you for pointing me to the test suite file. I had been explicitly selecting the tests with mvn test -Dtest=AnonymizeRequestsApiTest and wasn't aware there was a place I had to configure it to run as part of CI.

[brycekbargar]

Thank you for the feedback @alexanderkurash ! I've made and pushed the changes.