From 022755fb232d464d77d96830f81f4e831f83e471 Mon Sep 17 00:00:00 2001 From: Niels Erik Date: Wed, 12 Jul 2023 15:53:24 +0200 Subject: [PATCH] Support paging in /items-by-holdings-id API (MODINV-838) (#603) * Support paging in /items-by-holdings-id API (MODINV-838) --- ramls/inventory.raml | 3 +- .../resources/ItemsByHoldingsRecordId.java | 118 ++++++++---------- src/test/java/api/BoundWithTests.java | 14 ++- 3 files changed, 68 insertions(+), 67 deletions(-) diff --git a/ramls/inventory.raml b/ramls/inventory.raml index b5384ec10..c64e4bf3d 100644 --- a/ramls/inventory.raml +++ b/ramls/inventory.raml @@ -287,7 +287,8 @@ resourceTypes: schemaItem: item exampleItem: !include examples/items_get.json get: - is: [searchable: {description: "query by holdings record ID. This is a mandatory query parameter. + is: [pageable: {description: "The default page size is 200. Maximum page size is 10000."}, + searchable: {description: "query by holdings record ID. This is a mandatory query parameter. An optional parameter, 'relations', can be passed outside of the query to restrict what Items are returned based on their type of relationship with the holdings record. Possible values of the 'relations' parameter are: 'onlyBoundWiths', 'onlyBoundWithsSkipDirectlyLinkedItem'", diff --git a/src/main/java/org/folio/inventory/resources/ItemsByHoldingsRecordId.java b/src/main/java/org/folio/inventory/resources/ItemsByHoldingsRecordId.java index 2fc60da04..3089b5c48 100644 --- a/src/main/java/org/folio/inventory/resources/ItemsByHoldingsRecordId.java +++ b/src/main/java/org/folio/inventory/resources/ItemsByHoldingsRecordId.java @@ -5,14 +5,13 @@ import io.vertx.ext.web.Router; import io.vertx.ext.web.RoutingContext; import io.vertx.ext.web.client.WebClient; +import org.apache.commons.lang.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.folio.inventory.common.WebContext; import org.folio.inventory.common.api.request.PagingParameters; import org.folio.inventory.storage.Storage; import org.folio.inventory.storage.external.CollectionResourceClient; -import org.folio.inventory.storage.external.CqlQuery; -import org.folio.inventory.storage.external.MultipleRecordsFetchClient; import org.folio.inventory.support.http.client.OkapiHttpClient; import org.folio.inventory.support.http.server.ClientErrorResponse; import org.folio.inventory.support.http.server.FailureResponseConsumer; @@ -24,7 +23,6 @@ import java.net.URL; import java.util.Arrays; import java.util.List; -import java.util.stream.Collectors; import static java.lang.String.format; import static org.folio.inventory.support.CqlHelper.buildQueryByIds; @@ -36,7 +34,6 @@ public class ItemsByHoldingsRecordId extends Items private static final String RELATIVE_ITEMS_FOR_HOLDINGS_PATH = "/inventory/items-by-holdings-id"; // Supporting API private static final String BOUND_WITH_PARTS_STORAGE_PATH = "/inventory-storage/bound-with-parts"; - private static final String ITEM_STORAGE_PATH = "/item-storage/items"; private static final String RELATION_PARAM_ONLY_BOUND_WITHS = "onlyBoundWiths"; private static final String RELATION_PARAM_ONLY_BOUND_WITHS_SKIP_DIRECTLY_LINKED_ITEM = "onlyBoundWithsSkipDirectlyLinkedItem"; @@ -51,17 +48,17 @@ public void register( Router router ) } /** - * Finds the item IDs of all items under the given holdings record ID; - * then also finds all bound-with parts that includes this holdings record/title; - * then passes the holdings record ID as well as the bound-with item IDs, if any, on to - * {@link #joinAndRespondWithManyItems(RoutingContext,WebContext,List,String,String) joinAndRespondWithManyItems} - * for that method to retrieve all the actual Item objects. + * Finds bound-with parts involving the given holdings record and passes + * the bound-with item IDs on to + * {@link #respondWithRegularItemsAndBoundWithItems(RoutingContext,WebContext,List,String,String,PagingParameters) joinAndRespondWithManyItems} + * for that method to retrieve the actual Item objects together with Items directly attached to the holdings record. */ private void getBoundWithItems( RoutingContext routingContext) { WebContext context = new WebContext(routingContext); String queryByHoldingsRecordId = context.getStringParameter("query", null); String relationsParam = context.getStringParameter( "relations", null ); + PagingParameters pagingParameters = getPagingParameters(context); if (queryByHoldingsRecordId == null || !queryByHoldingsRecordId.contains( "holdingsRecordId" )) { ClientErrorResponse.badRequest(routingContext.response(), @@ -89,40 +86,31 @@ private void getBoundWithItems( RoutingContext routingContext) { String holdingsRecordId = keyVal[1]; - CollectionResourceClient itemsClient = - getCollectionResourceRepository( routingContext, context, ITEM_STORAGE_PATH ); - itemsClient.getMany("holdingsRecordId=="+holdingsRecordId, - 1000000, - 0, - response -> { - List holdingsRecordsItemIds = response.getJson() - .getJsonArray( "items" ).stream() - .map(item -> ((JsonObject) item).getString("id")) - .collect( Collectors.toList()); - - CollectionResourceClient boundWithPartsClient = - getCollectionResourceRepository( - routingContext, - context, - BOUND_WITH_PARTS_STORAGE_PATH); - - MultipleRecordsFetchClient itemsFetcher = MultipleRecordsFetchClient.builder() - .withCollectionPropertyName("boundWithParts") - .withExpectedStatus(200) - .withCollectionResourceClient(boundWithPartsClient) - .build(); - - BoundWithPartsCql boundWithPartsCql = new BoundWithPartsCql(holdingsRecordId); - itemsFetcher.find(holdingsRecordsItemIds, boundWithPartsCql::byHoldingsRecordIdOrListOfItemIds) - .thenAccept(boundWithParts -> - joinAndRespondWithManyItems(routingContext, context, boundWithParts, holdingsRecordId, relationsParam)); - }); + CollectionResourceClient boundWithPartsClient = + getCollectionResourceRepository( + routingContext, + context, + BOUND_WITH_PARTS_STORAGE_PATH); + + if (boundWithPartsClient != null) { + boundWithPartsClient.getMany("holdingsRecordId==" + holdingsRecordId, + 100, + 0, + response -> { + List boundWithItemIds = response.getJson() + .getJsonArray("boundWithParts").stream() + .map(part -> ((JsonObject) part).getString("itemId")) + .toList(); + respondWithRegularItemsAndBoundWithItems(routingContext, context, boundWithItemIds, holdingsRecordId, relationsParam, pagingParameters); + }); + } + } /** * Retrieves Inventory Items that are directly related to the provided holdings record ID * or that are bound-with items containing the holdings record ID/title. - * @param boundWithParts list of bound-with entries for bound-withs containing the + * @param boundWithItemIds list of bound-with entries for bound-withs containing the * given holdings record/title * @param holdingsRecordId the ID of the given holdings record * @param relationsParam optional parameter indicating a sub-set of items to retrieve - @@ -130,13 +118,13 @@ private void getBoundWithItems( RoutingContext routingContext) { * -- that contains the holdings record/title -- with or without the bound-with * directly linked to the given holdings-records. */ - private void joinAndRespondWithManyItems(RoutingContext routingContext, - WebContext webContext, - List boundWithParts, - String holdingsRecordId, - String relationsParam) { + private void respondWithRegularItemsAndBoundWithItems(RoutingContext routingContext, + WebContext webContext, + List boundWithItemIds, + String holdingsRecordId, + String relationsParam, + PagingParameters pagingParameters) { String itemQuery; - List itemIds; boolean onlyBoundWiths = relationsParam != null && ( relationsParam.equals(RELATION_PARAM_ONLY_BOUND_WITHS ) || relationsParam.equals( @@ -144,13 +132,9 @@ private void joinAndRespondWithManyItems(RoutingContext routingContext, boolean skipDirectlyLinkedItem = relationsParam != null && relationsParam.equals( RELATION_PARAM_ONLY_BOUND_WITHS_SKIP_DIRECTLY_LINKED_ITEM ); - itemIds = boundWithParts.stream() - .map( part -> part.getString( "itemId" ) ) - .collect( Collectors.toList() ); - - boolean boundWithsFound = itemIds.size()>0; + boolean boundWithsFound = !boundWithItemIds.isEmpty(); if (boundWithsFound) { - itemQuery = buildQueryByIds( itemIds ); + itemQuery = buildQueryByIds( boundWithItemIds ); if (skipDirectlyLinkedItem) { itemQuery += " and holdingsRecordId <>" + holdingsRecordId; @@ -169,7 +153,7 @@ private void joinAndRespondWithManyItems(RoutingContext routingContext, } try { storage.getItemCollection(webContext).findByCql(itemQuery, - new PagingParameters(1000000,0), success -> + pagingParameters, success -> respondWithManyItems(routingContext, webContext, success.getResult()), FailureResponseConsumer.serverError(routingContext.response())); } catch (UnsupportedEncodingException e) { @@ -203,19 +187,27 @@ protected OkapiHttpClient createHttpClient( exception.toString()))); } - static class BoundWithPartsCql { - private final String holdingsId; - - public BoundWithPartsCql(String holdingsRecordId) { - this.holdingsId = holdingsRecordId; + /** + * Ensures a limit on the page size of no more than 10,000 items at a time, + * defaults to a limit of 200 items for a page. + * @param context The web context to extract requested paging from + * @return Adapted paging parameters. + */ + private PagingParameters getPagingParameters (WebContext context) { + final String maxPageSize = "10000"; + String limit = context.getStringParameter("limit", "200"); + String offset = context.getStringParameter("offset", "0"); + if (!StringUtils.isNumeric(limit) || StringUtils.isEmpty(limit)) { + limit = "200"; + } else if (Integer.parseInt(limit) > Integer.parseInt(maxPageSize)) { + log.error("A paging of {} items was requested but the /items-by-holdings-id API cuts off the page at {} items.", limit, maxPageSize); + limit = maxPageSize; } - - public CqlQuery byHoldingsRecordIdOrListOfItemIds(List itemIds) { - return CqlQuery.exactMatchAny("id", itemIds) - .or(CqlQuery.exactMatch("holdingsRecordId", this.holdingsId)); - + if (!StringUtils.isNumeric(offset) || StringUtils.isEmpty(offset)) { + offset = "0"; } + PagingParameters enforcedPaging = new PagingParameters(Integer.parseInt(limit), Integer.parseInt(offset)); + log.debug("Paging resolved to limit: {}, offset: {}", enforcedPaging.limit, enforcedPaging.offset); + return enforcedPaging; } - - } diff --git a/src/test/java/api/BoundWithTests.java b/src/test/java/api/BoundWithTests.java index 8960d6526..b5e8ec164 100644 --- a/src/test/java/api/BoundWithTests.java +++ b/src/test/java/api/BoundWithTests.java @@ -192,7 +192,7 @@ public void canRetrieveBoundWithItemByHoldingsRecordId() throws InterruptedExcep Response itemsResponse2 = okapiClient.get(ApiTestSuite.apiRoot()+ "/inventory/items-by-holdings-id?query=holdingsRecordId==" - +holdings1a.getJson().getString( "id" )) + +holdings1a.getJson().getString( "id" )+"&offset=0&limit=20000") .toCompletableFuture().get(5, SECONDS); assertThat("One and only one bound-with item is found: ", itemsResponse2.getJson().getInteger( "totalRecords" ), is(1)); @@ -215,7 +215,7 @@ public void canRetrieveBoundWithItemByHoldingsRecordId() throws InterruptedExcep Response itemsResponse5 = okapiClient.get(ApiTestSuite.apiRoot()+ "/inventory/items-by-holdings-id?query=holdingsRecordId==" - +holdings3a.getJson().getString( "id" )) + +holdings3a.getJson().getString( "id" )+"&offset=string&limit=string") .toCompletableFuture().get(5, SECONDS); assertThat("One item is found for 'holdings3a' (non-bound-with) with relations criterion: ", itemsResponse5.getJson().getInteger( "totalRecords" ), is(1)); @@ -234,7 +234,7 @@ public void canRetrieveManyItemsThroughItemsByHoldingsId() throws InterruptedExc } Response itemsResponse = okapiClient.get(ApiTestSuite.apiRoot()+ "/inventory/items-by-holdings-id?query=holdingsRecordId==" - +holdings1.getJson().getString( "id" )) + +holdings1.getJson().getString( "id" )+"&offset=0&limit=1200") .toCompletableFuture().get(5, SECONDS); assertThat("1100 items found for 'holdings1': ", itemsResponse.getJson().getInteger( "totalRecords" ), is(1100)); assertThat("1100 items found for 'holdings1': ", itemsResponse.getJson().getJsonArray("items").size(), is(1100)); @@ -284,6 +284,14 @@ public void mustQueryBoundWithItemsByHoldingsRecordIdId() assertThat("Response code 400 (bad request) expected when not querying by holdingsRecordId", itemsResponse3.getStatusCode(), is(400)); + + Response itemsResponse4 = okapiClient.get(ApiTestSuite.apiRoot()+ + "/inventory/items-by-holdings-id/?query=holdingsRecordId") + .toCompletableFuture().get(5, SECONDS); + + assertThat("Response code 400 (bad request) expected when not querying by holdingsRecordId", + itemsResponse4.getStatusCode(), is(400)); + } @Test