Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Germplasm Search Optimizations #49

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/java/io/swagger/model/SearchRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import io.swagger.model.core.SortBy;
import io.swagger.model.core.SortOrder;

import java.util.ArrayList;
import java.util.List;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import io.swagger.model.core.BatchDeletesListResponseResult;
import org.brapi.test.BrAPITestServer.auth.AuthDetails;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.exceptions.InvalidPagingException;
import org.brapi.test.BrAPITestServer.service.PagingUtility;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
Expand All @@ -33,29 +33,14 @@

public class BrAPIController {
private static final Logger log = LoggerFactory.getLogger(ServerInfoApiController.class);

protected Metadata generateMetaDataTemplateForSearch(Integer originalRequestedPage, Integer newRequestedPage,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was unused.

Integer originalRequestedPageSize, Integer newRequestedPageSize) throws BrAPIServerException {
Integer page = newRequestedPage;
Integer pageSize = newRequestedPageSize;

if (page == null) {
page = originalRequestedPage;
}
if (pageSize == null) {
pageSize = originalRequestedPageSize;
}

return generateMetaDataTemplate(page, pageSize);
}

protected Metadata generateMetaDataTemplate(SearchRequest request) throws BrAPIServerException {
return generateMetaDataTemplate(request.getPage(), request.getPageSize());
}

protected Metadata generateMetaDataTemplate(String pageToken, Integer pageSize) {
if (pageSize == null) {
pageSize = 1000;
pageSize = PagingUtility.getDefaultPageSize();
}

Metadata metaData = generateEmptyMetadataToken();
Expand All @@ -65,14 +50,14 @@ protected Metadata generateMetaDataTemplate(String pageToken, Integer pageSize)
}

protected Metadata generateMetaDataTemplate(Integer page, Integer pageSize) throws BrAPIServerException {
validatePaging(page, pageSize);
PagingUtility.validatePaging(page, pageSize);

// defaults
if (page == null) {
page = 0;
}
if (pageSize == null) {
pageSize = 1000;
pageSize = PagingUtility.getDefaultPageSize();
}

Metadata metaData = generateEmptyMetadata();
Expand All @@ -81,16 +66,6 @@ protected Metadata generateMetaDataTemplate(Integer page, Integer pageSize) thro
return metaData;
}

private void validatePaging(Integer page, Integer pageSize) throws BrAPIServerException {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to PagingUtility

boolean pageValid = (page == null) || (page >= 0);
if (!pageValid)
throw new InvalidPagingException("page");
boolean pageSizeValid = (pageSize == null) || (pageSize >= 1);
if (!pageSizeValid)
throw new InvalidPagingException("pageSize");

}

protected Metadata generateEmptyMetadata() {
Metadata metaData = new Metadata();
metaData.setDatafiles(new ArrayList<>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ public ResponseEntity<CrossesListResponse> crossesGet(
validateSecurityContext(request, "ROLE_ANONYMOUS", "ROLE_USER");
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(page, pageSize);
List<Cross> data = crossService.findCrosses(crossingProjectDbId, crossingProjectName, crossDbId, crossName,
commonCropName, programDbId, externalReferenceId, externalReferenceID, externalReferenceSource,
List<Cross> data = crossService.findCrosses(crossingProjectDbId, crossDbId,
externalReferenceID, externalReferenceSource,
metadata);
return responseOK(new CrossesListResponse(), new CrossesListResponseResult(), data, metadata);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public ResponseEntity<CrossingProjectsListResponse> crossingProjectsGet(
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(page, pageSize);
List<CrossingProject> data = crossingProjectService.findCrossingProjects(crossingProjectDbId,
crossingProjectName, includePotentialParents, commonCropName, programDbId, externalReferenceId,
externalReferenceID, externalReferenceSource, metadata);
crossingProjectName, includePotentialParents, commonCropName, programDbId,
externalReferenceID, externalReferenceSource, metadata);
return responseOK(new CrossingProjectsListResponse(), new CrossingProjectsListResponseResult(), data, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,37 @@ public ResponseEntity<? extends BrAPIResponse> searchGermplasmPost(@RequestBody
log.debug("Request: " + request.getRequestURI());
validateSecurityContext(request, "ROLE_ANONYMOUS", "ROLE_USER");
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(body);

String searchReqDbId = searchService.saveSearchRequest(body, SearchRequestTypes.GERMPLASM);
if (searchReqDbId != null) {
return responseAccepted(searchReqDbId);
}

// WARN: This code was introduced to deal with a specific use case from BI which requires all data associated with
// a particular program to be retreived at once. This method of data retreival is highly unadvised and can come
// with serious performance deficits, such as slow response times and exhausted memory allocation.
// Benchmarking suggests that at around 245-275k germplasm records returned 8GB of allocated memory will fail to
// be enough to return a result.

// This code is a stop-gap to allow BI to continue to do this improper retreival in a way that will be efficient
// for their specific use case.

// To get the data in this ill-advised way, forgo sending a page or pageSize attribute in the germplasm search request
// to this endpoint. This will trigger the findGermplasmWithoutPaging code, which will grab all of the data without regard
// to data size.

// To use the endpoint the right way, ensure one or both of the aforementioned attributes are set and the germplasm
// records will be retrieved and returned paginated to limit resource consumption. This way is much more fine tuned
// and will result in fast retrieval times with minimal memory allocation.
if (body.getPage() == null && body.getPageSize() == null) {
log.debug("Retrieving germs without pagination");
List<Germplasm> data = germplasmService.findGermplasmWithoutPaging(body);
Metadata metadata = generateEmptyMetadata();
metadata.getPagination().setTotalCount(data.size());
return responseOK(new GermplasmListResponse(), new GermplasmListResponseResult(), data, metadata);
} else {
log.debug("Retrieving germs with pagination");
Metadata metadata = generateMetaDataTemplate(body);
List<Germplasm> data = germplasmService.findGermplasm(body, metadata);
return responseOK(new GermplasmListResponse(), new GermplasmListResponseResult(), data, metadata);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public ResponseEntity<PlannedCrossesListResponse> plannedCrossesGet(
validateSecurityContext(request, "ROLE_ANONYMOUS", "ROLE_USER");
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(page, pageSize);
List<PlannedCross> data = crossService.findPlannedCrosses(crossingProjectDbId, crossingProjectName,
plannedCrossDbId, plannedCrossName, status, commonCropName, programDbId, externalReferenceId,
List<PlannedCross> data = crossService.findPlannedCrosses(crossingProjectDbId,
plannedCrossDbId,
externalReferenceID, externalReferenceSource, metadata);
return responseOK(new PlannedCrossesListResponse(), new PlannedCrossesListResponseResult(), data, metadata);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public ResponseEntity<SeedLotListResponse> seedlotsGet(
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(page, pageSize);
List<SeedLot> data = seedLotService.findSeedLots(seedLotDbId, germplasmDbId, germplasmName, crossDbId,
crossName, commonCropName, programDbId, externalReferenceId, externalReferenceID,
crossName, commonCropName, programDbId, externalReferenceID,
externalReferenceSource, metadata);
return responseOK(new SeedLotListResponse(), new SeedLotListResponseResult(), data, metadata);
}
Expand Down Expand Up @@ -156,7 +156,7 @@ public ResponseEntity<SeedLotTransactionListResponse> seedlotsTransactionsGet(
validateAcceptHeader(request);
Metadata metadata = generateMetaDataTemplate(page, pageSize);
List<SeedLotTransaction> data = seedLotService.findSeedLotTransactions(transactionDbId, seedLotDbId,
germplasmDbId, germplasmName, crossDbId, crossName, commonCropName, programDbId, externalReferenceId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

externalReferenceId is the newer, correct spelling, introduced in BrAPI v2.1.
externalReferenceID is the deprecated one and could be deleted if there is no need for backwards compatibility with v2.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were autofixes for params that were being unused in these sigs, I can rename to externalReferenceId here and elsewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will use the right one here and other places, but think we should wait to fully delete from ApiControllers and data models, or do it in another commit/MR.

Copy link

Choose a reason for hiding this comment

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

I would either continue to support the deprecated externalReferenceID parameters (see my comment on CrossingProjectsApiController.java about how to do that), or fully remove them from controller signatures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I would rather this not be a sticking point then. These changes are pretty far removed and out of scope of the things we actually want. I will just revert these changes.

If we actually want to remove those attributes from the codebase would rather that be done in a separate MR, bc it's a decent amount of changes to fully support.

germplasmDbId, germplasmName, crossDbId, crossName, commonCropName, programDbId,
externalReferenceID, externalReferenceSource, metadata);
return responseOK(new SeedLotTransactionListResponse(), new SeedLotTransactionListResponseResult(), data,
metadata);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ public class JsonbConverter implements AttributeConverter<Object, String> {
@Override
public String convertToDatabaseColumn(Object jsonb) {
try {
return mapper.writeValueAsString(jsonb);
if (jsonb != null) {
return mapper.writeValueAsString(jsonb);
}
return null;
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,18 @@

public class InvalidPagingException extends BrAPIServerException {
private static final long serialVersionUID = 6250184179200451757L;

private static final String pageNumExceedsTotalPages =
"A page was requested which exceeds total amount of data available. Please make a RQ with page < totalPages - 1. " +
"Page numbers start at 0, and you can find out totalPages by making a RQ with \"page\": 0";

public InvalidPagingException(String field) {
super(HttpStatus.BAD_REQUEST, "\'" + field + "\' value is invalid");
}

// This constructor should only be used when the pageNum of the RQ exceeds the total number of pages available.
public InvalidPagingException() {
super(HttpStatus.BAD_REQUEST, pageNumExceedsTotalPages);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
import io.swagger.model.SearchRequest;
import io.swagger.model.core.BatchDeleteTypes;
import jakarta.validation.Valid;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;

import java.util.List;

public interface BrAPIComponent<T, R extends SearchRequest> {
List<T> findEntities(@Valid R request, Metadata metadata);
List<T> findEntities(@Valid R request, Metadata metadata) throws BrAPIServerException;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All these BrAPIAServerExceptions were added to catch and populate the InvalidPagingException up the stack to deliver a 400 to the user and give them a message instrucitng them how to page correctly.

BatchDeleteTypes getBatchDeleteType();
List<String> collectDbIds(List<T> entities);
void deleteBatchDeleteData(List<String> dbIds);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io.swagger.model.core.ListSearchRequest;
import io.swagger.model.core.ListSummary;
import jakarta.validation.Valid;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.factory.BrAPIComponent;
import org.brapi.test.BrAPITestServer.service.core.ListService;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -23,7 +24,8 @@ public ListComponent(ListService listService) {
}

@Override
public List<ListSummary> findEntities(@Valid ListSearchRequest request, Metadata metadata) {
public List<ListSummary> findEntities(@Valid ListSearchRequest request, Metadata metadata)
throws BrAPIServerException {
return listService.findLists(request, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io.swagger.model.core.BatchDeleteTypes;
import io.swagger.model.core.Trial;
import io.swagger.model.core.TrialSearchRequest;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.factory.BrAPIComponent;
import org.brapi.test.BrAPITestServer.service.core.TrialService;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -22,7 +23,8 @@ public TrialComponent(TrialService trialService) {
}

@Override
public List<Trial> findEntities(TrialSearchRequest request, Metadata metadata) {
public List<Trial> findEntities(TrialSearchRequest request, Metadata metadata)
throws BrAPIServerException {
return trialService.findTrials(request, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io.swagger.model.geno.Plate;
import io.swagger.model.geno.PlateSearchRequest;
import org.apache.commons.lang3.NotImplementedException;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.factory.BrAPIComponent;
import org.brapi.test.BrAPITestServer.service.geno.PlateService;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -23,7 +24,8 @@ public PlateComponent(PlateService plateService) {
}

@Override
public List<Plate> findEntities(PlateSearchRequest request, Metadata metadata) {
public List<Plate> findEntities(PlateSearchRequest request, Metadata metadata)
throws BrAPIServerException {
return plateService.findPlates(request, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io.swagger.model.core.BatchDeleteTypes;
import io.swagger.model.geno.Sample;
import io.swagger.model.geno.SampleSearchRequest;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.factory.BrAPIComponent;
import org.brapi.test.BrAPITestServer.service.geno.SampleService;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -22,7 +23,8 @@ public SampleComponent(SampleService sampleService) {
}

@Override
public List<Sample> findEntities(SampleSearchRequest request, Metadata metadata) {
public List<Sample> findEntities(SampleSearchRequest request, Metadata metadata)
throws BrAPIServerException {
return sampleService.findSamples(request, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io.swagger.model.germ.Germplasm;
import io.swagger.model.germ.GermplasmSearchRequest;
import jakarta.validation.Valid;
import org.brapi.test.BrAPITestServer.exceptions.BrAPIServerException;
import org.brapi.test.BrAPITestServer.factory.BrAPIComponent;
import org.brapi.test.BrAPITestServer.service.germ.GermplasmService;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -23,7 +24,8 @@ public GermplasmComponent(GermplasmService germplasmService) {
}

@Override
public List<Germplasm> findEntities(@Valid GermplasmSearchRequest request, Metadata metadata) {
public List<Germplasm> findEntities(@Valid GermplasmSearchRequest request, Metadata metadata)
throws BrAPIServerException {
return germplasmService.findGermplasm(request, metadata);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.List;
import java.util.Optional;

import org.brapi.test.BrAPITestServer.exceptions.InvalidPagingException;
import org.brapi.test.BrAPITestServer.model.entity.BrAPIPrimaryEntity;
import org.brapi.test.BrAPITestServer.service.SearchQueryBuilder;
import org.springframework.data.domain.Page;
Expand All @@ -14,7 +15,11 @@
@NoRepositoryBean
public interface BrAPIRepository<T extends BrAPIPrimaryEntity, ID extends Serializable> extends JpaRepository<T, ID> {

public Page<T> findAllBySearch(SearchQueryBuilder<T> searchQuery, Pageable pageReq);
public Page<T> findAllBySearchAndPaginate(SearchQueryBuilder<T> searchQuery, Pageable pageReq) throws InvalidPagingException;

public Page<T> findAllBySearchPaginatingWithFetches(SearchQueryBuilder<T> searchQuery, Pageable pageReq) throws InvalidPagingException;

public List<T> findAllBySearch(SearchQueryBuilder<T> searchQuery);

public Optional<T> findById(ID id);

Expand All @@ -24,5 +29,5 @@ public interface BrAPIRepository<T extends BrAPIPrimaryEntity, ID extends Serial

public <S extends T> void refresh(S entity);

public void fetchXrefs(Page<T> page, Class<T> searchClass);
public void fetchXrefs(Page<T> page, Class<T> searchClass) throws InvalidPagingException;
}
Loading