CCD-6129:#2893
Conversation
| Long[] caseReferences = searchRoleAssignments.stream() | ||
| .map(SearchRoleAssignment::getCaseReference) | ||
| .collect(Collectors.toList())); | ||
| .filter(ref -> ref != null && ref.matches("\\d+")) |
There was a problem hiding this comment.
While .matches("\d+") ensures the string is numeric, if an invalid digit string sneaks into the role assignments, Long.parseLong will throw a NumberFormatException and crash the entire search transaction (HTTP 500).
You can use CaseReferenceValidator::validateUID. It is strictly enforces a 16-digit length, it mathematically guarantees Long.parseLong() will never throw an exception.
| .filter(ref -> ref.matches("\\d+")) | ||
| .map(Long::parseLong) |
There was a problem hiding this comment.
If we use a filter to silently drop invalid/corrupted case IDs, we create two different security outcomes:
-
In Grant builders: Silently dropping a bad ID is safe. The user simply fails to get access to the malformed case (Fail-Secure).
-
In the Excluded builder: Silently dropping a bad ID is a security risk. The AND NOT SQL clause is never generated, meaning the user retains access to a case they were explicitly forbidden from seeing (Fail-Open).
In ExcludedGrantTypeQueryBuilder, do not silently filter out invalid IDs. If an exclusion ID fails validateUID(), you must explicitly handle it. either by throwing an exception (failing the search entirely so the corrupt assignment can be fixed) or by logging a severe warning.
|
|
||
| // Used for reference lists to avoid PostgreSQL's 65,535 parameter limit | ||
| // = ANY() treats the entire array as a single parameter unlike IN which expands each element | ||
| public static final String QUERY_ANY = "%s = ANY(ARRAY(SELECT unnest(CAST(:%s AS bigint[]))))"; |
There was a problem hiding this comment.
The new template is slightly redundant: = ANY(ARRAY(SELECT unnest(CAST(:%s AS bigint[]))))
This round-trips the parameter from an array → set → array for no functional effect.
Please check this out again to find a simpler way
| // Previously the IN clause expanded each case reference into an individual parameter, | ||
| // causing PSQLException: PreparedStatement can have at most 65,535 parameters. | ||
| List<RoleAssignment> roleAssignments = new ArrayList<>(); | ||
| for (int i = 0; i < 70000; i++) { |
There was a problem hiding this comment.
you can safely drop the 70k loop down to ~3 items. Since there is no database connected in the unit test, it can't reproduce the Postgres exception anyway. Testing a few items is enough to prove the = ANY() syntax is generated.
Note: If you switch to using validateUID(), make sure the mock data generates valid 16-digit case references instead of String.valueOf(i)
|
|
||
| // Create 70,000 role assignments to simulate a user with a large number of specific case assignments | ||
| List<RoleAssignmentResource> roleAssignmentRecords = new ArrayList<>(); | ||
| for (int i = 0; i < 70000; i++) { |
There was a problem hiding this comment.
Just keep an eye on CI memory, as serializing 70k objects in WireMock is heavy
2 participants
*### JIRA link (if applicable) ###
https://tools.hmcts.net/jira/browse/CCD-6129
Change description
Fixes a PSQLException thrown when users with large numbers of specific case-level role assignments search for cases. The JDBC driver rejected queries where the IN (:p1, :p2, ...) clause exceeded PostgreSQL's 65,535 parameter limit.
Replaced the IN clause with = ANY(ARRAY(SELECT unnest(CAST(:references_param AS bigint[])))) in GrantTypeSqlQueryBuilder and ExcludedGrantTypeQueryBuilder, passing case references as a single bigint[] array parameter instead of individual JDBC parameters.
Does this PR introduce a breaking change? (check one with "x")