diff --git a/NEWS.md b/NEWS.md index 4a5b86175..21965e31e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,6 +19,7 @@ ### Bug fixes * Add item barcode right truncation search index ([MODINVSTOR-1292](https://folio-org.atlassian.net/browse/MODINVSTOR-1292)) +* Sort holdings by location name in instanceId query ([MODINVSTOR-1343](https://folio-org.atlassian.net/browse/MODINVSTOR-1343)) ### Tech Dept * Description ([ISSUE](https://folio-org.atlassian.net/browse/ISSUE)) diff --git a/ramls/holdings-storage.raml b/ramls/holdings-storage.raml index 1accb70e4..a9f5cbfb9 100755 --- a/ramls/holdings-storage.raml +++ b/ramls/holdings-storage.raml @@ -36,8 +36,13 @@ resourceTypes: exampleItem: !include examples/holdings-storage/holdingsRecord_get.json get: is: [pageable, - searchable: {description: "by instance ID (using CQL)", - example: "instanceId=\"2b94c631-fca9-4892-a730-03ee529ffe2a\""}, + searchable: { + description: + "Sorting by a foreign table field is not supported; the only exception is a query of this form: + 'instanceId==\"\" sortBy [callNumberPrefix|callNumber|callNumberSuffix|effectiveLocation.name]+' + quoting the UUID is optional.", + example: "instanceId==\"2b94c631-fca9-4892-a730-03ee529ffe2a\"" + }, ] post: is: [validate] diff --git a/src/main/java/org/folio/persist/HoldingsRepository.java b/src/main/java/org/folio/persist/HoldingsRepository.java index 7d8e086d6..48960d0f2 100644 --- a/src/main/java/org/folio/persist/HoldingsRepository.java +++ b/src/main/java/org/folio/persist/HoldingsRepository.java @@ -2,11 +2,13 @@ import static org.folio.rest.impl.HoldingsStorageApi.HOLDINGS_RECORD_TABLE; import static org.folio.rest.persist.PgUtil.postgresClient; +import static org.folio.services.location.LocationService.LOCATION_TABLE; import io.vertx.core.Context; import io.vertx.core.Future; import io.vertx.sqlclient.Row; import io.vertx.sqlclient.RowSet; +import io.vertx.sqlclient.Tuple; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -19,6 +21,53 @@ public HoldingsRepository(Context context, Map okapiHeaders) { super(postgresClient(context, okapiHeaders), HOLDINGS_RECORD_TABLE, HoldingsRecord.class); } + /** + * Row where the first value is an array with all holdings records; the second value is + * the exact totalRecords count. + */ + public Future getByInstanceId(String instanceId, String[] sortBys, int offset, int limit) { + var orderBy = new StringBuilder(); + for (var sortBy : sortBys) { + if (sortBy.isEmpty()) { + continue; + } + var s = switch (sortBy) { + case "effectiveLocation.name" -> "name"; + case "callNumberPrefix", + "callNumber", + "callNumberSuffix" -> "jsonb->>'" + sortBy + "'"; + default -> null; + }; + if (s == null) { + return Future.failedFuture(new IllegalArgumentException("sortBy: " + sortBy)); + } + if (!orderBy.isEmpty()) { + orderBy.append(", "); + } + orderBy.append(s); + } + var sql = "WITH data AS (" + + " SELECT h.jsonb AS jsonb, l.jsonb->>'name' AS name" + + " FROM " + HOLDINGS_RECORD_TABLE + " h" + + " LEFT JOIN " + LOCATION_TABLE + " l" + + " ON h.effectiveLocationId = l.id" + + " WHERE h.instanceId=$1" + + " )" + + " SELECT json_array(" + + " SELECT jsonb" + + " FROM data" + + " ORDER BY " + orderBy + + " OFFSET $2" + + " LIMIT $3" + + " )::text, (" + + " SELECT COUNT(*)" + + " FROM " + HOLDINGS_RECORD_TABLE + + " WHERE instanceId=$1" + + ")"; + return postgresClient.withReadConn(conn -> conn.execute(sql, Tuple.of(instanceId, offset, limit))) + .map(rowSet -> rowSet.iterator().next()); + } + /** * Delete by CQL. For each deleted record return a {@link Row} with the instance id String * and with the holdings' jsonb String. diff --git a/src/main/java/org/folio/rest/impl/HoldingsStorageApi.java b/src/main/java/org/folio/rest/impl/HoldingsStorageApi.java index 82bfde2a1..d4e49747e 100644 --- a/src/main/java/org/folio/rest/impl/HoldingsStorageApi.java +++ b/src/main/java/org/folio/rest/impl/HoldingsStorageApi.java @@ -29,9 +29,17 @@ public void getHoldingsStorageHoldings(String totalRecords, int offset, int limi RoutingContext routingContext, Map okapiHeaders, Handler> asyncResultHandler, Context vertxContext) { - - PgUtil.streamGet(HOLDINGS_RECORD_TABLE, HoldingsRecordView.class, query, offset, - limit, null, "holdingsRecords", routingContext, okapiHeaders, vertxContext); + new HoldingsService(vertxContext, okapiHeaders) + .getByInstanceId(offset, limit, query) + .onSuccess(response -> { + if (response != null) { + asyncResultHandler.handle(succeededFuture(response)); + return; + } + PgUtil.streamGet(HOLDINGS_RECORD_TABLE, HoldingsRecordView.class, query, offset, + limit, null, "holdingsRecords", routingContext, okapiHeaders, vertxContext); + }) + .onFailure(handleFailure(asyncResultHandler)); } @Validate diff --git a/src/main/java/org/folio/services/holding/HoldingsService.java b/src/main/java/org/folio/services/holding/HoldingsService.java index d2e854c5a..6fb869375 100644 --- a/src/main/java/org/folio/services/holding/HoldingsService.java +++ b/src/main/java/org/folio/services/holding/HoldingsService.java @@ -28,6 +28,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; @@ -55,6 +57,12 @@ public class HoldingsService { private static final Logger log = getLogger(HoldingsService.class); private static final ObjectMapper OBJECT_MAPPER = ObjectMapperTool.getMapper(); + private static final Pattern INSTANCEID_PATTERN = Pattern.compile( + "^ *instanceId *== *\"?(" + + "[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}" + + ")\"?" + + " +sortBy" + + "(( +(effectiveLocation\\.name|callNumberPrefix|callNumber|callNumberSuffix))+) *$"); private final Context vertxContext; private final Map okapiHeaders; @@ -82,6 +90,35 @@ public HoldingsService(Context context, Map okapiHeaders) { context.get(ConsortiumDataCache.class.getName())); } + /** + * Returns Response if the query is supported by instanceId query, null otherwise. + * + *

