Implement Generalized Filtering and Sorting for the Prod Server#17
Implement Generalized Filtering and Sorting for the Prod Server#17jloux-brapi wants to merge 12 commits into
Conversation
…rrors on serialization
Additionally add active as a filter column
| Map.entry("studyDbId", "id"), | ||
| Map.entry("studyLocation", "location.id"), | ||
| Map.entry("trialDbId", "trial.id"), | ||
| Map.entry("studyType", "studyName"), |
There was a problem hiding this comment.
Are both studyType and studyName intended to map to studyName?
There was a problem hiding this comment.
Yes, that seemed to be the original intent in the previous implementation of the prod/test server for single column filtering on studies.
There was a problem hiding this comment.
Wonder if that was an oversight because the studyType filter uses studyType, not studyName.
nickpalladino
left a comment
There was a problem hiding this comment.
I'm evaluating potentially adding automated AI code reviews that run when a PR is opened prior to reviewer assignment. I ran one for this PR and made some separate comments on things I thought should be addressed. Wondering if you would take a look at the automated review comments below to discuss the value. P1 are high priority - should be fixed and generally block merging. P2 are medium priority - should fix soon but limited scope.
- [P1] Preserve distinct roots for collection joins — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/SearchQueryBuilder.java:26-27
When a search joins a collection, such as studies by season or trials by contact/external reference, removing distinct makes both pagination and getTotalCount() operate on joined rows. An entity with multiple
matching children is counted multiple times and can consume multiple page slots, producing incorrect metadata and omitted or duplicate results across every service using this builder.
- [P1] Apply filterBy to study searches — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/core/StudyService.java:162-162
For a POST /search/studies request containing filterBy, the request setter validates and stores the filters, but this query chain only applies sorting. The endpoint therefore silently returns studies that do
not match the requested filters; append .filterBy(request.getFilterBy()) as done by TrialService.
- [P1] Default the optional GET sort order — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/core/StudyService.java:110-113
The GET API declares sortOrder optional, and the previous implementation defaulted it to ascending. A request such as /studies?sortBy=studyName now calls SortOrder.valueOf(null) and returns a 500; lowercase
values also regress because valueOf is case-sensitive. The same pattern exists in TrialService.
- [P1] Avoid LIKE filters on non-string fields — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/SearchQueryBuilder.java:143-144
TrialSearchRequest permits filtering on UUID, date, and Boolean fields such as trialDbId, programDbId, startDate, and active, but every filter is rendered as lower(field) LIKE .... Hibernate/PostgreSQL cannot
apply lower to those types, so otherwise valid filter requests fail during query creation instead of returning results.
- [P2] Give each filter predicate a unique parameter — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/SearchQueryBuilder.java:140-145
When a request combines a legacy criterion with filterBy on the same field, such as trialNames plus a trialName filter, both predicates use the trialName parameter and appendLike overwrites the list value
with a string while the earlier HQL still uses IN. Multiple filters on the same field similarly all bind the last value, causing query failures or incorrect results.
- [P1] Exclude the internal sort getter from persisted JSON — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/io/swagger/model/SearchRequest.java:178-180
For searches large enough to be persisted by SearchService, this public getter serializes an additional sortByElements property alongside the annotated sortBy property. When the search-results endpoint
reloads a request with sorting, Jackson treats sortByElements as setterless, calls this getter on the new object, receives null, and throws InvalidDefinitionException, so the accepted search can never be
retrieved.
- [P1] Keep request field names stable when persisting — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/io/swagger/model/SearchRequest.java:170-173
When an asynchronous search uses a mapped field such as trialDbId, this setter changes the request value to the internal name id; SearchRequestEntity then persists that mutated JSON. On reload, the setter
validates id against API-facing keys such as trialDbId and rejects the stored request, so the search-results endpoint returns a deserialization error. Sorting performs the same destructive remapping.
- [P2] Reject null sort orders before building HQL — /Users/palladinon/IdeaProjects/brapi-Java-ProdServer/src/main/java/org/brapi/test/BrAPITestServer/service/SearchQueryBuilder.java:358-359
If a POSTed sort element explicitly contains a null or unknown sortOrder, SortOrder.fromValue leaves it null and this method emits ORDER BY ... null. That produces an HQL parsing failure and a server error
rather than the intended bad-request response or default ascending order.
| Map.entry("studyDbId", "id"), | ||
| Map.entry("studyLocation", "location.id"), | ||
| Map.entry("trialDbId", "trial.id"), | ||
| Map.entry("studyType", "studyName"), |
There was a problem hiding this comment.
Wonder if that was an oversight because the studyType filter uses studyType, not studyName.
| "createdBy", "createdBy", | ||
| "trialDbId", "id", | ||
| "programDbId","program.id", | ||
| "startDate", "startDate", |
There was a problem hiding this comment.
Looks like this is missing programName and locationDbId from spec
There was a problem hiding this comment.
I'll add it, but not super worried about maintaining spec wrt sorting. I think it's hardly used in general anyways.
| .appendList(request.getStudyPUIs(), "studyPUI").appendList(request.getStudyTypes(), "studyType") | ||
| .appendList(request.getTrialDbIds(), "trial.id").appendList(request.getTrialNames(), "trial.trialName") | ||
| .withSort(getSortByField(request.getSortBy()), request.getSortOrder()); | ||
| .sortBy(request.getSortByElements()); |
There was a problem hiding this comment.
This is missing a .filterBy that is present in the TrialService
There was a problem hiding this comment.
We aren't advertising to anyone that filterBy is available to anyone but us, and I'm not sure it's needed yet for DeltaBreed to filter in this particular service, so I think we should leave it alone for now.
Description
Story: BI-2861
To support the removal of the cache for TrialEntities, a generalized implementation of filtering and sorting was created for the prod server.
Many changes were made to support this, including:
SearchRequest,sortFilterEntityColumnNamesByRequestName. Entities that wish to be sortable must implement the get method for this map.Dependencies
Testing
Experimentstab of a Program. Mess with filtering, testing case insensitivity, and different data types. For sorting, test the order of different columns and ASC vs DESC order. Mess with paging, try selecting different pages, and different number of records per pageChecklist: