From fe3b144f3534d0fe76724f8ce502a41a4d8156e3 Mon Sep 17 00:00:00 2001 From: Mukhiddin Yusupov <133661057+mukhiddin-yusuf@users.noreply.github.com> Date: Wed, 3 Jul 2024 11:29:09 +0500 Subject: [PATCH] feat(consortium-search): Increase limit for batch request IDs count for searching items/holdings in consortium * feat(consortium-search): Increase limit for batch request IDs count for searching items/holdings in consortium Closes: MSEARCH-785 --- NEWS.md | 1 + README.md | 1 + descriptors/ModuleDescriptor-template.json | 5 + .../SearchConfigurationProperties.java | 10 + .../SearchConsortiumController.java | 24 +- .../model/service/CqlSearchRequest.java | 2 +- .../folio/search/service/SearchService.java | 4 +- .../ConsortiumInstanceSearchService.java | 143 +++++++++--- src/main/resources/application.yml | 2 + .../swagger.api/parameters/batchIdsDto.yaml | 1 - .../BrowseClassificationConsortiumIT.java | 3 +- .../controller/BrowseClassificationIT.java | 3 +- .../controller/BrowseSubjectConsortiumIT.java | 3 +- .../search/controller/BrowseSubjectIT.java | 3 +- .../ConsortiumSearchHoldingsIT.java | 13 +- .../controller/ConsortiumSearchItemsIT.java | 14 +- .../ConsortiumInstanceSearchServiceTest.java | 212 +++++++++++++----- src/test/resources/application.yml | 1 + 18 files changed, 329 insertions(+), 116 deletions(-) diff --git a/NEWS.md b/NEWS.md index c37d2ffd9..e0d5c60c0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -26,6 +26,7 @@ * Implement Indexing of Institutions from Kafka ([MSEARCH-771](https://issues.folio.org/browse/MSEARCH-771)) * Implement Indexing of Libraries from Kafka ([MSEARCH-769](https://issues.folio.org/browse/MSEARCH-769)) * Return Unified List of Inventory Campuses in a Consortium ([MSEARCH-773](https://issues.folio.org/browse/MSEARCH-773)) +* Increase batch IDs limit for search consolidated items/holdings in consortium ([MSEARCH-785](https://folio-org.atlassian.net/browse/MSEARCH-785)) * Return Unified List of Inventory Libraries in a Consortium ([MSEARCH-772](https://issues.folio.org/browse/MSEARCH-772)) * Index, search Instance place of publication field ([MSEARCH-755](https://folio-org.atlassian.net/browse/MSEARCH-755)) diff --git a/README.md b/README.md index dcd24220e..1acce7523 100644 --- a/README.md +++ b/README.md @@ -278,6 +278,7 @@ and [Cross-cluster replication](https://docs.aws.amazon.com/opensearch-service/l | MAX_BROWSE_REQUEST_OFFSET | 500 | The maximum elasticsearch query offset for additional requests on browse around | | SYSTEM_USER_ENABLED | true | Defines if system user must be created at service tenant initialization or used for egress service requests | | REINDEX_LOCATION_BATCH_SIZE | 1_000 | Defines number of locations to retrieve per inventory http request on locations reindex process | +| MAX_SEARCH_BATCH_REQUEST_IDS_COUNT | 20_000 | Defines maximum batch request IDs count for searching consolidated items/holdings in consortium | The module uses system user to communicate with other modules from Kafka consumers. For production deployments you MUST specify the password for this system user via env variable: diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json index b40d01cbb..f58caa3fa 100644 --- a/descriptors/ModuleDescriptor-template.json +++ b/descriptors/ModuleDescriptor-template.json @@ -1080,6 +1080,11 @@ "name": "REINDEX_LOCATION_BATCH_SIZE", "value": "1000", "description": "Defines number of locations to retrieve per inventory http request on locations reindex process" + }, + { + "name": "MAX_SEARCH_BATCH_REQUEST_IDS_COUNT,", + "value": "20_000", + "description": "Defines maximum batch request IDs count for searching consolidated items/holdings in consortium" } ] } diff --git a/src/main/java/org/folio/search/configuration/properties/SearchConfigurationProperties.java b/src/main/java/org/folio/search/configuration/properties/SearchConfigurationProperties.java index 046127711..8abe7260f 100644 --- a/src/main/java/org/folio/search/configuration/properties/SearchConfigurationProperties.java +++ b/src/main/java/org/folio/search/configuration/properties/SearchConfigurationProperties.java @@ -41,6 +41,16 @@ public class SearchConfigurationProperties { */ private long maxBrowseRequestOffset = 500L; + /** + * Provides the maximum number of IDs for performing search with batch requests. + */ + private long maxSearchBatchRequestIdsCount = 20_000L; + + /** + * Provides the size parameter for querying consortium records (holdings or items). + */ + private int searchConsortiumRecordsPageSize = 5_000; + /** * Provides map with global features configuration. Can be overwritten by tenant configuration. */ diff --git a/src/main/java/org/folio/search/controller/SearchConsortiumController.java b/src/main/java/org/folio/search/controller/SearchConsortiumController.java index 9e9f7ab14..2869b0bac 100644 --- a/src/main/java/org/folio/search/controller/SearchConsortiumController.java +++ b/src/main/java/org/folio/search/controller/SearchConsortiumController.java @@ -3,9 +3,8 @@ import static org.folio.search.utils.SearchUtils.INSTANCE_HOLDING_FIELD_NAME; import static org.folio.search.utils.SearchUtils.INSTANCE_ITEM_FIELD_NAME; -import java.util.Set; +import java.util.HashSet; import java.util.UUID; -import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import lombok.extern.log4j.Log4j2; import org.folio.search.domain.dto.BatchIdsDto; @@ -156,10 +155,12 @@ public ResponseEntity getConsortiumItem(UUID id, String tenantHe public ResponseEntity fetchConsortiumBatchHoldings(String tenantHeader, BatchIdsDto batchIdsDto) { var tenant = verifyAndGetTenant(tenantHeader); - var holdingIds = batchIdsDto.getIds().stream().map(UUID::toString).collect(Collectors.toSet()); - var searchRequest = idsCqlRequest(tenant, INSTANCE_HOLDING_FIELD_NAME, holdingIds); + if (batchIdsDto.getIds().isEmpty()) { + return ResponseEntity + .ok(new ConsortiumHoldingCollection()); + } - var result = searchService.fetchConsortiumBatchHoldings(searchRequest, holdingIds); + var result = searchService.fetchConsortiumBatchHoldings(tenant, new HashSet<>(batchIdsDto.getIds())); return ResponseEntity.ok(result); } @@ -172,10 +173,8 @@ public ResponseEntity fetchConsortiumBatchItems(String } var tenant = verifyAndGetTenant(tenantHeader); - var itemIds = batchIdsDto.getIds().stream().map(UUID::toString).collect(Collectors.toSet()); - var searchRequest = idsCqlRequest(tenant, INSTANCE_ITEM_FIELD_NAME, itemIds); - var result = searchService.fetchConsortiumBatchItems(searchRequest, itemIds); + var result = searchService.fetchConsortiumBatchItems(tenant, new HashSet<>(batchIdsDto.getIds())); return ResponseEntity.ok(result); } @@ -191,13 +190,4 @@ private CqlSearchRequest idCqlRequest(String tenant, String fieldName, var query = fieldName + ".id=" + id; return CqlSearchRequest.of(Instance.class, tenant, query, 1, 0, true, false, true); } - - private CqlSearchRequest idsCqlRequest(String tenant, String fieldName, Set ids) { - var query = ids.stream() - .map((fieldName + ".id=%s")::formatted) - .collect(Collectors.joining(" or ")); - - return CqlSearchRequest.of(Instance.class, tenant, query, ids.size(), 0, true, false, true); - } - } diff --git a/src/main/java/org/folio/search/model/service/CqlSearchRequest.java b/src/main/java/org/folio/search/model/service/CqlSearchRequest.java index a5856e87d..e3a4dd5e0 100644 --- a/src/main/java/org/folio/search/model/service/CqlSearchRequest.java +++ b/src/main/java/org/folio/search/model/service/CqlSearchRequest.java @@ -61,7 +61,7 @@ public class CqlSearchRequest implements ResourceRequest { /** * Creates {@link CqlSearchRequest} object for given variables. * - * @param resourceClass - resource class + * @param resourceClass - resource class * @param tenantId - tenant id * @param query - CQL query * @param limit - search result records limit diff --git a/src/main/java/org/folio/search/service/SearchService.java b/src/main/java/org/folio/search/service/SearchService.java index 962c258b0..edf5981ef 100644 --- a/src/main/java/org/folio/search/service/SearchService.java +++ b/src/main/java/org/folio/search/service/SearchService.java @@ -30,6 +30,8 @@ @RequiredArgsConstructor public class SearchService { + public static final int DEFAULT_MAX_SEARCH_RESULT_WINDOW = 10_000; + private final SearchRepository searchRepository; private final SearchFieldProvider searchFieldProvider; private final CqlSearchQueryConverter cqlSearchQueryConverter; @@ -47,7 +49,7 @@ public class SearchService { public SearchResult search(CqlSearchRequest request) { log.debug("search:: by [query: {}, resource: {}]", request.getQuery(), request.getResource()); - if (request.getOffset() + request.getLimit() > 10_000L) { + if (request.getOffset() + request.getLimit() > DEFAULT_MAX_SEARCH_RESULT_WINDOW) { var validationException = new RequestValidationException("The sum of limit and offset should not exceed 10000.", "offset + limit", String.valueOf(request.getOffset() + request.getLimit())); log.warn(validationException.getMessage()); diff --git a/src/main/java/org/folio/search/service/consortium/ConsortiumInstanceSearchService.java b/src/main/java/org/folio/search/service/consortium/ConsortiumInstanceSearchService.java index c76ddf2b0..8cc4cbbc5 100644 --- a/src/main/java/org/folio/search/service/consortium/ConsortiumInstanceSearchService.java +++ b/src/main/java/org/folio/search/service/consortium/ConsortiumInstanceSearchService.java @@ -1,11 +1,24 @@ package org.folio.search.service.consortium; import static org.apache.commons.collections4.CollectionUtils.isEmpty; - +import static org.folio.search.service.SearchService.DEFAULT_MAX_SEARCH_RESULT_WINDOW; +import static org.folio.search.utils.SearchUtils.INSTANCE_HOLDING_FIELD_NAME; +import static org.folio.search.utils.SearchUtils.INSTANCE_ITEM_FIELD_NAME; +import static org.opensearch.index.query.QueryBuilders.boolQuery; +import static org.opensearch.index.query.QueryBuilders.termsQuery; +import static org.opensearch.search.sort.SortBuilders.fieldSort; +import static org.opensearch.search.sort.SortOrder.ASC; + +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.UUID; +import java.util.function.BiFunction; +import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import lombok.extern.log4j.Log4j2; +import org.folio.search.configuration.properties.SearchConfigurationProperties; import org.folio.search.converter.ConsortiumHoldingMapper; import org.folio.search.converter.ConsortiumItemMapper; import org.folio.search.domain.dto.ConsortiumHolding; @@ -13,9 +26,15 @@ import org.folio.search.domain.dto.ConsortiumItem; import org.folio.search.domain.dto.ConsortiumItemCollection; import org.folio.search.domain.dto.Instance; +import org.folio.search.exception.RequestValidationException; +import org.folio.search.model.SearchResult; import org.folio.search.model.service.CqlSearchRequest; +import org.folio.search.repository.SearchRepository; import org.folio.search.service.SearchService; +import org.folio.search.service.converter.ElasticsearchDocumentConverter; import org.folio.spring.FolioExecutionContext; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.search.builder.SearchSourceBuilder; import org.springframework.stereotype.Service; /** @@ -28,6 +47,9 @@ public class ConsortiumInstanceSearchService { private final SearchService searchService; + private final SearchRepository searchRepository; + private final ElasticsearchDocumentConverter documentConverter; + private final SearchConfigurationProperties properties; public ConsortiumHolding getConsortiumHolding(String id, CqlSearchRequest searchRequest) { var result = searchService.search(searchRequest); @@ -67,34 +89,105 @@ public ConsortiumItem getConsortiumItem(String id, CqlSearchRequest se return ConsortiumItemMapper.toConsortiumItem(instance.getId(), item); } - public ConsortiumHoldingCollection fetchConsortiumBatchHoldings(CqlSearchRequest searchRequest, - Set ids) { - var result = searchService.search(searchRequest); - var consortiumHoldings = result.getRecords().stream() - .flatMap(instance -> - instance.getHoldings().stream() - .filter(holding -> ids.contains(holding.getId())) - .map(holding -> ConsortiumHoldingMapper.toConsortiumHolding(instance.getId(), holding)) - ) - .toList(); + public ConsortiumHoldingCollection fetchConsortiumBatchHoldings(String tenant, Set holdingIds) { + validateIdsCount(holdingIds.size()); + + var ids = holdingIds.stream().map(UUID::toString).collect(Collectors.toSet()); + var targetField = INSTANCE_HOLDING_FIELD_NAME + ".id"; + + var searchRecords = getConsortiumBatchResults(tenant, ids, targetField, this::mapToConsortiumHolding); + + if (searchRecords.isEmpty()) { + return new ConsortiumHoldingCollection(); + } + return new ConsortiumHoldingCollection() - .holdings(consortiumHoldings) - .totalRecords(consortiumHoldings.size()); + .holdings(searchRecords.stream().map(SearchResult::getRecords).flatMap(List::stream).toList()) + .totalRecords(searchRecords.iterator().next().getTotalRecords()); } - public ConsortiumItemCollection fetchConsortiumBatchItems(CqlSearchRequest searchRequest, - Set ids) { - var result = searchService.search(searchRequest); - var consortiumItems = result.getRecords().stream() - .flatMap(instance -> - instance.getItems().stream() - .filter(item -> ids.contains(item.getId())) - .map(item -> ConsortiumItemMapper.toConsortiumItem(instance.getId(), item)) - ) - .toList(); + public ConsortiumItemCollection fetchConsortiumBatchItems(String tenant, Set itemIds) { + validateIdsCount(itemIds.size()); + + var ids = itemIds.stream().map(UUID::toString).collect(Collectors.toSet()); + var targetField = INSTANCE_ITEM_FIELD_NAME + ".id"; + + var searchRecords = getConsortiumBatchResults(tenant, ids, targetField, this::mapToConsortiumItem); + + if (searchRecords.isEmpty()) { + return new ConsortiumItemCollection(); + } return new ConsortiumItemCollection() - .items(consortiumItems) - .totalRecords(consortiumItems.size()); + .items(searchRecords.stream().map(SearchResult::getRecords).flatMap(List::stream).toList()) + .totalRecords(searchRecords.iterator().next().getTotalRecords()); + } + + private List> getConsortiumBatchResults(String tenant, Set ids, String targetField, + BiFunction, List> recordMapper) { + var request = CqlSearchRequest.of(Instance.class, tenant, "", 0, 0, true, false, true); + var termsQuery = termsQuery(targetField, ids); + + if (ids.size() < DEFAULT_MAX_SEARCH_RESULT_WINDOW) { + var searchSourceBuilder = queryBuilder(termsQuery, ids.size()); + var response = searchRepository.search(request, searchSourceBuilder); + var searchResult = documentConverter.convertToSearchResult(response, request.getResourceClass(), + (hits, item) -> recordMapper.apply(item, ids)); + var records = searchResult.getRecords().stream() + .flatMap(List::stream) + .toList(); + return List.of(SearchResult.of(records.size(), records)); + } + + var searchSourceBuilder = queryBuilder(termsQuery, properties.getSearchConsortiumRecordsPageSize()) + .sort(fieldSort(targetField).order(ASC)) + .searchAfter(new Object[]{""}); + var response = searchRepository.search(request, searchSourceBuilder); + List> searchRecords = new ArrayList<>(); + + while (response.getHits() != null && response.getHits().getHits().length > 0) { + var searchResult = documentConverter.convertToSearchResult(response, request.getResourceClass(), + (hits, item) -> recordMapper.apply(item, ids)); + var records = searchResult.getRecords().stream() + .flatMap(List::stream) + .toList(); + searchRecords.add(SearchResult.of(records.size(), records)); + var searchAfterValue = response.getHits() + .getAt(response.getHits().getHits().length - 1).getSortValues()[0]; + searchSourceBuilder.searchAfter(new Object[]{searchAfterValue}); + response = searchRepository.search(request, searchSourceBuilder); + } + + return searchRecords; + } + + private List mapToConsortiumHolding(Instance instance, Set ids) { + return instance.getHoldings().stream() + .filter(holding -> ids.contains(holding.getId())) + .map(holding -> ConsortiumHoldingMapper.toConsortiumHolding(instance.getId(), holding)) + .toList(); + } + + private List mapToConsortiumItem(Instance instance, Set ids) { + return instance.getItems().stream() + .filter(item -> ids.contains(item.getId())) + .map(holding -> ConsortiumItemMapper.toConsortiumItem(instance.getId(), holding)) + .toList(); + } + + private SearchSourceBuilder queryBuilder(QueryBuilder filterQuery, int size) { + return new SearchSourceBuilder() + .query(boolQuery().filter(filterQuery)) + .size(size) + .from(0) + .trackTotalHits(true); + } + + private void validateIdsCount(long count) { + var idsLimit = properties.getMaxSearchBatchRequestIdsCount(); + if (count > idsLimit) { + throw new RequestValidationException("IDs array size exceeds the maximum allowed limit %s".formatted(idsLimit), + "size", Long.toString(count)); + } } } diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml index 5671b3487..fabf0d0f7 100644 --- a/src/main/resources/application.yml +++ b/src/main/resources/application.yml @@ -61,6 +61,8 @@ folio: initial-languages: ${INITIAL_LANGUAGES:eng} max-supported-languages: ${MAX_SUPPORTED_LANGUAGES:5} max-browse-request-offset: ${MAX_BROWSE_REQUEST_OFFSET:500} + max-search-batch-request-ids-count: ${MAX_SEARCH_BATCH_REQUEST_IDS_COUNT:20000} + search-consortium-records-page-size: ${SEARCH_CONSORTIUM_RECORDS_PAGE_SIZE:5000} search-features: search-all-fields: ${SEARCH_BY_ALL_FIELDS_ENABLED:false} browse-cn-intermediate-values: ${BROWSE_CN_INTERMEDIATE_VALUES_ENABLED:true} diff --git a/src/main/resources/swagger.api/parameters/batchIdsDto.yaml b/src/main/resources/swagger.api/parameters/batchIdsDto.yaml index ce32f0619..367ec45b8 100644 --- a/src/main/resources/swagger.api/parameters/batchIdsDto.yaml +++ b/src/main/resources/swagger.api/parameters/batchIdsDto.yaml @@ -4,7 +4,6 @@ properties: ids: description: Entity IDs type: array - maxItems: 1000 items: type: string format: uuid diff --git a/src/test/java/org/folio/search/controller/BrowseClassificationConsortiumIT.java b/src/test/java/org/folio/search/controller/BrowseClassificationConsortiumIT.java index 448cb7ace..b495eac9c 100644 --- a/src/test/java/org/folio/search/controller/BrowseClassificationConsortiumIT.java +++ b/src/test/java/org/folio/search/controller/BrowseClassificationConsortiumIT.java @@ -28,7 +28,6 @@ import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.folio.search.domain.dto.BrowseOptionType; import org.folio.search.domain.dto.Classification; @@ -164,7 +163,7 @@ private static Instance instance(List data) { .map(pair -> new Classification() .classificationNumber(String.valueOf(pair.getFirst())) .classificationTypeId(String.valueOf(pair.getSecond()))) - .collect(Collectors.toList())) + .toList()) .staffSuppress(false) .discoverySuppress(false) .holdings(emptyList()); diff --git a/src/test/java/org/folio/search/controller/BrowseClassificationIT.java b/src/test/java/org/folio/search/controller/BrowseClassificationIT.java index 9fc8add1b..aaa11bafb 100644 --- a/src/test/java/org/folio/search/controller/BrowseClassificationIT.java +++ b/src/test/java/org/folio/search/controller/BrowseClassificationIT.java @@ -19,7 +19,6 @@ import java.util.List; import java.util.UUID; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.folio.search.domain.dto.BrowseConfig; import org.folio.search.domain.dto.BrowseOptionType; @@ -215,7 +214,7 @@ private static Instance instance(List data) { .map(pair -> new Classification() .classificationNumber(String.valueOf(pair.getFirst())) .classificationTypeId(String.valueOf(pair.getSecond()))) - .collect(Collectors.toList())) + .toList()) .staffSuppress(false) .discoverySuppress(false) .holdings(emptyList()); diff --git a/src/test/java/org/folio/search/controller/BrowseSubjectConsortiumIT.java b/src/test/java/org/folio/search/controller/BrowseSubjectConsortiumIT.java index 7b756e0c6..be59b659b 100644 --- a/src/test/java/org/folio/search/controller/BrowseSubjectConsortiumIT.java +++ b/src/test/java/org/folio/search/controller/BrowseSubjectConsortiumIT.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.folio.search.domain.dto.Facet; import org.folio.search.domain.dto.FacetResult; @@ -111,7 +110,7 @@ private static Instance instance(List data) { } else { return new Subject().value(String.valueOf(val)); } - }).collect(Collectors.toList())) + }).toList()) .staffSuppress(false) .discoverySuppress(false) .holdings(emptyList()); diff --git a/src/test/java/org/folio/search/controller/BrowseSubjectIT.java b/src/test/java/org/folio/search/controller/BrowseSubjectIT.java index 620afe578..35bc96125 100644 --- a/src/test/java/org/folio/search/controller/BrowseSubjectIT.java +++ b/src/test/java/org/folio/search/controller/BrowseSubjectIT.java @@ -16,7 +16,6 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import java.util.List; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.folio.search.domain.dto.Instance; import org.folio.search.domain.dto.Subject; @@ -275,7 +274,7 @@ private static Instance instance(List data) { } else { return new Subject().value(String.valueOf(val)); } - }).collect(Collectors.toList())) + }).toList()) .staffSuppress(false) .discoverySuppress(false) .holdings(emptyList()); diff --git a/src/test/java/org/folio/search/controller/ConsortiumSearchHoldingsIT.java b/src/test/java/org/folio/search/controller/ConsortiumSearchHoldingsIT.java index 2fa9da3f7..5e7af22ed 100644 --- a/src/test/java/org/folio/search/controller/ConsortiumSearchHoldingsIT.java +++ b/src/test/java/org/folio/search/controller/ConsortiumSearchHoldingsIT.java @@ -1,7 +1,7 @@ package org.folio.search.controller; import static org.assertj.core.api.Assertions.assertThat; -import static org.folio.search.controller.ConsortiumSearchItemsIT.WRONG_SIZE_MSG; +import static org.folio.search.controller.ConsortiumSearchItemsIT.WRONG_SIZE_MSG_TEMPLATE; import static org.folio.search.controller.SearchConsortiumController.REQUEST_NOT_ALLOWED_MSG; import static org.folio.search.model.Pair.pair; import static org.folio.search.sample.SampleInstances.getSemanticWeb; @@ -147,17 +147,18 @@ void tryGetConsortiumBatchHoldings_returns400_whenRequestedForNotCentralTenant() void tryGetConsortiumBatchHoldings_returns400_whenMoreIdsThanLimit() throws Exception { var request = new BatchIdsDto() .ids( - Stream.iterate(0, i -> i < 1001, i -> ++i) + Stream.iterate(0, i -> i < 501, i -> ++i) .map(i -> UUID.randomUUID()) .toList() ); - tryPost(consortiumBatchHoldingsSearchPath(), request) + tryPost(consortiumBatchHoldingsSearchPath(), CENTRAL_TENANT_ID, request) .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.errors[0].message", is(WRONG_SIZE_MSG))) - .andExpect(jsonPath("$.errors[0].type", is("MethodArgumentNotValidException"))) + .andExpect(jsonPath("$.errors[0].message", is(WRONG_SIZE_MSG_TEMPLATE.formatted(500)))) + .andExpect(jsonPath("$.errors[0].type", is("RequestValidationException"))) .andExpect(jsonPath("$.errors[0].code", is("validation_error"))) - .andExpect(jsonPath("$.errors[0].parameters[0].key", is("ids"))); + .andExpect(jsonPath("$.errors[0].parameters[0].key", is("size"))) + .andExpect(jsonPath("$.errors[0].parameters[0].value", is(request.getIds().size() + ""))); } private ConsortiumHolding[] getExpectedHoldings() { diff --git a/src/test/java/org/folio/search/controller/ConsortiumSearchItemsIT.java b/src/test/java/org/folio/search/controller/ConsortiumSearchItemsIT.java index 026ad9cff..8cf1e10ea 100644 --- a/src/test/java/org/folio/search/controller/ConsortiumSearchItemsIT.java +++ b/src/test/java/org/folio/search/controller/ConsortiumSearchItemsIT.java @@ -33,8 +33,7 @@ @IntegrationTest class ConsortiumSearchItemsIT extends BaseConsortiumIntegrationTest { - static final String WRONG_SIZE_MSG = - "size must be between 0 and 1000"; + static final String WRONG_SIZE_MSG_TEMPLATE = "IDs array size exceeds the maximum allowed limit %s"; @BeforeAll static void prepare() { @@ -153,17 +152,18 @@ void tryGetConsortiumBatchItems_returns400_whenRequestedForNotCentralTenant() th void tryGetConsortiumBatchItems_returns400_whenMoreIdsThanLimit() throws Exception { var request = new BatchIdsDto() .ids( - Stream.iterate(0, i -> i < 1001, i -> ++i) + Stream.iterate(0, i -> i < 501, i -> ++i) .map(i -> UUID.randomUUID()) .toList() ); - tryPost(consortiumBatchItemsSearchPath(), request) + tryPost(consortiumBatchItemsSearchPath(), CENTRAL_TENANT_ID, request) .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.errors[0].message", is(WRONG_SIZE_MSG))) - .andExpect(jsonPath("$.errors[0].type", is("MethodArgumentNotValidException"))) + .andExpect(jsonPath("$.errors[0].message", is(WRONG_SIZE_MSG_TEMPLATE.formatted(500)))) + .andExpect(jsonPath("$.errors[0].type", is("RequestValidationException"))) .andExpect(jsonPath("$.errors[0].code", is("validation_error"))) - .andExpect(jsonPath("$.errors[0].parameters[0].key", is("ids"))); + .andExpect(jsonPath("$.errors[0].parameters[0].key", is("size"))) + .andExpect(jsonPath("$.errors[0].parameters[0].value", is(request.getIds().size() + ""))); } private ConsortiumItem[] getExpectedItems() { diff --git a/src/test/java/org/folio/search/service/consortium/ConsortiumInstanceSearchServiceTest.java b/src/test/java/org/folio/search/service/consortium/ConsortiumInstanceSearchServiceTest.java index a7141b869..d8e581259 100644 --- a/src/test/java/org/folio/search/service/consortium/ConsortiumInstanceSearchServiceTest.java +++ b/src/test/java/org/folio/search/service/consortium/ConsortiumInstanceSearchServiceTest.java @@ -3,12 +3,25 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.folio.search.utils.TestConstants.CENTRAL_TENANT_ID; import static org.folio.search.utils.TestConstants.MEMBER_TENANT_ID; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.common.collect.Sets; import java.util.Collections; import java.util.List; import java.util.UUID; +import java.util.function.BiFunction; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import org.apache.lucene.search.TotalHits; +import org.folio.search.configuration.properties.SearchConfigurationProperties; +import org.folio.search.converter.ConsortiumHoldingMapper; +import org.folio.search.converter.ConsortiumItemMapper; import org.folio.search.domain.dto.ConsortiumHolding; import org.folio.search.domain.dto.ConsortiumHoldingCollection; import org.folio.search.domain.dto.ConsortiumItem; @@ -16,9 +29,12 @@ import org.folio.search.domain.dto.Holding; import org.folio.search.domain.dto.Instance; import org.folio.search.domain.dto.Item; +import org.folio.search.exception.RequestValidationException; import org.folio.search.model.SearchResult; import org.folio.search.model.service.CqlSearchRequest; +import org.folio.search.repository.SearchRepository; import org.folio.search.service.SearchService; +import org.folio.search.service.converter.ElasticsearchDocumentConverter; import org.folio.spring.testing.type.UnitTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -26,12 +42,19 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.search.SearchHit; +import org.opensearch.search.SearchHits; +import org.opensearch.search.builder.SearchSourceBuilder; @UnitTest @ExtendWith(MockitoExtension.class) class ConsortiumInstanceSearchServiceTest { private @Mock SearchService searchService; + private @Mock SearchConfigurationProperties properties; + private @Mock SearchRepository searchRepository; + private @Mock ElasticsearchDocumentConverter documentConverter; private @InjectMocks ConsortiumInstanceSearchService service; @Test @@ -156,59 +179,160 @@ void getConsortiumItem_positive_noDesiredItem() { } @Test - void fetchConsortiumBatchHoldings_positive() { - var ids = List.of(UUID.randomUUID().toString(), UUID.randomUUID().toString()); - var instances = List.of( - instanceForHoldings(List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), - holding(ids.get(0), MEMBER_TENANT_ID))), - instanceForHoldings(List.of(holding(ids.get(1), CENTRAL_TENANT_ID))), - instanceForHoldings(List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID)))); - var searchResult = SearchResult.of(instances.size(), instances); - var request = Mockito.>mock(); + void fetchConsortiumBatchHoldings_negative_exceedsAllowedIdsCount() { + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(10L); + var ids = IntStream.range(1, 12).mapToObj(i -> UUID.randomUUID()).collect(Collectors.toSet()); + + var ex = + assertThrows(RequestValidationException.class, + () -> service.fetchConsortiumBatchHoldings(CENTRAL_TENANT_ID, ids)); + + assertThat(ex.getMessage()).isEqualTo("IDs array size exceeds the maximum allowed limit %s" + .formatted(properties.getMaxSearchBatchRequestIdsCount())); + } + + @Test + void fetchConsortiumBatchHoldings_positive_notExceedsSearchResultWindow() { + var ids = List.of(UUID.randomUUID(), UUID.randomUUID()); + var instancesHoldings = List.of( + List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), + holding(ids.get(0).toString(), MEMBER_TENANT_ID)), + List.of(holding(ids.get(1).toString(), CENTRAL_TENANT_ID)), + List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID))); + var expectedConsortiumHoldings = instancesHoldings.subList(0, instancesHoldings.size() - 1).stream() + .map(holdings -> { + var holding = holdings.size() > 1 ? holdings.get(1) : holdings.get(0); + return ConsortiumHoldingMapper.toConsortiumHolding(UUID.randomUUID().toString(), holding); + }).toList(); + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(10L); + when(searchRepository.search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class))) + .thenReturn(mock(SearchResponse.class)); + when(documentConverter.convertToSearchResult(any(SearchResponse.class), eq(Instance.class), any(BiFunction.class))) + .thenReturn(SearchResult.of(2, List.of(expectedConsortiumHoldings))); var expected = new ConsortiumHoldingCollection() - .holdings(instances.subList(0, instances.size() - 1).stream().map(instance -> { - var holding = instance.getHoldings().size() > 1 ? instance.getHoldings().get(1) : instance.getHoldings().get(0); - return new ConsortiumHolding() - .id(holding.getId()) - .instanceId(instance.getId()) - .discoverySuppress(false) - .tenantId(holding.getTenantId()); - }).toList()) + .holdings(expectedConsortiumHoldings) .totalRecords(2); - when(searchService.search(request)).thenReturn(searchResult); + var result = service.fetchConsortiumBatchHoldings(CENTRAL_TENANT_ID, Sets.newHashSet(ids)); + + assertThat(result).isEqualTo(expected); + } + + @Test + void fetchConsortiumBatchHoldings_positive_exceedsSearchResultWindow() { + var ids = IntStream.range(1, 10002).mapToObj(i -> UUID.randomUUID()).toList(); + var instancesHoldings = List.of( + List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), + holding(ids.get(0).toString(), MEMBER_TENANT_ID)), + List.of(holding(ids.get(1).toString(), CENTRAL_TENANT_ID)), + List.of(holding(UUID.randomUUID().toString(), CENTRAL_TENANT_ID))); + var expectedConsortiumHoldings = instancesHoldings.subList(0, instancesHoldings.size() - 1).stream() + .map(holdings -> { + var holding = holdings.size() > 1 ? holdings.get(1) : holdings.get(0); + return ConsortiumHoldingMapper.toConsortiumHolding(UUID.randomUUID().toString(), holding); + }).toList(); + var responseMock1 = mock(SearchResponse.class); + var responseMock2 = mock(SearchResponse.class); + var hit = mock(SearchHit.class); + when(responseMock2.getHits()).thenReturn(SearchHits.empty()); + when(hit.getSortValues()).thenReturn(new Object[]{""}); + when(responseMock1.getHits()).thenReturn( + new SearchHits(new SearchHit[]{hit}, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0f)); + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(20000L); + when(properties.getSearchConsortiumRecordsPageSize()).thenReturn(2); + when(searchRepository.search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class))) + .thenReturn(responseMock1).thenReturn(responseMock2); + when(documentConverter.convertToSearchResult(eq(responseMock1), eq(Instance.class), any(BiFunction.class))) + .thenReturn(SearchResult.of(2, List.of(expectedConsortiumHoldings))); + var expected = new ConsortiumHoldingCollection() + .holdings(expectedConsortiumHoldings) + .totalRecords(2); - var result = service.fetchConsortiumBatchHoldings(request, Sets.newHashSet(ids)); + var result = service.fetchConsortiumBatchHoldings(CENTRAL_TENANT_ID, Sets.newHashSet(ids)); assertThat(result).isEqualTo(expected); + verify(searchRepository, times(2)) + .search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class)); + verify(documentConverter) + .convertToSearchResult(any(SearchResponse.class), eq(Instance.class), any(BiFunction.class)); } @Test - void fetchConsortiumBatchItems_positive() { - var ids = List.of(UUID.randomUUID().toString(), UUID.randomUUID().toString()); - var instances = List.of( - instanceForItems(List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), - item(ids.get(0), MEMBER_TENANT_ID))), - instanceForItems(List.of(item(ids.get(1), CENTRAL_TENANT_ID))), - instanceForItems(List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID)))); - var searchResult = SearchResult.of(instances.size(), instances); - var request = Mockito.>mock(); + void fetchConsortiumBatchItems_negative_exceedsAllowedIdsCount() { + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(10L); + var ids = IntStream.range(1, 12).mapToObj(i -> UUID.randomUUID()).collect(Collectors.toSet()); + + var ex = + assertThrows(RequestValidationException.class, () -> service.fetchConsortiumBatchItems(CENTRAL_TENANT_ID, ids)); + + assertThat(ex.getMessage()).isEqualTo("IDs array size exceeds the maximum allowed limit %s" + .formatted(properties.getMaxSearchBatchRequestIdsCount())); + } + + @Test + void fetchConsortiumBatchItems_positive_notExceedsSearchResultWindow() { + var ids = List.of(UUID.randomUUID(), UUID.randomUUID()); + var instancesItems = List.of( + List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), + item(ids.get(0).toString(), MEMBER_TENANT_ID)), + List.of(item(ids.get(1).toString(), CENTRAL_TENANT_ID)), + List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID))); + var expectedConsortiumItems = instancesItems.subList(0, instancesItems.size() - 1).stream() + .map(items -> { + var item = items.size() > 1 ? items.get(1) : items.get(0); + return ConsortiumItemMapper.toConsortiumItem(UUID.randomUUID().toString(), item); + }).toList(); + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(10L); + when(searchRepository.search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class))) + .thenReturn(mock(SearchResponse.class)); + when(documentConverter.convertToSearchResult(any(SearchResponse.class), eq(Instance.class), any(BiFunction.class))) + .thenReturn(SearchResult.of(2, List.of(expectedConsortiumItems))); var expected = new ConsortiumItemCollection() - .items(instances.subList(0, instances.size() - 1).stream().map(instance -> { - var item = instance.getItems().size() > 1 ? instance.getItems().get(1) : instance.getItems().get(0); - return new ConsortiumItem() - .id(item.getId()) - .instanceId(instance.getId()) - .tenantId(item.getTenantId()) - .holdingsRecordId(item.getHoldingsRecordId()); - }).toList()) + .items(expectedConsortiumItems) .totalRecords(2); - when(searchService.search(request)).thenReturn(searchResult); + var result = service.fetchConsortiumBatchItems(CENTRAL_TENANT_ID, Sets.newHashSet(ids)); + + assertThat(result).isEqualTo(expected); + } - var result = service.fetchConsortiumBatchItems(request, Sets.newHashSet(ids)); + @Test + void fetchConsortiumBatchItems_positive_exceedsSearchResultWindow() { + var ids = IntStream.range(1, 10002).mapToObj(i -> UUID.randomUUID()).toList(); + var instancesItems = List.of( + List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID), + item(ids.get(0).toString(), MEMBER_TENANT_ID)), + List.of(item(ids.get(1).toString(), CENTRAL_TENANT_ID)), + List.of(item(UUID.randomUUID().toString(), CENTRAL_TENANT_ID))); + var expectedConsortiumItems = instancesItems.subList(0, instancesItems.size() - 1).stream() + .map(items -> { + var item = items.size() > 1 ? items.get(1) : items.get(0); + return ConsortiumItemMapper.toConsortiumItem(UUID.randomUUID().toString(), item); + }).toList(); + var responseMock1 = mock(SearchResponse.class); + var responseMock2 = mock(SearchResponse.class); + var hit = mock(SearchHit.class); + when(responseMock2.getHits()).thenReturn(SearchHits.empty()); + when(hit.getSortValues()).thenReturn(new Object[]{""}); + when(responseMock1.getHits()).thenReturn( + new SearchHits(new SearchHit[]{hit}, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0f)); + when(properties.getMaxSearchBatchRequestIdsCount()).thenReturn(20000L); + when(properties.getSearchConsortiumRecordsPageSize()).thenReturn(2); + when(searchRepository.search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class))) + .thenReturn(responseMock1).thenReturn(responseMock2); + when(documentConverter.convertToSearchResult(eq(responseMock1), eq(Instance.class), any(BiFunction.class))) + .thenReturn(SearchResult.of(2, List.of(expectedConsortiumItems))); + var expected = new ConsortiumItemCollection() + .items(expectedConsortiumItems) + .totalRecords(2); + + var result = service.fetchConsortiumBatchItems(CENTRAL_TENANT_ID, Sets.newHashSet(ids)); assertThat(result).isEqualTo(expected); + verify(searchRepository, times(2)) + .search(any(CqlSearchRequest.class), any(SearchSourceBuilder.class)); + verify(documentConverter) + .convertToSearchResult(any(SearchResponse.class), eq(Instance.class), any(BiFunction.class)); } private Holding holding(String id, String tenantId) { @@ -223,16 +347,4 @@ private Item item(String id, String tenantId) { .tenantId(tenantId) .holdingsRecordId(UUID.randomUUID().toString()); } - - private Instance instanceForHoldings(List holdings) { - return new Instance() - .id(UUID.randomUUID().toString()) - .holdings(holdings); - } - - private Instance instanceForItems(List items) { - return new Instance() - .id(UUID.randomUUID().toString()) - .items(items); - } } diff --git a/src/test/resources/application.yml b/src/test/resources/application.yml index 486a6c3aa..eac95ee07 100644 --- a/src/test/resources/application.yml +++ b/src/test/resources/application.yml @@ -46,6 +46,7 @@ folio: environment: folio-test search-config: initial-languages: eng,fre,ita,spa,ger + max-search-batch-request-ids-count: ${MAX_SEARCH_BATCH_REQUEST_IDS_COUNT:500} search-features: search-all-fields: false browse-cn-intermediate-values: true