+ */ + public Future getByInstanceId(int offset, int limit, String query) { + if (query == null) { + return Future.succeededFuture(); + } + var matcher = INSTANCEID_PATTERN.matcher(query); + if (!matcher.find()) { + return Future.succeededFuture(); + } + var instanceId = matcher.group(1); + var sortBy = matcher.group(2).split(" +"); + return holdingsRepository.getByInstanceId(instanceId, sortBy, offset, limit) + .map(row -> { + var json = "{ \"holdingsRecords\": " + row.getString(0) + ",\n" + + " \"totalRecords\": " + row.getLong(1) + ",\n" + + " \"resultInfo\": { \n" + + " \"totalRecords\": " + row.getLong(1) + ",\n" + + " \"totalRecordsEstimated\": false\n" + + " }\n" + + "}"; + return Response.ok(json, MediaType.APPLICATION_JSON).build(); + }) + .onFailure(e -> log.error("getByInstanceId:: {}", e.getMessage(), e)); + } + /** * Deletes all holdings but sends only a single domain event (Kafka) message "all records removed", * this is much faster than sending one message for each deleted holding. diff --git a/src/main/resources/templates/db_scripts/schema.json b/src/main/resources/templates/db_scripts/schema.json index f92d732c5..b50ad09a7 100644 --- a/src/main/resources/templates/db_scripts/schema.json +++ b/src/main/resources/templates/db_scripts/schema.json @@ -624,6 +624,7 @@ { "fieldName": "effectiveLocationId", "targetTable": "location", + "targetTableAlias": "effectiveLocation", "tOps": "ADD" }, { diff --git a/src/test/java/org/folio/rest/api/HoldingsStorageTest.java b/src/test/java/org/folio/rest/api/HoldingsStorageTest.java index a868b9cfb..3273f4679 100644 --- a/src/test/java/org/folio/rest/api/HoldingsStorageTest.java +++ b/src/test/java/org/folio/rest/api/HoldingsStorageTest.java @@ -59,11 +59,13 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import junitparams.JUnitParamsRunner; import junitparams.Parameters; +import kotlin.jvm.functions.Function4; import lombok.SneakyThrows; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; @@ -86,6 +88,7 @@ import org.folio.rest.tools.utils.OptimisticLockingUtil; import org.folio.services.consortium.entities.SharingInstance; import org.folio.services.consortium.entities.SharingStatus; +import org.folio.util.PercentCodec; import org.junit.After; import org.junit.AfterClass; import org.junit.Before; @@ -699,6 +702,54 @@ public void canPageAllHoldings() assertThat(secondPage.getInteger("totalRecords"), is(5)); } + @Test + @SneakyThrows + public void canGetByInstanceId() { + UUID instanceId = UUID.randomUUID(); + instancesClient.create(nod(instanceId)); + Function4 createHolding = + (location, prefix, callNumber, suffix) -> createHolding(new HoldingRequestBuilder() + .forInstance(instanceId) + .withSource(getPreparedHoldingSourceId()) + .withPermanentLocation(location) + .withCallNumberPrefix(prefix) + .withCallNumber(callNumber) + .withCallNumberSuffix(suffix)) + .getId().toString(); + final var h7 = createHolding.invoke(SECOND_FLOOR_LOCATION_ID, "b", "k", "p"); + final var h6 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "c", "j", "q"); + final var h5 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "b", "k", "q"); + final var h4 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "a", "l", "r"); + final var h3 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "a", "k", "o"); + final var h2 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "a", "j", "q"); + final var h1 = createHolding.invoke(MAIN_LIBRARY_LOCATION_ID, "a", "j", "p"); + final var h0 = createHolding.invoke(ANNEX_LIBRARY_LOCATION_ID, "b", "k", "q"); + Function> getHoldingsIds = query -> holdingsClient.getByQuery(query).stream() + .map(holding -> holding.getString("id")).toList(); + + var query = "instanceId==" + instanceId + + " sortBy effectiveLocation.name callNumberPrefix callNumber callNumberSuffix"; + query = "?query=" + PercentCodec.encode(query); + var ids = getHoldingsIds.apply(query); + assertThat(ids, is(List.of(h0, h1, h2, h3, h4, h5, h6, h7))); + + ids = getHoldingsIds.apply(query + "&offset=2&limit=4"); + assertThat(ids, is(List.of(h2, h3, h4, h5))); + + query = "instanceId==" + instanceId + " sortBy callNumberSuffix callNumber callNumberPrefix effectiveLocation.name"; + query = "?query=" + PercentCodec.encode(query); + ids = getHoldingsIds.apply(query); + // h3 M a k o + // h1 M a j p + // h7 S b k p + // h2 M a j q + // h6 M c j q + // h0 A b k q + // h5 M b k q + // h4 M a l r + assertThat(ids, is(List.of(h3, h1, h7, h2, h6, h0, h5, h4))); + } + @Test public void canDeleteAllHoldings() { UUID firstInstanceId = UUID.randomUUID(); @@ -1059,7 +1110,7 @@ public void updatingHoldingsUpdatesItemShelvingOrder( UUID holdingId = UUID.randomUUID(); - final JsonObject holding = holdingsClient.create(new HoldingRequestBuilder() + final JsonObject holding = createHolding(new HoldingRequestBuilder() .withId(holdingId) .forInstance(instanceId) .withSource(getPreparedHoldingSourceId()) @@ -3362,6 +3413,12 @@ private void assertHridRange(Response response, String minHrid, String maxHrid) is(both(greaterThanOrEqualTo(minHrid)).and(lessThanOrEqualTo(maxHrid)))); } + private IndividualResource createHolding(HoldingRequestBuilder holding) { + return holdingsClient.create(holding.create(), + TENANT_ID, + Map.of(X_OKAPI_URL, mockServer.baseUrl())); + } + private Response create(URL url, Object entity) throws InterruptedException, ExecutionException, TimeoutException { CompletableFuture createCompleted = new CompletableFuture<>();