Merge pull request #1128 from folio-org/MODINVSTOR-1343

MODINVSTOR-1343: Sort holdings by location name in instanceId query

NEWS.md

@@ -20,6 +20,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))

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==\"<UUID>\" sortBy [callNumberPrefix|callNumber|callNumberSuffix|effectiveLocation.name]+'
+                quoting the UUID is optional.",
+             example: "instanceId==\"2b94c631-fca9-4892-a730-03ee529ffe2a\""
+           },
           ]
     post:
       is: [validate]

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,54 @@ public HoldingsRepository(Context context, Map<String, String> okapiHeaders) {
     super(postgresClient(context, okapiHeaders), HOLDINGS_RECORD_TABLE, HoldingsRecord.class);
   }
 
+  /**
+   * Produce a single row where the {@code holdings} column is a text field
+   * containing a JSON array with all holdings records; the
+   * {@code total_records} column is the exact totalRecords count.
+   */
+  public Future<Row> 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 NOT MATERIALIZED ("
+        + " 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 AS holdings, ("
+        + "   SELECT COUNT(*) AS total_records"
+        + "   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.

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<String, String> okapiHeaders,
                                          Handler<AsyncResult<Response>> 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

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,14 @@
 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 *== *\"?("
+      // UUID
+      + "[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"
+      // allow any sub-set of the fields and allow the fields in any order
+      + "(( +(effectiveLocation\\.name|callNumberPrefix|callNumber|callNumberSuffix))+) *$");
 
   private final Context vertxContext;
   private final Map<String, String> okapiHeaders;
@@ -82,6 +92,35 @@ public HoldingsService(Context context, Map<String, String> okapiHeaders) {
       context.get(ConsortiumDataCache.class.getName()));
   }
 
+  /**
+   * Returns Response if the query is supported by instanceId query, null otherwise.
+   *
+   * <p>
+   */
+  public Future<Response> 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("holdings") + ",\n"
+              + "  \"totalRecords\": " + row.getLong("total_records") + ",\n"
+              + "  \"resultInfo\": { \n"
+              + "    \"totalRecords\": " + row.getLong("total_records") + ",\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.

src/main/resources/templates/db_scripts/schema.json

@@ -624,6 +624,7 @@
         {
           "fieldName": "effectiveLocationId",
           "targetTable": "location",
+          "targetTableAlias": "effectiveLocation",
           "tOps": "ADD"
         },
         {

src/test/java/org/folio/persist/HoldingsRepositoryTest.java

@@ -0,0 +1,24 @@
+package org.folio.persist;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+
+import io.vertx.core.Vertx;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+class HoldingsRepositoryTest {
+
+  @Test
+  void invalidSortBy() {
+    String[] sortBys = {"foo"};
+    var context = Vertx.vertx().getOrCreateContext();
+    var headers = Map.of("X-Okapi-Tenant", "diku");
+    var holdingsRepository = new HoldingsRepository(context, headers);
+    var future = holdingsRepository.getByInstanceId(null, sortBys, 0, 0);
+    assertThat(future.cause(), is(instanceOf(IllegalArgumentException.class)));
+    assertThat(future.cause().getMessage(), is("sortBy: foo"));
+  }
+
+}

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<UUID, String, String, String, String> 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<String, List<String>> 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<Response> createCompleted = new CompletableFuture<>();