From 2f9ca55664929d8e9a54b4f29c63884f8e21956a Mon Sep 17 00:00:00 2001 From: humsika Date: Mon, 12 Oct 2026 16:08:14 -0500 Subject: [PATCH 1/2] BI-2964: Applied observation filtering and pagination on BrAPI side and added Unit tests. --- .../brapi/v2/BrAPIObservationsController.java | 73 +++++++------------ .../brapi/v2/dao/BrAPIObservationDAO.java | 41 +++++++---- ...ObservationsControllerIntegrationTest.java | 55 +++++++++++++- .../v2/dao/BrAPIObservationDAOUnitTest.java | 73 ++++++++++++++++++- 4 files changed, 179 insertions(+), 63 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/BrAPIObservationsController.java b/src/main/java/org/breedinginsight/brapi/v2/BrAPIObservationsController.java index 7c1a18897..094880e4d 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/BrAPIObservationsController.java +++ b/src/main/java/org/breedinginsight/brapi/v2/BrAPIObservationsController.java @@ -17,7 +17,6 @@ package org.breedinginsight.brapi.v2; - import io.micronaut.http.HttpResponse; import io.micronaut.http.HttpStatus; import io.micronaut.http.annotation.*; @@ -28,13 +27,12 @@ import org.brapi.client.v2.ApiResponse; import org.brapi.client.v2.model.exceptions.ApiException; import org.brapi.client.v2.modules.phenotype.ObservationsApi; -import org.brapi.v2.model.BrAPIIndexPagination; import org.brapi.v2.model.BrAPIMetadata; import org.brapi.v2.model.BrAPIStatus; import org.brapi.v2.model.BrAPIWSMIMEDataTypes; -import org.brapi.v2.model.core.BrAPIStudy; import org.brapi.v2.model.pheno.BrAPIObservation; -import org.brapi.v2.model.pheno.response.*; +import org.brapi.v2.model.pheno.response.BrAPIObservationListResponse; +import org.brapi.v2.model.pheno.response.BrAPIObservationTableResponse; import org.breedinginsight.api.auth.ProgramSecured; import org.breedinginsight.api.auth.ProgramSecuredRoleGroup; import org.breedinginsight.brapi.v1.controller.BrapiVersion; @@ -46,7 +44,7 @@ import org.breedinginsight.services.ProgramService; import org.breedinginsight.services.brapi.BrAPIEndpointProvider; import org.breedinginsight.utilities.Utilities; - +import org.breedinginsight.utilities.response.ResponseUtils; import javax.annotation.Nullable; import javax.inject.Inject; @@ -144,54 +142,37 @@ public HttpResponse observationsGet(@PathVariable("programId") UUID programId, ); } - // Get a filtered list of observations. - // TODO: Handle filtering and pagination of this request entirely on BrAPI side once study and ou cache is removed: [BI-2964] - List observations = observationDAO.getObservationsByFilters(program.get(), studyDbId); - - // If page is not provided, set it to the default value 0. - if (page == null) page = 0; - // If pageSize is not provided, set it to the default value 1000. - if (pageSize == null) pageSize = 1000; + boolean pageSizeValid = pageSize == null || pageSize > 0; + boolean pageValid = page == null || page >= 0; - // Total number of records in the unpaged super set. - int totalCount = observations.size(); - // The least of pageSize and totalCount, unless pageSize is zero, in which case use totalCount. - int requestedPageSize = pageSize > 0 ? Math.min(pageSize, totalCount) : totalCount; - // Integer division and round up. - int totalPages = totalCount / requestedPageSize + ((totalCount % requestedPageSize == 0) ? 0 : 1); - log.info("(Pagination) totalCount: " + totalCount + " page (0-indexed): " + page + " requestedPageSize: " + requestedPageSize + " totalPages: " + totalPages); - - // Determine validity of pagination query parameters. - boolean pageSizeValid = pageSize > 0; - boolean pageValid = page >= 0 && page < totalPages; - - // Only paginate if valid pagination values were sent. - if (pageSizeValid && pageValid) { - int start = page * requestedPageSize; - // Account for last page, which may have fewer than requestedPageSize items, or exactly requestedPageSize items. - int end = (page == (totalPages - 1) && totalCount % requestedPageSize != 0) ? (start + (totalCount % requestedPageSize)) : Math.min(((page + 1) * requestedPageSize), totalCount); - log.info("(Pagination) start " + start + " end " + end); - // Sort observations so that paging is consistent and coherent. - observations.sort(Comparator.comparing(BrAPIObservation::getObservationDbId)); - // Paginate response. - observations = observations.subList(start, end); - } else { + if (!pageSizeValid || !pageValid) { String errorMessage = "Invalid query parameters: page, pageSize"; - return HttpResponse.badRequest(new BrAPIObservationListResponse().metadata(new BrAPIMetadata().status(List.of(new BrAPIStatus().messageType(BrAPIStatus.MessageTypeEnum.ERROR) - .message(errorMessage))))); + return HttpResponse.badRequest(new BrAPIObservationListResponse() + .metadata(new BrAPIMetadata() + .status(List.of(new BrAPIStatus() + .messageType(BrAPIStatus.MessageTypeEnum.ERROR) + .message(errorMessage))))); } - return HttpResponse.ok(new BrAPIObservationListResponse().metadata(new BrAPIMetadata().pagination(new BrAPIIndexPagination() - .currentPage(page) - .totalPages(totalPages) - .pageSize(observations.size()) - .totalCount(totalCount))) - .result(new BrAPIObservationListResponseResult().data(observations))); + BrAPIObservationListResponse brAPIResponse = observationDAO.getObservationsByFilters(program.get(), studyDbId, page, pageSize); + + List observations = brAPIResponse.getResult().getData(); + return ResponseUtils.getBrapiQueryResponse(observations, brAPIResponse); } catch (ApiException e) { log.error(Utilities.generateApiExceptionLogMessage(e), e); - return HttpResponse.serverError(new BrAPIObservationListResponse().metadata(new BrAPIMetadata().status(List.of(new BrAPIStatus().messageType(BrAPIStatus.MessageTypeEnum.ERROR) - .message("Error fetching observations"))))); + + BrAPIObservationListResponse errorResponse = new BrAPIObservationListResponse() + .metadata(new BrAPIMetadata() + .status(List.of(new BrAPIStatus() + .messageType(BrAPIStatus.MessageTypeEnum.ERROR) + .message("Error fetching observations")))); + + if (e.getCode() == HttpStatus.BAD_REQUEST.getCode()) { + return HttpResponse.badRequest(errorResponse); + } + + return HttpResponse.serverError(errorResponse); } catch (Exception e) { log.error("Error fetching Observations", e); return HttpResponse.serverError(new BrAPIObservationListResponse().metadata(new BrAPIMetadata().status(List.of(new BrAPIStatus().messageType(BrAPIStatus.MessageTypeEnum.ERROR) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java index 5766cae0b..ccd95948e 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java @@ -203,21 +203,32 @@ public List getObservationsByObservationUnits(Collection getObservationsByFilters(Program program, String studyDbId) throws ApiException, DoesNotExistException { - Collection observations = getProgramObservations(program.getId()); - - HashMap traitIdsByObservationVariableDbId = - traitService.getIdsByObservationVariableDbIds( - program.getId(), - observations.stream() - .map(BrAPIObservation::getObservationVariableDbId) - .collect(Collectors.toList())); - - return observations.stream() - .filter(o -> studyDbId == null || studyDbId.equals(o.getStudyDbId())) - .peek(o -> - o.setObservationVariableDbId(traitIdsByObservationVariableDbId.get(o.getObservationVariableDbId()))) - .collect(Collectors.toList()); + public BrAPIObservationListResponse getObservationsByFilters(Program program, String studyDbId, Integer page, Integer pageSize) throws ApiException, DoesNotExistException { + BrAPIObservationSearchRequest observationSearchRequest = new BrAPIObservationSearchRequest() + .programDbIds(List.of(program.getBrapiProgram().getProgramDbId())); + + if (studyDbId != null) { + observationSearchRequest.setStudyDbIds(List.of(studyDbId)); + } + observationSearchRequest.setPage(page == null ? 0 : page); + observationSearchRequest.setPageSize(pageSize == null ? 1000 : pageSize); + + ObservationsApi api = brAPIEndpointProvider.get(programDAO.getCoreClient(program.getId()), ObservationsApi.class); + + BrAPIObservationListResponse brAPIResponse = brAPIDAOUtil.simpleSearch(api::searchObservationsPost, observationSearchRequest); + + List observations = brAPIDAOUtil.getListResult(brAPIResponse); + processObservations(program.getKey(), observations); + + HashMap traitIdsByObservationVariableDbId = traitService.getIdsByObservationVariableDbIds( + program.getId(), observations.stream() + .map(BrAPIObservation::getObservationVariableDbId).collect(Collectors.toList())); + + observations.forEach(observation -> observation + .setObservationVariableDbId(traitIdsByObservationVariableDbId.get(observation.getObservationVariableDbId()))); + brAPIResponse.getResult().setData(observations); + + return brAPIResponse; } @NotNull diff --git a/src/test/java/org/breedinginsight/brapi/v2/BrAPIObservationsControllerIntegrationTest.java b/src/test/java/org/breedinginsight/brapi/v2/BrAPIObservationsControllerIntegrationTest.java index aae656761..8ba8580b0 100644 --- a/src/test/java/org/breedinginsight/brapi/v2/BrAPIObservationsControllerIntegrationTest.java +++ b/src/test/java/org/breedinginsight/brapi/v2/BrAPIObservationsControllerIntegrationTest.java @@ -368,6 +368,58 @@ public void testGetObsPagination() { checkPagination(1, 0, HttpStatus.BAD_REQUEST, null, null, null); checkPagination(10, 100, HttpStatus.BAD_REQUEST, null, null, null); + // Invalid negative values. + checkPagination(-1, 2, HttpStatus.BAD_REQUEST, null, null, null); + checkPagination(0, -1, HttpStatus.BAD_REQUEST, null, null, null); + + // Invalid boundary when the final page is partial. + checkPagination(2, 3, HttpStatus.BAD_REQUEST, null, null, null); + } + + @Test + public void testGetObsUnknownStudyReturnsEmptyFirstPage() { + Flowable> call = client.exchange( + GET(String.format("/programs/%s/brapi/v2/observations" + "?studyDbId=%s&page=0&pageSize=2", program.getId(), UUID.randomUUID())) + .bearerAuth("test-registered-user"), String.class); + + HttpResponse response = call.blockingFirst(); + + assertEquals(HttpStatus.OK, response.getStatus()); + + JsonObject responseObj = gson.fromJson(response.body(), JsonObject.class); + JsonArray observations = responseObj.getAsJsonObject("result").getAsJsonArray("data"); + JsonObject pagination = responseObj.getAsJsonObject("metadata").getAsJsonObject("pagination"); + + assertEquals(0, observations.size()); + assertEquals(0, pagination.get("currentPage").getAsInt()); + assertEquals(2, pagination.get("pageSize").getAsInt()); + assertEquals(0, pagination.get("totalCount").getAsInt()); + assertEquals(0, pagination.get("totalPages").getAsInt()); + } + + @Test + public void testGetObsByStudyDbIdWithPagination() { + Flowable> call = client.exchange( + GET(String.format( + "/programs/%s/brapi/v2/observations?studyDbId=%s&page=0&pageSize=1", + program.getId(), + envIds.get(0))) + .bearerAuth("test-registered-user"), String.class); + + HttpResponse response = call.blockingFirst(); + assertEquals(HttpStatus.OK, response.getStatus()); + + JsonObject responseObj = gson.fromJson(response.body(), JsonObject.class); + JsonArray observations = responseObj.getAsJsonObject("result").getAsJsonArray("data"); + JsonObject pagination = responseObj.getAsJsonObject("metadata").getAsJsonObject("pagination"); + + assertEquals(1, observations.size()); + assertEquals(envIds.get(0), observations.get(0).getAsJsonObject().get("studyDbId").getAsString()); + + assertEquals(0, pagination.get("currentPage").getAsInt()); + assertEquals(1, pagination.get("pageSize").getAsInt()); + assertEquals(2, pagination.get("totalCount").getAsInt()); + assertEquals(2, pagination.get("totalPages").getAsInt()); } private void checkPagination(Integer page, Integer pageSize, HttpStatus expectedStatus, Integer expectedSize, Integer expectedTotalCount, Integer expectedTotalPages) { @@ -400,7 +452,8 @@ private void checkPagination(Integer page, Integer pageSize, HttpStatus expected JsonObject responseObj = gson.fromJson(response.body(), JsonObject.class); // Get metadata. JsonObject pagination = responseObj.getAsJsonObject("metadata").getAsJsonObject("pagination"); - assertEquals(expectedSize, pagination.get("pageSize").getAsInt()); + int expectedPageSize = pageSize == null ? 1000 : pageSize; + assertEquals(expectedPageSize, pagination.get("pageSize").getAsInt()); int expectedPage = page == null ? 0 : page; assertEquals(expectedPage, pagination.get("currentPage").getAsInt()); assertEquals(expectedTotalPages, pagination.get("totalPages").getAsInt()); diff --git a/src/test/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAOUnitTest.java b/src/test/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAOUnitTest.java index a82bb9389..566fce142 100644 --- a/src/test/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAOUnitTest.java +++ b/src/test/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAOUnitTest.java @@ -41,6 +41,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.*; +import org.brapi.v2.model.pheno.response.BrAPIObservationListResponse; +import org.brapi.v2.model.pheno.response.BrAPIObservationListResponseResult; +import java.util.HashMap; public class BrAPIObservationDAOUnitTest { @@ -50,6 +53,7 @@ public class BrAPIObservationDAOUnitTest { private BrAPIObservationUnitDAO observationUnitDAO; private BrAPIDAOUtil brAPIDAOUtil; private Program program; + private TraitService traitService; @BeforeEach void setup() { @@ -65,6 +69,7 @@ void setup() { ProgramDAO programDAO = mock(ProgramDAO.class); observationUnitDAO = mock(BrAPIObservationUnitDAO.class); brAPIDAOUtil = mock(BrAPIDAOUtil.class); + traitService = mock(TraitService.class); when(programDAO.getCoreClient(programId)).thenReturn(mock(BrAPIClient.class)); @@ -75,7 +80,7 @@ void setup() { brAPIDAOUtil, new BrAPIEndpointProvider(), 1000, - mock(TraitService.class) + traitService ); } @@ -200,4 +205,70 @@ void observationSearchIsSkippedForEmptyFilters() { verifyNoInteractions(observationUnitDAO); verify(brAPIDAOUtil, never()).search(any(Function.class),any(Function4.class),any(BrAPIObservationSearchRequest.class)); } + + @Test + @SneakyThrows + void getObservationsByFiltersUsesDefaultsWithoutStudyFilter() { + List observations = List.of(); + + BrAPIObservationListResponse brAPIResponse = new BrAPIObservationListResponse().result(new BrAPIObservationListResponseResult().data(observations)); + + when(brAPIDAOUtil.simpleSearch(any(Function.class), any(BrAPIObservationSearchRequest.class))).thenReturn(brAPIResponse); + + doReturn(observations).when(brAPIDAOUtil).getListResult(brAPIResponse); + + when(traitService.getIdsByObservationVariableDbIds(program.getId(), List.of())).thenReturn(new HashMap<>()); + + observationDAO.getObservationsByFilters(program, null, null, null); + + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(BrAPIObservationSearchRequest.class); + + verify(brAPIDAOUtil).simpleSearch(any(Function.class), requestCaptor.capture()); + + BrAPIObservationSearchRequest request = requestCaptor.getValue(); + + assertEquals(List.of(BRAPI_PROGRAM_DB_ID), request.getProgramDbIds()); + assertEquals(null, request.getStudyDbIds()); + assertEquals(0, request.getPage()); + assertEquals(1000, request.getPageSize()); + } + + @Test + @SneakyThrows + void getObservationsByFiltersUsesBrAPISearchPagination() { + String brapiVariableDbId = "brapi-variable-1"; + String traitId = UUID.randomUUID().toString(); + + BrAPIObservation observation = new BrAPIObservation() + .observationDbId("observation-1") + .studyDbId("study-1") + .observationVariableDbId(brapiVariableDbId); + + List brapiObservations = List.of(observation); + BrAPIObservationListResponse brAPIResponse = new BrAPIObservationListResponse().result(new BrAPIObservationListResponseResult().data(brapiObservations)); + + when(brAPIDAOUtil.simpleSearch(any(Function.class), any(BrAPIObservationSearchRequest.class))).thenReturn(brAPIResponse); + + doReturn(brapiObservations).when(brAPIDAOUtil).getListResult(brAPIResponse); + + HashMap traitIdsByObservationVariableDbId = new HashMap<>(); + traitIdsByObservationVariableDbId.put(brapiVariableDbId, traitId); + + when(traitService.getIdsByObservationVariableDbIds(program.getId(), List.of(brapiVariableDbId))) + .thenReturn(traitIdsByObservationVariableDbId); + + BrAPIObservationListResponse result = observationDAO.getObservationsByFilters(program, "study-1", 2, 25); + + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(BrAPIObservationSearchRequest.class); + + verify(brAPIDAOUtil).simpleSearch(any(Function.class), requestCaptor.capture()); + + BrAPIObservationSearchRequest request = requestCaptor.getValue(); + + assertEquals(List.of(BRAPI_PROGRAM_DB_ID), request.getProgramDbIds()); + assertEquals(List.of("study-1"), request.getStudyDbIds()); + assertEquals(2, request.getPage()); + assertEquals(25, request.getPageSize()); + assertEquals(traitId, result.getResult().getData().get(0).getObservationVariableDbId()); + } } \ No newline at end of file From 9108636b362df23495f25e8756d5ca0aa8ff8b6a Mon Sep 17 00:00:00 2001 From: jloux-brapi Date: Thu, 15 Oct 2026 10:30:59 -0400 Subject: [PATCH 2/2] Update src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java --- .../org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java index ccd95948e..0ca219012 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationDAO.java @@ -220,6 +220,7 @@ public BrAPIObservationListResponse getObservationsByFilters(Program program, St List observations = brAPIDAOUtil.getListResult(brAPIResponse); processObservations(program.getKey(), observations); + // TODO: Investigate if this resetting of the observationVariableDbId can be undone when trait exrefs are examined [BI-2925] HashMap traitIdsByObservationVariableDbId = traitService.getIdsByObservationVariableDbIds( program.getId(), observations.stream() .map(BrAPIObservation::getObservationVariableDbId).collect(Collectors.toList()));