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

https://folio-org.atlassian.net/browse/MODINVSTOR-1343

The UI has the instance view that shows the holdings that belong to that instance. There are many libraries with many holdings per instance, they need this sorted by effectiveLocation.name, callNumberPrefix, callNumber, callNumberSuffix.

RMB doesn’t support sorting by effectiveLocation.name because that’s a foreign table field. It only supports sorting by effectiveLocationId.

Implement this special case that the UI will use:

instanceId==5b966ed6-3cd6-4dda-a0c7-f9b5e2826ccd sortBy effectiveLocation.name callNumberPrefix callNumber callNumberSuffix

Allow any sub-set and order for the sortBy fields.

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))

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,53 @@ public HoldingsRepository(Context context, Map<String, String> 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<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 ("
+        + " 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.

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(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.

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<>();