Skip to content

Implement Generalized Filtering and Sorting for the Prod Server#17

Open
jloux-brapi wants to merge 12 commits into
epic/BI-2862from
feature/BI-2861
Open

Implement Generalized Filtering and Sorting for the Prod Server#17
jloux-brapi wants to merge 12 commits into
epic/BI-2862from
feature/BI-2861

Conversation

@jloux-brapi

@jloux-brapi jloux-brapi commented May 29, 2026

Copy link
Copy Markdown

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:

  • New filter/sort models available to all search requests
  • Removal of old sort implementation on Study and Trial endpoints. These now utilize the new models
  • Implementation example for Trials and Studies. The available filter columns are now configurable via a base Map in parent SearchRequest, sortFilterEntityColumnNamesByRequestName . Entities that wish to be sortable must implement the get method for this map.
  • Support for invalid sort/filter requests that do not contain valid sortable/filterable column names
  • Generalized custom sql code for filtering and sorting. This code should be safe from SQL injection as it uses hibernate parameterization for queries generated from user input.
  • @formula entity columns made to capture important sorting/filtering columns from additionalInfo columns in Trials. These seem to work very well with the filtering and sorting implementation, and should be replicated for other entities.

Dependencies

Testing

  • Test filtering and sorting of Trials by messing around with the different functionalities of the data table in the Experiments tab 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 page

Checklist:

Map.entry("studyDbId", "id"),
Map.entry("studyLocation", "location.id"),
Map.entry("trialDbId", "trial.id"),
Map.entry("studyType", "studyName"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are both studyType and studyName intended to map to studyName?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that seemed to be the original intent in the previous implementation of the prod/test server for single column filtering on studies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if that was an oversight because the studyType filter uses studyType, not studyName.

@nickpalladino nickpalladino left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if that was an oversight because the studyType filter uses studyType, not studyName.

"createdBy", "createdBy",
"trialDbId", "id",
"programDbId","program.id",
"startDate", "startDate",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is missing programName and locationDbId from spec

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a .filterBy that is present in the TrialService

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@humsika humsika left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants