From 17a7881f5fb30cd7e2a006b1380d2c081b0cd132 Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Wed, 2 Aug 2023 14:56:47 +0200 Subject: [PATCH 1/2] MODINVSTOR-941: Batch holdings update updates items Closes https://issues.folio.org/browse/MODINVSTOR-941 "Location changes in holdings records via batch-synchronous APIs not triggering update to item effective location" Partly addresses https://issues.folio.org/browse/MODINVSTOR-870 "POST/PUT instance/holding/item performance" * Update items when doing a bulk holdings updates. Item properties to update: * effective shelving order * effective call number components * effective location id * Database trigger fills holdings HRID if needed * No performance regression * Performance improvements Move all Java code needed to update affected items into SQL functions. Otherwise Java code must fetch and check item records that requires round trips from Java to DB and back to Java and database locks for consistency resulting in an unacceptable performance regression. Performance of this bulk holdings update API is a critical requirement for several institutions. Move Java code that fetches the next holdings HRID from database into a database trigger. This avoids a holdings record query and an HRID query in Java. The code changes are minimal so that is can easily been back-ported to Orchid and released as a critical service patch (CSP). Separate pull requests will remove all Java code that is duplicated in SQL functions; they will not be back-ported as CSPs: https://issues.folio.org/browse/MODINVSTOR-1090 https://issues.folio.org/browse/MODINVSTOR-1091 - [x] Adjust existing tests and add new ones for the the new implementation --- pom.xml | 6 +- ramls/holdingsrecord.json | 2 +- .../org/folio/persist/HoldingsRepository.java | 26 ++ .../rest/support/EndpointFailureHandler.java | 5 + .../AbstractDomainEventPublisher.java | 8 + .../CommonDomainEventPublisher.java | 10 + .../services/holding/HoldingsService.java | 115 +++--- .../instance-hr-item/hridTrigger.sql | 32 ++ .../db_scripts/instance-hr-item/write.sql | 379 ++++++++++++++++++ .../templates/db_scripts/schema.json | 10 + .../folio/persist/HoldingsRepositoryTest.java | 45 +++ .../folio/rest/api/HoldingsStorageTest.java | 124 +++++- ...ItemEffectiveCallNumberComponentsTest.java | 11 +- .../rest/api/ItemEffectiveLocationTest.java | 5 +- .../org/folio/rest/impl/WriteSqlTest.java | 226 +++++++++++ .../kafka/GroupedCollectedMessages.java | 3 +- .../messages/ItemEventMessageChecks.java | 1 - .../folio/services/CallNumberUtilsTest.java | 61 ++- 18 files changed, 989 insertions(+), 80 deletions(-) create mode 100644 src/main/resources/templates/db_scripts/instance-hr-item/hridTrigger.sql create mode 100644 src/main/resources/templates/db_scripts/instance-hr-item/write.sql create mode 100644 src/test/java/org/folio/persist/HoldingsRepositoryTest.java create mode 100644 src/test/java/org/folio/rest/impl/WriteSqlTest.java diff --git a/pom.xml b/pom.xml index 7b7502a68..f12caa7c2 100644 --- a/pom.xml +++ b/pom.xml @@ -70,7 +70,6 @@ - org.marc4j marc4j @@ -181,6 +180,11 @@ vertx-unit test + + io.vertx + vertx-junit5 + test + org.apache.commons commons-lang3 diff --git a/ramls/holdingsrecord.json b/ramls/holdingsrecord.json index a2a70e698..370d2abe5 100644 --- a/ramls/holdingsrecord.json +++ b/ramls/holdingsrecord.json @@ -14,7 +14,7 @@ }, "hrid": { "type": "string", - "description": "the human readable ID, also called eye readable ID. A system-assigned sequential ID which maps to the Instance ID" + "description": "the human readable ID, also called eye readable ID. Unique. On creation it can be provided, otherwise the system assigns the next sequential ID using the settings of the /hrid-settings-storage/hrid-settings API. An update with a different hrid is rejected. An update without hrid is populated with the holdings record's current hrid." }, "holdingsTypeId": { "type": "string", diff --git a/src/main/java/org/folio/persist/HoldingsRepository.java b/src/main/java/org/folio/persist/HoldingsRepository.java index 2845cf1d1..e3954ef3a 100644 --- a/src/main/java/org/folio/persist/HoldingsRepository.java +++ b/src/main/java/org/folio/persist/HoldingsRepository.java @@ -5,10 +5,14 @@ import io.vertx.core.Context; import io.vertx.core.Future; +import io.vertx.core.json.JsonObject; import io.vertx.sqlclient.Row; import io.vertx.sqlclient.RowSet; +import io.vertx.sqlclient.Tuple; +import java.util.List; import java.util.Map; import org.folio.cql2pgjson.CQL2PgJSON; +import org.folio.dbschema.ObjectMapperTool; import org.folio.rest.jaxrs.model.HoldingsRecord; import org.folio.rest.persist.cql.CQLWrapper; @@ -17,6 +21,28 @@ public HoldingsRepository(Context context, Map okapiHeaders) { super(postgresClient(context, okapiHeaders), HOLDINGS_RECORD_TABLE, HoldingsRecord.class); } + /** + * Upsert holdings records. + * + *

Returns + * { "holdingsRecords": [{"old": {...}, "new": {...}}, ...], + * "items": [{"old": {...}, "new": {...}}, ...] + * } + * providing old and new jsonb content of all updated holdings and items. For a newly + * inserted holding only "new" is provided. + * + * @param holdingsRecords records to insert or update (upsert) + */ + public Future upsert(List holdingsRecords) { + try { + var array = ObjectMapperTool.getMapper().writeValueAsString(holdingsRecords); + return postgresClient.selectSingle("SELECT upsert_holdings($1::text::jsonb)", Tuple.of(array)) + .map(row -> row.getJsonObject(0)); + } catch (Exception e) { + return Future.failedFuture(e); + } + } + /** * 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/support/EndpointFailureHandler.java b/src/main/java/org/folio/rest/support/EndpointFailureHandler.java index 0020d216b..ce86438c7 100644 --- a/src/main/java/org/folio/rest/support/EndpointFailureHandler.java +++ b/src/main/java/org/folio/rest/support/EndpointFailureHandler.java @@ -13,6 +13,7 @@ import org.folio.rest.exceptions.NotFoundException; import org.folio.rest.exceptions.ValidationException; import org.folio.rest.jaxrs.model.Errors; +import org.folio.rest.persist.PgExceptionFacade; import org.folio.rest.persist.PgExceptionUtil; import org.folio.rest.persist.cql.CQLQueryValidationException; import org.folio.rest.tools.client.exceptions.ResponseException; @@ -68,6 +69,10 @@ public static Response failureResponse(Throwable error) { } else if (error instanceof ResponseException) { return ((ResponseException) error).getResponse(); } + var sqlState = new PgExceptionFacade(error).getSqlState(); + if ("239HR".equals(sqlState)) { // Cannot change hrid of holdings record + return textPlainResponse(400, error); + } String message = PgExceptionUtil.badRequestMessage(error); if (message != null) { return textPlainResponse(400, message); diff --git a/src/main/java/org/folio/services/domainevent/AbstractDomainEventPublisher.java b/src/main/java/org/folio/services/domainevent/AbstractDomainEventPublisher.java index c9b0f17c1..eb2a391a5 100644 --- a/src/main/java/org/folio/services/domainevent/AbstractDomainEventPublisher.java +++ b/src/main/java/org/folio/services/domainevent/AbstractDomainEventPublisher.java @@ -11,6 +11,7 @@ import io.vertx.core.Future; import io.vertx.core.Handler; +import io.vertx.core.json.JsonObject; import java.util.Collection; import java.util.List; import javax.ws.rs.core.Response; @@ -92,6 +93,13 @@ public Future publishAllRemoved() { return domainEventService.publishAllRecordsRemoved(); } + public Future publishUpserted(String instanceId, JsonObject oldRecord, JsonObject newRecord) { + if (oldRecord == null) { + return domainEventService.publishRecordCreated(instanceId, newRecord.encode()); + } + return domainEventService.publishRecordUpdated(instanceId, oldRecord.encode(), newRecord.encode()); + } + public Handler publishUpdated(D oldRecord) { return response -> { if (!isUpdateSuccessResponse(response)) { diff --git a/src/main/java/org/folio/services/domainevent/CommonDomainEventPublisher.java b/src/main/java/org/folio/services/domainevent/CommonDomainEventPublisher.java index 3457d2a22..137aea0ae 100644 --- a/src/main/java/org/folio/services/domainevent/CommonDomainEventPublisher.java +++ b/src/main/java/org/folio/services/domainevent/CommonDomainEventPublisher.java @@ -116,6 +116,11 @@ Future publishRecordUpdated(String instanceId, T oldRecord, T newRecord) { return publish(instanceId, domainEvent); } + Future publishRecordUpdated(String instanceId, String oldRecord, String newRecord) { + var domainEvent = DomainEventRaw.updateEvent(oldRecord, newRecord, tenantId(okapiHeaders)); + return publish(instanceId, domainEvent); + } + Future publishRecordsUpdated(Collection> updatedRecords) { if (updatedRecords.isEmpty()) { return succeededFuture(); @@ -133,6 +138,11 @@ Future publishRecordCreated(String instanceId, T newRecord) { return publish(instanceId, domainEvent); } + Future publishRecordCreated(String id, String newRecord) { + var domainEvent = DomainEventRaw.createEvent(newRecord, tenantId(okapiHeaders)); + return publish(id, domainEvent); + } + Future publishRecordsCreated(List> records) { if (records.isEmpty()) { return succeededFuture(); diff --git a/src/main/java/org/folio/services/holding/HoldingsService.java b/src/main/java/org/folio/services/holding/HoldingsService.java index e5463c613..dd995b919 100644 --- a/src/main/java/org/folio/services/holding/HoldingsService.java +++ b/src/main/java/org/folio/services/holding/HoldingsService.java @@ -1,7 +1,6 @@ package org.folio.services.holding; import static io.vertx.core.Promise.promise; -import static org.apache.logging.log4j.LogManager.getLogger; import static org.folio.rest.impl.HoldingsStorageApi.HOLDINGS_RECORD_TABLE; import static org.folio.rest.impl.StorageHelper.MAX_ENTITIES; import static org.folio.rest.jaxrs.resource.HoldingsStorage.DeleteHoldingsStorageHoldingsByHoldingsRecordIdResponse; @@ -14,39 +13,34 @@ import static org.folio.rest.persist.PgUtil.postSync; import static org.folio.rest.persist.PgUtil.postgresClient; import static org.folio.services.batch.BatchOperationContextFactory.buildBatchOperationContext; -import static org.folio.validator.HridValidators.refuseWhenHridChanged; -import io.vertx.core.AsyncResult; import io.vertx.core.Context; import io.vertx.core.Future; -import io.vertx.core.Handler; import io.vertx.core.Promise; +import io.vertx.core.json.JsonObject; +import java.util.HashMap; import java.util.List; import java.util.Map; import javax.ws.rs.core.Response; import org.apache.commons.lang3.StringUtils; -import org.apache.logging.log4j.Logger; import org.folio.persist.HoldingsRepository; import org.folio.rest.jaxrs.model.HoldingsRecord; -import org.folio.rest.jaxrs.model.Item; import org.folio.rest.persist.PostgresClient; -import org.folio.rest.persist.SQLConnection; import org.folio.rest.support.CqlQuery; +import org.folio.rest.support.EndpointFailureHandler; import org.folio.rest.support.HridManager; +import org.folio.rest.tools.utils.OptimisticLockingUtil; import org.folio.services.domainevent.HoldingDomainEventPublisher; import org.folio.services.domainevent.ItemDomainEventPublisher; -import org.folio.services.item.ItemService; import org.folio.validator.CommonValidators; import org.folio.validator.NotesValidators; public class HoldingsService { - private static final Logger log = getLogger(HoldingsService.class); - + private static final String INSTANCE_ID = "instanceId"; private final Context vertxContext; private final Map okapiHeaders; private final PostgresClient postgresClient; private final HridManager hridManager; - private final ItemService itemService; private final HoldingsRepository holdingsRepository; private final ItemDomainEventPublisher itemEventService; private final HoldingDomainEventPublisher domainEventPublisher; @@ -55,7 +49,6 @@ public HoldingsService(Context context, Map okapiHeaders) { this.vertxContext = context; this.okapiHeaders = okapiHeaders; - itemService = new ItemService(context, okapiHeaders); postgresClient = postgresClient(context, okapiHeaders); hridManager = new HridManager(postgresClient); holdingsRepository = new HoldingsRepository(context, okapiHeaders); @@ -77,7 +70,7 @@ public Future updateHoldingRecord(String holdingId, HoldingsRecord hol return holdingsRepository.getById(holdingId) .compose(existingHoldingsRecord -> { if (holdingsRecordFound(existingHoldingsRecord)) { - return updateHolding(existingHoldingsRecord, holdingsRecord); + return updateHolding(holdingsRecord); } else { return createHolding(holdingsRecord); } @@ -134,6 +127,10 @@ public Future deleteHoldings(String cql) { } public Future createHoldings(List holdings, boolean upsert, boolean optimisticLocking) { + if (upsert) { + return upsertHoldings(holdings, optimisticLocking); + } + for (HoldingsRecord holdingsRecord : holdings) { holdingsRecord.setEffectiveLocationId(calculateEffectiveLocation(holdingsRecord)); } @@ -147,28 +144,65 @@ public Future createHoldings(List holdings, boolean up .onSuccess(domainEventPublisher.publishCreatedOrUpdated(batchOperation))); } - private Future updateHolding(HoldingsRecord oldHoldings, HoldingsRecord newHoldings) { + + public Future upsertHoldings(List holdings, boolean optimisticLocking) { + try { + for (HoldingsRecord holdingsRecord : holdings) { + holdingsRecord.setEffectiveLocationId(calculateEffectiveLocation(holdingsRecord)); + } + + if (optimisticLocking) { + OptimisticLockingUtil.unsetVersionIfMinusOne(holdings); + } else { + if (! OptimisticLockingUtil.isSuppressingOptimisticLockingAllowed()) { + return Future.succeededFuture(Response.status(413).entity( + "DB_ALLOW_SUPPRESS_OPTIMISTIC_LOCKING environment variable doesn't allow " + + "to disable optimistic locking").build()); + } + OptimisticLockingUtil.setVersionToMinusOne(holdings); + } + + return NotesValidators.refuseHoldingLongNotes(holdings) + .compose(x -> holdingsRepository.upsert(holdings)) + .onSuccess(this::publishEvents) + .map(Response.status(201).build()) + .otherwise(EndpointFailureHandler::failureResponse); + } catch (ReflectiveOperationException e) { + throw new SecurityException(e); + } + } + + private Future updateHolding(HoldingsRecord newHoldings) { newHoldings.setEffectiveLocationId(calculateEffectiveLocation(newHoldings)); if (Integer.valueOf(-1).equals(newHoldings.getVersion())) { newHoldings.setVersion(null); // enforce optimistic locking } - return refuseWhenHridChanged(oldHoldings, newHoldings) - .compose(notUsed -> NotesValidators.refuseLongNotes(newHoldings)) - .compose(notUsed -> { - final Promise> overallResult = promise(); - - postgresClient.startTx( - connection -> holdingsRepository.update(connection, oldHoldings.getId(), newHoldings) - .compose(updateRes -> itemService.updateItemsOnHoldingChanged(connection, newHoldings)) - .onComplete(handleTransaction(connection, overallResult))); + return NotesValidators.refuseLongNotes(newHoldings) + .compose(x -> holdingsRepository.upsert(List.of(newHoldings))) + .onSuccess(this::publishEvents) + .map(x -> PutHoldingsStorageHoldingsByHoldingsRecordIdResponse.respond204()); + } - return overallResult.future() - .compose(itemsBeforeUpdate -> itemEventService.publishUpdated(oldHoldings, newHoldings, itemsBeforeUpdate)) - .map(res -> PutHoldingsStorageHoldingsByHoldingsRecordIdResponse.respond204()) - .onSuccess(domainEventPublisher.publishUpdated(oldHoldings)); - }); + private void publishEvents(JsonObject holdingsItems) { + Map instanceIdByHoldingsRecordId = new HashMap<>(); + holdingsItems.getJsonArray("holdingsRecords").forEach(o -> { + var oldHolding = ((JsonObject) o).getJsonObject("old"); + var newHolding = ((JsonObject) o).getJsonObject("new"); + var instanceId = newHolding.getString(INSTANCE_ID); + var holdingId = newHolding.getString("id"); + instanceIdByHoldingsRecordId.put(holdingId, instanceId); + domainEventPublisher.publishUpserted(instanceId, oldHolding, newHolding); + }); + holdingsItems.getJsonArray("items").forEach(o -> { + var oldItem = ((JsonObject) o).getJsonObject("old"); + var newItem = ((JsonObject) o).getJsonObject("new"); + var instanceId = instanceIdByHoldingsRecordId.get(newItem.getString("holdingsRecordId")); + oldItem.put(INSTANCE_ID, instanceId); + newItem.put(INSTANCE_ID, instanceId); + itemEventService.publishUpserted(instanceId, oldItem, newItem); + }); } private String calculateEffectiveLocation(HoldingsRecord holdingsRecord) { @@ -182,31 +216,6 @@ private String calculateEffectiveLocation(HoldingsRecord holdingsRecord) { } } - private Handler> handleTransaction( - AsyncResult connection, Promise overallResult) { - - return transactionResult -> { - if (transactionResult.succeeded()) { - postgresClient.endTx(connection, commitResult -> { - if (commitResult.succeeded()) { - overallResult.complete(transactionResult.result()); - } else { - log.error("Unable to commit transaction", commitResult.cause()); - overallResult.fail(commitResult.cause()); - } - }); - } else { - log.error("Reverting transaction"); - postgresClient.rollbackTx(connection, revertResult -> { - if (revertResult.failed()) { - log.error("Unable to revert transaction", revertResult.cause()); - } - overallResult.fail(transactionResult.cause()); - }); - } - }; - } - private boolean holdingsRecordFound(HoldingsRecord holdingsRecord) { return holdingsRecord != null; } diff --git a/src/main/resources/templates/db_scripts/instance-hr-item/hridTrigger.sql b/src/main/resources/templates/db_scripts/instance-hr-item/hridTrigger.sql new file mode 100644 index 000000000..1990ba1e3 --- /dev/null +++ b/src/main/resources/templates/db_scripts/instance-hr-item/hridTrigger.sql @@ -0,0 +1,32 @@ +-- Fill in the next hrid from the sequence when the instance/holding/item +-- doesn't have a hrid. +CREATE OR REPLACE FUNCTION hrid_trigger() RETURNS TRIGGER +AS $$ +DECLARE + name TEXT; + hrid TEXT; + prefix TEXT; + zeroes BOOLEAN; +BEGIN + IF NEW.jsonb->'hrid' IS NOT NULL THEN + RETURN NEW; + END IF; + name = CASE TG_TABLE_NAME + WHEN 'instance' THEN 'instances' + WHEN 'holdings_record' THEN 'holdings' + WHEN 'item' THEN 'items' + END; + SELECT nextval('hrid_' || name || '_seq'), jsonb->name->>'prefix', jsonb->>'commonRetainLeadingZeroes' + INTO STRICT hrid, prefix, zeroes FROM hrid_settings; + IF zeroes IS TRUE THEN + hrid = repeat('0', 11 - length(hrid)) || hrid; + END IF; + NEW.jsonb = jsonb_set(NEW.jsonb, '{hrid}', to_jsonb(concat(prefix, hrid))); + RETURN NEW; +END; +$$ language 'plpgsql'; + +-- currently only the holding hrid has a trigger; instance and item hrid still use Java code + +DROP TRIGGER IF EXISTS hrid_holdings_record ON holdings_record CASCADE; +CREATE TRIGGER hrid_holdings_record BEFORE INSERT ON holdings_record FOR EACH ROW EXECUTE FUNCTION hrid_trigger(); diff --git a/src/main/resources/templates/db_scripts/instance-hr-item/write.sql b/src/main/resources/templates/db_scripts/instance-hr-item/write.sql new file mode 100644 index 000000000..7b463f5b7 --- /dev/null +++ b/src/main/resources/templates/db_scripts/instance-hr-item/write.sql @@ -0,0 +1,379 @@ +-- Input is a JSON array of the holdings records to insert. +-- Returns the JSON array of the jsonb column of the inserted holdings +-- records after triggers (_version, metadata, etc.) have been applied. +CREATE OR REPLACE FUNCTION insert_holdings(input jsonb) + RETURNS jsonb AS $$ + DECLARE + holding jsonb; + holdings jsonb = '[]'; + BEGIN + FOR holding IN + INSERT INTO holdings_record (id, jsonb) + SELECT (jsonb_array_elements(input)->>'id')::uuid, jsonb_array_elements(input) + RETURNING jsonb + LOOP + holdings = holdings || holding; + END LOOP; + RETURN holdings; + END; +$$ LANGUAGE plpgsql; + +-- Input is a JSON array of the holdings records to update or insert. +-- Returns +-- { "holdingsRecords": [{"old": {...}, "new": {...}}, ...], +-- "items": [{"old": {...}, "new": {...}}, ...] +-- } +-- providing old and new jsonb content of all updated holdings and items. For a newly +-- inserted holding only "new" is provided. +-- "new" has the content after triggers (_version, metadata, etc.) have been applied. +CREATE OR REPLACE FUNCTION upsert_holdings(input jsonb) + RETURNS jsonb AS $$ + DECLARE + holding jsonb; + holdings jsonb = '[]'; + item jsonb; + items jsonb = '[]'; + BEGIN + FOR holding IN + UPDATE holdings_record new + SET jsonb = input.jsonb || jsonb_build_object('hrid', COALESCE(input.jsonb->'hrid', old.jsonb->'hrid')) + FROM (SELECT (jsonb_array_elements(input)->>'id')::uuid id, jsonb_array_elements(input) jsonb) input, + (SELECT id, jsonb FROM holdings_record FOR UPDATE) old + WHERE new.id = input.id + AND old.id = input.id + RETURNING jsonb_build_object('old', old.jsonb, 'new', new.jsonb) + LOOP + IF holding->'new'->'hrid' <> holding->'old'->'hrid' THEN + RAISE 'Cannot change hrid of holdings record id=%, old hrid=%, new hrid=%', + holding->'old'->>'id', holding->'old'->>'hrid', holding->'new'->>'hrid' + USING ERRCODE='239HR'; + END IF; + holdings := holdings || holding; + FOR item IN + UPDATE item new + SET jsonb = + set_effective_shelving_order(old.jsonb || effective_call_number_components(holding->'new', new.jsonb)) + || jsonb_build_object('effectiveLocationId', + COALESCE(temporarylocationid::text, permanentlocationid::text, holding->'new'->>'effectiveLocationId')) + || jsonb_build_object('metadata', set_metadata(old.jsonb->'metadata', holding->'new'->'metadata')) + FROM (SELECT id, jsonb FROM item FOR UPDATE) old + WHERE old.id = new.id + AND holdingsrecordid = (holding->'new'->>'id')::uuid + AND ( (holding->'new'->'effectiveLocationId' IS DISTINCT FROM holding->'old'->'effectiveLocationId' + AND permanentlocationid IS NULL + AND temporarylocationid IS NULL) + OR (holding->'new'->'callNumber' IS DISTINCT FROM holding->'old'->'callNumber' + AND length(trim(coalesce(old.jsonb->>'itemLevelCallNumber', ''))) = 0) + OR (holding->'new'->'callNumberPrefix' IS DISTINCT FROM holding->'old'->'callNumberPrefix' + AND length(trim(coalesce(old.jsonb->>'itemLevelCallNumberPrefix', ''))) = 0) + OR (holding->'new'->'callNumberSuffix' IS DISTINCT FROM holding->'old'->'callNumberSuffix' + AND length(trim(coalesce(old.jsonb->>'itemLevelCallNumberSuffix', ''))) = 0) + OR (holding->'new'->'callNumberTypeId' IS DISTINCT FROM holding->'old'->'callNumberTypeId' + AND length(trim(coalesce(old.jsonb->>'itemLevelCallNumberTypeId', ''))) = 0) + ) + RETURNING jsonb_build_object('old', old.jsonb, 'new', new.jsonb) + LOOP + items := items || item; + END LOOP; + END LOOP; + + FOR holding IN + INSERT INTO holdings_record (id, jsonb) + SELECT (jsonb_array_elements(input)->>'id')::uuid, jsonb_array_elements(input) + ON CONFLICT DO NOTHING + RETURNING jsonb_build_object('new', jsonb) + LOOP + holdings = holdings || holding; + END LOOP; + + RETURN jsonb_build_object('holdingsRecords', holdings, 'items', items); + END; +$$ LANGUAGE plpgsql; + + +CREATE OR REPLACE FUNCTION set_metadata(item_metadata jsonb, holding_metadata jsonb) + RETURNS jsonb AS $$ + BEGIN + IF holding_metadata->'updatedDate' IS NULL THEN + item_metadata = item_metadata - 'updatedDate'; + ELSE + item_metadata = jsonb_set(item_metadata, '{updatedDate}', holding_metadata->'updatedDate'); + END IF; + IF holding_metadata->'updatedByUserId' IS NULL THEN + item_metadata = item_metadata - 'updatedByUserId'; + ELSE + item_metadata = jsonb_set(item_metadata, '{updatedByUserId}', holding_metadata->'updatedByUserId'); + END IF; + RETURN item_metadata; + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION set_item_effective_values(holding jsonb, item jsonb) + RETURNS jsonb AS $$ + BEGIN + RETURN set_effective_shelving_order( + item + || jsonb_build_object('effectiveLocationId', + COALESCE(item->>'temporaryLocationId', item->>'permanentLocationId', holding->>'effectiveLocationId')) + || effective_call_number_components(holding->'new', new.jsonb)); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION effective_call_number_components(holding jsonb, item jsonb) + RETURNS jsonb AS $$ + BEGIN + RETURN jsonb_build_object('effectiveCallNumberComponents', + effective_call_number_component('callNumber', item->>'itemLevelCallNumber', holding->>'callNumber') + || effective_call_number_component('prefix', item->>'itemLevelCallNumberPrefix', holding->>'callNumberPrefix') + || effective_call_number_component('suffix', item->>'itemLevelCallNumberSuffix', holding->>'callNumberSuffix') + || effective_call_number_component('typeId', item->>'itemLevelCallNumberTypeId', holding->>'callNumberTypeId')); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION effective_call_number_component(key text, value1 text, value2 text) + RETURNS jsonb AS $$ + BEGIN + IF length(trim(value1)) > 0 THEN + RETURN jsonb_build_object(key, value1); + END IF; + IF length(trim(value2)) > 0 THEN + RETURN jsonb_build_object(key, value2); + END IF; + RETURN jsonb_build_object(); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE; + + +CREATE OR REPLACE FUNCTION set_effective_shelving_order(item jsonb) + RETURNS jsonb AS $$ + DECLARE + call_number text; + BEGIN + call_number = trim(item->'effectiveCallNumberComponents'->>'callNumber'); + IF call_number IS NULL OR call_number = '' THEN + RETURN item - 'effectiveShelvingOrder'; + END IF; + call_number = concat_ws(' ', call_number, + trim2null(item->>'volume'), + trim2null(item->>'enumeration'), + trim2null(item->>'chronology'), + trim2null(item->>'copyNumber')); + CASE item->'effectiveCallNumberComponents'->>'typeId' + WHEN '03dd64d0-5626-4ecd-8ece-4531e0069f35' THEN call_number = dewey_call_number(call_number); + WHEN '95467209-6d7b-468b-94df-0f5d7ad2747d' THEN call_number = lc_nlm_call_number(call_number); -- LC + WHEN '054d460d-d6b9-4469-9e37-7a78a2266655' THEN call_number = lc_nlm_call_number(call_number); -- NLM + WHEN 'fc388041-6cd0-4806-8a74-ebe3b9ab4c6e' THEN call_number = su_doc_call_number(call_number); + ELSE NULL; + END CASE; + RETURN item || jsonb_build_object('effectiveShelvingOrder', + concat_ws(' ', call_number, trim2null(item->'effectiveCallNumberComponents'->>'suffix'))); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION dewey_call_number(call_number text) + RETURNS text AS $$ + DECLARE + matches text[]; + class_digits text; + class_decimal text; + cutter text; + other text; + BEGIN + matches = regexp_match(call_number, '^(\d+)(\.\d+)? *\.?(?:([A-Z]\d{1,3}(?:[A-Z]+)?) *(.*)|(.*))$'); + IF matches IS NULL THEN + RETURN NULL; + END IF; + class_digits = matches[1]; + class_decimal = matches[2]; + cutter = matches[3]; + other = numerically_sortable(trim2null(concat(trim2null(matches[4]), trim2null(matches[5])))); + RETURN concat_ws(' ', concat(sortable_number(class_digits), class_decimal), + cutter, + other + ); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION lc_nlm_call_number(call_number text) + RETURNS text AS $$ + DECLARE + matches text[]; + classification text; + classLetters text; + classDigits text; + classDecimal text; + everythingElse text; + classSuffix text; + cutter text; + BEGIN + call_number = upper(call_number); + matches = regexp_match(call_number, '^(([A-Z]+) *(?:(\d+)(\.\d+)?)?)(.*)$'); + IF matches IS NULL THEN + RETURN NULL; + END IF; + classification = trim(matches[1]); + classLetters = trim(matches[2]); + classDigits = trim(matches[3]); + classDecimal = trim(matches[4]); + everythingElse = matches[5]; + CASE substr(classLetters, 1, 1) + -- LC call numbers can't begin with I, O, W, X, or Y + -- NLM call numbers begin with W or S + WHEN 'I', 'O', 'X', 'Y' THEN + RETURN NULL; + ELSE + NULL; + END CASE; + IF classDigits IS NULL THEN + RETURN NULL; + END IF; + IF length(everythingElse) > 0 THEN + -- combining greedy and non-greedy: + -- https://www.postgresql.org/docs/current/functions-matching.html#POSIX-MATCHING-RULES + matches = regexp_match(everythingElse, '(?:(.*?)(\.?[A-Z]\d+|^\.[A-Z]| \.[A-Z])(.*)){1,1}'); + IF matches IS NULL THEN + classSuffix = trim2null(everythingElse); + ELSE + classSuffix = trim2null(matches[1]); + cutter = trim(matches[2] || matches[3]); + END IF; + END IF; + classSuffix = numerically_sortable(classSuffix); + IF substr(classSuffix, 1, 1) BETWEEN 'A' AND 'Z' THEN + classSuffix = '_' || classSuffix; + END IF; + cutter = cutter_shelf_key(cutter); + return trim(concat_ws(' ', classLetters, + concat(length(classDigits), classDigits, classDecimal), + trim(classSuffix), + trim(cutter) + )); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION su_doc_call_number(call_number text) + RETURNS text AS $$ + DECLARE + matches text[]; + BEGIN + matches = regexp_match(upper(call_number), + '^([A-Z]+)\s*(\d+)(\.(?:[A-Z]+\d*|\d+))(/(?:[A-Z]+(?:\d+(?:-\d+)?)?|\d+(?:-\d+)?))?:?(.*)$'); + IF matches IS NULL THEN + RETURN NULL; + END IF; + RETURN concat_ws(' ', matches[1], su_doc_part(matches[2]), su_doc_part(matches[3]), su_doc_part(matches[4]), su_doc_part(matches[5])); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +CREATE OR REPLACE FUNCTION su_doc_part(part text) + RETURNS text AS $$ + DECLARE + chunk text; + key text = ''; + BEGIN + IF trim(part) = '' THEN + RETURN NULL; + END IF; + IF starts_with(part, '.') OR starts_with(part, '/') OR starts_with(part, '-') OR starts_with(part, ':') THEN + part = substr(part, 2); + END IF; + FOREACH chunk IN ARRAY regexp_split_to_array(part, '[./ -]') LOOP + IF length(key) > 0 THEN + key = concat(key, ' '); + END IF; + chunk = trim(chunk); + CONTINUE WHEN chunk = ''; + IF substring(chunk, 1, 1) BETWEEN 'A' AND 'Z' THEN + key = concat(key, ' !'); + ELSIF length(chunk) >= 3 THEN + key = concat(key, '!'); + END IF; + key = concat(key, numerically_sortable(chunk)); + END LOOP; + RETURN key; + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +-- return null if trim(v) is empty, otherwise trim(v) +CREATE OR REPLACE FUNCTION trim2null(v text) + RETURNS text AS $$ + BEGIN + IF trim(v) = '' THEN + RETURN null; + END IF; + RETURN trim(v); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +-- return n with all leading 0 removed and the resulting string prepended with its length +-- 6.78 yiels 46.78, 5 yields 15, 0 yields 0 +CREATE OR REPLACE FUNCTION sortable_number(n text) + RETURNS text AS $$ + BEGIN + n = regexp_replace(n, '^0+', ''); + RETURN concat(length(n), n); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +-- A number is a sequence of digits and may contain a decimal, but not at the first position; +-- all leading zeros of a number are removed, and the length is prepended: 00123.45 becomes 6123.45 +-- A-Z is kept. +-- A sequence of one or more other characters is replaced by a single space. +CREATE OR REPLACE FUNCTION numerically_sortable(s text) + RETURNS text AS $$ + DECLARE + match text; + result text = ''; + BEGIN + FOR match IN SELECT (regexp_matches(upper(s), '[A-Z]+|[0-9][.0-9]*|[^A-Z0-9]+', 'g'))[1] LOOP + CASE + WHEN substring(match, 1, 1) BETWEEN 'A' AND 'Z' THEN + result = result || match || ' '; + WHEN substring(match, 1, 1) BETWEEN '0' AND '9' THEN + result = result || sortable_number(match); + ELSE + result = trim(result) || ' '; + END CASE; + END LOOP; + RETURN trim(result); + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; + + +-- A number is a sequence of digits and may contain a decimal, but not at the first position; +-- all leading zeros of a number are removed, and the length is prepended: 00123.45 becomes 6123.45 +-- A-Z is kept. +-- A sequence of one or more other characters is replaced by a single space. +CREATE OR REPLACE FUNCTION cutter_shelf_key(s text) + RETURNS text AS $$ + DECLARE + chunk text; + matches text[]; + cutter text; + suffix text; + result text; + BEGIN + FOREACH chunk IN ARRAY regexp_split_to_array(s, '(?=[A-Z][0-9])') LOOP + matches = regexp_match(chunk, '([A-Z][0-9]+)(.*)'); + IF matches IS NULL THEN + -- before the first cutter + result = trim2null(numerically_sortable(chunk)); + ELSE + cutter = matches[1]; + suffix = trim2null(numerically_sortable(matches[2])); + result = concat_ws(' ', result, cutter, suffix); + END IF; + END LOOP; + RETURN result; + END; +$$ LANGUAGE plpgsql IMMUTABLE PARALLEL SAFE STRICT; diff --git a/src/main/resources/templates/db_scripts/schema.json b/src/main/resources/templates/db_scripts/schema.json index f113af3dc..bdb3dc7d0 100644 --- a/src/main/resources/templates/db_scripts/schema.json +++ b/src/main/resources/templates/db_scripts/schema.json @@ -1125,6 +1125,16 @@ "run": "after", "snippetPath": "hridSettingsView.sql", "fromModuleVersion": "26.1.0" + }, + { + "run": "after", + "snippetPath": "instance-hr-item/write.sql", + "fromModuleVersion": "26.1.0" + }, + { + "run": "after", + "snippetPath": "instance-hr-item/hridTrigger.sql", + "fromModuleVersion": "26.1.0" } ] } diff --git a/src/test/java/org/folio/persist/HoldingsRepositoryTest.java b/src/test/java/org/folio/persist/HoldingsRepositoryTest.java new file mode 100644 index 000000000..3c667ed77 --- /dev/null +++ b/src/test/java/org/folio/persist/HoldingsRepositoryTest.java @@ -0,0 +1,45 @@ +package org.folio.persist; + +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +import io.vertx.core.Vertx; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import java.util.List; +import java.util.Map; +import org.folio.cql2pgjson.exception.QueryValidationException; +import org.folio.rest.jaxrs.model.HoldingsRecord; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(VertxExtension.class) +class HoldingsRepositoryTest { + + private static HoldingsRepository holdingsRepository = + new HoldingsRepository(Vertx.vertx().getOrCreateContext(), Map.of()); + private static HoldingsRecord holdingThrowingFoo = new HoldingsRecord() { + public String getId() { + throw new IllegalCallerException("foo"); + } + }; + + @Test + void upsertException(VertxTestContext vtc) { + holdingsRepository.upsert(List.of(holdingThrowingFoo)) + .onComplete(vtc.failing(e -> { + assertThat(e.getCause().getMessage(), is("foo")); + vtc.completeNow(); + })); + } + + @Test + void deleteException(VertxTestContext vtc) { + holdingsRepository.delete(")") + .onComplete(vtc.failing(e -> { + assertThat(e.getCause(), instanceOf(QueryValidationException.class)); + vtc.completeNow(); + })); + } +} diff --git a/src/test/java/org/folio/rest/api/HoldingsStorageTest.java b/src/test/java/org/folio/rest/api/HoldingsStorageTest.java index c64751d1f..52766603e 100644 --- a/src/test/java/org/folio/rest/api/HoldingsStorageTest.java +++ b/src/test/java/org/folio/rest/api/HoldingsStorageTest.java @@ -13,6 +13,7 @@ import static org.folio.rest.support.http.InterfaceUrls.holdingsStorageSyncUnsafeUrl; import static org.folio.rest.support.http.InterfaceUrls.holdingsStorageSyncUrl; import static org.folio.rest.support.http.InterfaceUrls.holdingsStorageUrl; +import static org.folio.rest.support.http.InterfaceUrls.itemsStorageSyncUrl; import static org.folio.rest.support.http.InterfaceUrls.itemsStorageUrl; import static org.folio.rest.support.matchers.PostgresErrorMessageMatchers.isMaximumSequenceValueError; import static org.folio.utility.ModuleUtility.getClient; @@ -41,6 +42,7 @@ import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; @@ -382,10 +384,7 @@ public void canMoveHoldingsToNewInstance() throws ExecutionException, Interrupte holdingsMessageChecks.updatedMessagePublished(holdingResource.getJson(), holdingFromGet); - JsonObject newItem = item.copy() - .put("_version", 2); - - itemMessageChecks.updatedMessagePublished(item, newItem, instanceId.toString()); + // item doesn't change when holding moves } @Test @@ -1541,7 +1540,6 @@ public void updatingHoldingsDoesNotUpdateItemsOnAnotherHoldings() firstHolding.put("callNumberSuffix", "updatedFirstCallNumberSuffix"); Response putResponse = update(firstHoldingsUrl, firstHolding); - Response updatedFirstHoldingResponse = get(firstHoldingsUrl); JsonObject updatedFirstHolding = updatedFirstHoldingResponse.getJson(); @@ -2251,7 +2249,8 @@ public void cannotChangeHridAfterCreation() assertThat(response.getStatusCode(), is(HttpURLConnection.HTTP_BAD_REQUEST)); assertThat(response.getBody(), - is("The hrid field cannot be changed: new=ABC123, old=ho00000000001")); + is("ERROR: Cannot change hrid of holdings record id=" + holdingsId + + ", old hrid=ho00000000001, new hrid=ABC123 (239HR)")); log.info("Finished cannotChangeHRIDAfterCreation"); } @@ -2280,16 +2279,11 @@ public void cannotRemoveHridAfterCreation() holdings.remove("hrid"); - final CompletableFuture updateCompleted = new CompletableFuture<>(); - - getClient().put(holdingsStorageUrl(String.format("/%s", holdingsId)), holdings, TENANT_ID, - text(updateCompleted)); - - final Response response = updateCompleted.get(10, TimeUnit.SECONDS); + getClient().put(holdingsStorageUrl(String.format("/%s", holdingsId)), holdings, TENANT_ID) + .get(10, TimeUnit.SECONDS); - assertThat(response.getStatusCode(), is(HttpURLConnection.HTTP_BAD_REQUEST)); - assertThat(response.getBody(), - is("The hrid field cannot be changed: new=null, old=ho00000000001")); + final Response get = getById(holdingsId.toString()); + assertThat(get.getJson().getString("hrid"), is("ho00000000001")); log.info("Finished cannotRemoveHRIDAfterCreation"); } @@ -2613,6 +2607,51 @@ public void canPostSynchronousBatchWithExistingIdWithUpsertTrue() { holdingsMessageChecks.updatedMessagePublished(holdingsBeforeUpdate, holdingsAfterUpdate); } + @Test + public void canPostSynchronousBatchWithItemCallNumber() { + final JsonArray holdings = threeHoldings(); + final JsonArray items = sixItems(holdings); + for (int i = 0; i < 3; i++) { + if (i == 0 || i == 2) { + holdings.getJsonObject(i) + .put("callNumberPrefix", "hcnp") + .put("callNumber", "hcn") + .put("callNumberSuffix", "hcns"); + } + } + for (int i = 0; i < 6; i++) { + if (i == 1 || i == 2 || i == 3 || i == 5) { + items.getJsonObject(i) + .put("itemLevelCallNumberPrefix", "icnp") + .put("itemLevelCallNumber", "icn") + .put("itemLevelCallNumberSuffix", "icns"); + } + } + final Response response1 = postSynchronousBatch("?upsert=true", holdings); + assertThat(response1, statusCodeIs(HTTP_CREATED)); + final Response itemResponse = postItemSynchronousBatch("?upsert=true", items); + assertThat(itemResponse, statusCodeIs(HTTP_CREATED)); + + assertItemEffectiveCallNumbers(items, "prefix", "hcnp", "icnp", "icnp", "icnp", "hcnp", "icnp"); + assertItemEffectiveCallNumbers(items, "callNumber", "hcn", "icn", "icn", "icn", "hcn", "icn"); + assertItemEffectiveCallNumbers(items, "suffix", "hcns", "icns", "icns", "icns", "hcns", "icns"); + + for (int i = 0; i < 3; i++) { + if (i == 0 || i == 1) { + holdings.getJsonObject(i) + .put("callNumberPrefix", "xcnp") + .put("callNumber", "xcn") + .put("callNumberSuffix", "xcns"); + } + } + final Response response2 = postSynchronousBatch("?upsert=true", holdings); + assertThat(response2, statusCodeIs(HTTP_CREATED)); + + assertItemEffectiveCallNumbers(items, "prefix", "xcnp", "icnp", "icnp", "icnp", "hcnp", "icnp"); + assertItemEffectiveCallNumbers(items, "callNumber", "xcn", "icn", "icn", "icn", "hcn", "icn"); + assertItemEffectiveCallNumbers(items, "suffix", "xcns", "icns", "icns", "icns", "hcns", "icns"); + } + @Test public void canSearchByDiscoverySuppressProperty() { final IndividualResource instance = instancesClient @@ -2865,6 +2904,23 @@ private JsonArray threeHoldings() { return holdingsArray; } + /** + * Return six item JsonObject, two for each of the three holdings. The items are not created. + */ + private JsonArray sixItems(JsonArray holdings) { + JsonArray items = new JsonArray(); + for (int i = 0; i < 6; i++) { + items.add(new JsonObject() + .put("id", UUID.randomUUID().toString()) + .put("holdingsRecordId", holdings.getJsonObject(i / 2).getString("id")) + .put("_version", 1) + .put("materialTypeId", bookMaterialTypeID) + .put("permanentLoanTypeId", canCirculateLoanTypeID) + .put("status", new JsonObject().put("name", "Available"))); + } + return items; + } + private Response postSynchronousBatchUnsafe(JsonArray holdingsArray) { return postSynchronousBatch(holdingsStorageSyncUnsafeUrl(""), holdingsArray); } @@ -2888,6 +2944,15 @@ private Response postSynchronousBatch(URL url, JsonArray holdingsArray) { } } + private Response postItemSynchronousBatch(String subPath, JsonArray itemArray) { + try { + JsonObject itemCollection = new JsonObject().put("items", itemArray); + return getClient().post(itemsStorageSyncUrl(subPath), itemCollection, TENANT_ID).get(10, SECONDS); + } catch (InterruptedException | ExecutionException | TimeoutException e) { + throw new RuntimeException(e); + } + } + private JsonObject smallAngryPlanet(UUID id) { JsonArray identifiers = new JsonArray(); identifiers.add(identifier(UUID_ISBN, "9781473619777")); @@ -2942,6 +3007,35 @@ private Response getById(String id) { } } + /** + * For each passed item use the id and fetch the item record from database. + */ + private List fetchItems(JsonArray items) { + try { + var result = new ArrayList(); + for (int i = 0; i < items.size(); i++) { + var id = items.getJsonObject(i).getString("id"); + result.add(getClient().get(itemsStorageUrl("/" + id), TENANT_ID).get(5, SECONDS).getJson()); + } + return result; + } catch (InterruptedException | ExecutionException | TimeoutException e) { + throw new RuntimeException(e); + } + } + + /** + * Assert that the effective call number property in the database equals the expected n1 ... n6. + * Fetch the item using the id from items. + */ + private void assertItemEffectiveCallNumbers(JsonArray items, String property, + String n1, String n2, String n3, String n4, String n5, String n6) { + + var actual = fetchItems(items).stream() + .map(item -> item.getJsonObject("effectiveCallNumberComponents").getString(property)) + .collect(Collectors.toList()); + assertThat(actual, is(List.of(n1, n2, n3, n4, n5, n6))); + } + private void assertExists(JsonObject expectedHolding) { Response response = getById(expectedHolding.getString("id")); assertThat(response, statusCodeIs(HttpStatus.HTTP_OK)); diff --git a/src/test/java/org/folio/rest/api/ItemEffectiveCallNumberComponentsTest.java b/src/test/java/org/folio/rest/api/ItemEffectiveCallNumberComponentsTest.java index b76f5d067..80bd1fe4d 100644 --- a/src/test/java/org/folio/rest/api/ItemEffectiveCallNumberComponentsTest.java +++ b/src/test/java/org/folio/rest/api/ItemEffectiveCallNumberComponentsTest.java @@ -221,7 +221,10 @@ public void canCalculateEffectiveCallNumberPropertyOnUpdate( final String effectivePropertyName = callNumberProperties.effectivePropertyName; final String initEffectiveValue = StringUtils.firstNonBlank(itemInitValue, holdingsInitValue); - final String targetEffectiveValue = StringUtils.firstNonBlank(itemTargetValue, holdingsTargetValue); + /** expected value after holdings update */ + final String targetEffectiveValue1 = StringUtils.firstNonBlank(itemInitValue, holdingsTargetValue); + /** expected value after holdings and item update */ + final String targetEffectiveValue2 = StringUtils.firstNonBlank(itemTargetValue, holdingsTargetValue); IndividualResource holdings = createHoldingsWithPropertySetAndInstance( holdingsPropertyName, holdingsInitValue @@ -246,7 +249,9 @@ public void canCalculateEffectiveCallNumberPropertyOnUpdate( var itemAfterHoldingsUpdate = getById(createdItem.getJson()); - itemMessageChecks.updatedMessagePublished(createdItem.getJson(), itemAfterHoldingsUpdate); + if (! Objects.equals(initEffectiveValue, targetEffectiveValue1)) { + itemMessageChecks.updatedMessagePublished(createdItem.getJson(), itemAfterHoldingsUpdate); + } if (!Objects.equals(itemInitValue, itemTargetValue)) { itemsClient.replace(createdItem.getId(), itemAfterHoldingsUpdate.copy() @@ -262,7 +267,7 @@ public void canCalculateEffectiveCallNumberPropertyOnUpdate( assertNotNull(updatedEffectiveCallNumberComponents); assertThat(updatedEffectiveCallNumberComponents.getString(effectivePropertyName), - is(targetEffectiveValue)); + is(targetEffectiveValue2)); } private IndividualResource createHoldingsWithPropertySetAndInstance( diff --git a/src/test/java/org/folio/rest/api/ItemEffectiveLocationTest.java b/src/test/java/org/folio/rest/api/ItemEffectiveLocationTest.java index 661237985..6782aa40f 100644 --- a/src/test/java/org/folio/rest/api/ItemEffectiveLocationTest.java +++ b/src/test/java/org/folio/rest/api/ItemEffectiveLocationTest.java @@ -209,7 +209,10 @@ public void canCalculateEffectiveLocationHoldingUpdate( assertThat(associatedItem.getString(EFFECTIVE_LOCATION_ID_KEY), is(effectiveLocation(holdingEndLoc, itemLoc))); - itemMessageChecks.updatedMessagePublished(createdItem, associatedItem); + if (! associatedItem.getString(EFFECTIVE_LOCATION_ID_KEY) + .equals(createdItem.getString(EFFECTIVE_LOCATION_ID_KEY))) { + itemMessageChecks.updatedMessagePublished(createdItem, associatedItem); + } holdingsMessageChecks.updatedMessagePublished(createdHolding, holdingsClient.getById(holdingsRecordId).getJson()); diff --git a/src/test/java/org/folio/rest/impl/WriteSqlTest.java b/src/test/java/org/folio/rest/impl/WriteSqlTest.java new file mode 100644 index 000000000..80aefaf8e --- /dev/null +++ b/src/test/java/org/folio/rest/impl/WriteSqlTest.java @@ -0,0 +1,226 @@ +package org.folio.rest.impl; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; + +import io.vertx.core.Future; +import io.vertx.core.Vertx; +import io.vertx.core.json.JsonObject; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import io.vertx.sqlclient.Tuple; +import org.folio.postgres.testing.PostgresTesterContainer; +import org.folio.rest.persist.PostgresClient; +import org.folio.util.ResourceUtil; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.marc4j.callnum.DeweyCallNumber; +import org.marc4j.callnum.LCCallNumber; +import org.marc4j.callnum.NlmCallNumber; + +@ExtendWith(VertxExtension.class) +class WriteSqlTest { + private static Vertx vertx; + + @BeforeAll + static void beforeAll(Vertx vertx, VertxTestContext vtc) { + WriteSqlTest.vertx = vertx; + PostgresClient.setPostgresTester(new PostgresTesterContainer()); + var sql = "SET search_path TO 'public';\n" + + ResourceUtil.asString("/templates/db_scripts/instance-hr-item/write.sql"); + runSql(sql) + .onComplete(vtc.succeedingThenComplete()); + } + + static Future runSql(String sql) { + return PostgresClient.getInstance(vertx).runSQLFile(sql, true) + .compose(errors -> { + if (errors.isEmpty()) { + return Future.succeededFuture(); + } + return Future.failedFuture(errors.get(0)); + }); + } + + @AfterAll + static void afterAll() { + PostgresClient.stopPostgresTester(); + } + + @ParameterizedTest + @CsvSource({ + // callNumber, volume, enumeration, chronology, copyNumber, expected + "cn, v, e, ch, co, 'cn v e ch co'", + "' cn ', ' v ', ' e ', ' ch ', ' co ', 'cn v e ch co'", + "cn, v, , ch, , 'cn v ch'", + "cn, , e, , co, 'cn e co'", + ", v, e, ch, co, ", + "' ', v, e, ch, co, ", + }) + void setEffectiveShelvingOrder(String callNumber, String volume, String enumeration, String chronology, + String copyNumber, String expected, VertxTestContext vtc) { + + var jsonObject = new JsonObject().put("effectiveShelvingOrder", "old"); + if (callNumber != null) { + jsonObject.put("effectiveCallNumberComponents", new JsonObject().put("callNumber", callNumber)); + } + if (volume != null) { + jsonObject.put("volume", volume); + } + if (enumeration != null) { + jsonObject.put("enumeration", enumeration); + } + if (chronology != null) { + jsonObject.put("chronology", chronology); + } + if (copyNumber != null) { + jsonObject.put("copyNumber", copyNumber); + } + PostgresClient.getInstance(vertx) + .selectSingle("SELECT public.set_effective_shelving_order($1)", Tuple.of(jsonObject)) + .onComplete(vtc.succeeding(r -> { + assertThat(r.getJsonObject(0).getString("effectiveShelvingOrder"), is(expected)); + vtc.completeNow(); + })); + } + + @ParameterizedTest + @ValueSource(strings = { + // valid LC call number + "A1 B2 .C33", + "A1 B2 C33", + "A1 B2.C33", + "A1 B2C33", + "AB9 L3", + "BF199", + "BF199.", + "BF199.A1J7", + "G3841 .C2 1935 .M3", + "HC241.25 .I4 D47", + "HD 38.25.F8 R87 1989", + "HD38.25.F8 R87 1989", + "HE5.215 .N9/PT.A", + "HF 5549.5.T7 B294 1992", + "LD6329 1903 35TH", + "LD6353 1886", + "M1 .L33", + "M1 L33", + "M5 .L", + "M5 L3 1902", + "M5 L3 1902 V.2", + "M5 L3 1902 V2", + "M5 .L3 1902 V2 TANEYTOWN", + "M211 .M93 BMW240", + "M211 .M93 K.240", + "M211 .M93 K.240 1988 .A1", + "M211 .M93 K.240 1988 A1", + "M453 .Z29 Q1 L V.2", + "M857 .K93 H2 OP.79", + "M857 .M93 S412B M", + "M1001 .H", + "M1001 .M9 1900Z", + "M1001 .M9 K.173D B", + "M1001 .M9 K.551 1900Z M", + "M1001 .M939 S.3,13 2001", + "ML410 .M8 L25 .M95 1995", + "ML410 .M8 L25 M95 1995", + "ML410 .M9 P29 1941 M", + "MT37 2003M384", + "MT130 .M93 K96 .W83 1988", + "MT130 .M93 K96 W83 1988", + "PQ2678.K26 P54", + "PQ8550.21.R57 V5 1992", + "PR92 .L33 1990", + "PR919 .L33 1990", + "PR9199 .A39", + "PR9199.48 .B3", + "PS153 .G38 B73 2012", + "QA76", + "M1", + "BF1999.A63880 1978", + "BF1999 Aarons", + "bq1270", + "l666 15th A8", + // valid NLM (National Library of Medicine) call number + "QZ1 B2 .C33", + // "W1 B2 .C33", // bug in marc4j: https://github.com/marc4j/marc4j/pull/97 + "WR1 B2 .C33", + // invalid: + "", + "I1 B2 .C33", + "O1 B2 .C33", + "Q 11 .GA1 E53 2005", + "QSS 11 .GA1 E53 2005", + "WAA 102.5 B5315 2018", + "X1 B2 .C33", + "Y1 B2 .C33", + "Sony PDX10", + "RCA Jz(1)", + "XXKD671.G53 2012", + }) + void lcNlmCallNumber(String s, VertxTestContext vtc) { + PostgresClient.getInstance(vertx).selectSingle("SELECT public.lc_nlm_call_number($1)", Tuple.of(s)) + .onComplete(vtc.succeeding(r -> { + var nlm = new NlmCallNumber(s); + var lc = new LCCallNumber(s); + if (nlm.isValid()) { + var expected = nlm.getShelfKey(); + assertThat(s + " -> " + expected + " (NLM)", r.getString(0), is(expected)); + } else if (lc.isValid()) { + var expected = lc.getShelfKey(); + assertThat(s + " -> " + expected + " (LC)", r.getString(0), is(expected)); + } else { + assertThat(s + " -> null (for invalid)", r.getString(0), is(nullValue())); + } + vtc.completeNow(); + })); + } + + @ParameterizedTest + @ValueSource(strings = { + "1 .I39", + "1.23 .I39", + "11 .I39", + "11.34 .I39", + "11.34567 .I39", + "111 .I39", + "111 I39", + "111Q39", + "111.12 .I39", + "111.123 I39", + "111.134Q39", + "322.44 .F816 V.1 1974", + "322.45 .R513 1957", + "323 .A512RE NO.23-28", + "323 .A778 ED.2", + "323.09 .K43 V.1", + "324.54 .I39 F", + "324.548 .C425R", + "324.6 .A75CUA", + "341.7/58 / 21", + "394.1 O41b", + "9A2 C0444218 Music CD", + // invalid: + "", + "MC1 259", + "T1 105", + }) + void deweyCallNumber(String s, VertxTestContext vtc) { + PostgresClient.getInstance(vertx).selectSingle("SELECT public.dewey_call_number($1)", Tuple.of(s)) + .onComplete(vtc.succeeding(r -> { + var d = new DeweyCallNumber(s); + if (d.isValid()) { + var expected = d.getShelfKey(); + assertThat(s + " -> " + expected, r.getString(0), is(expected)); + } else { + assertThat(s + " -> null (for invalid)", r.getString(0), is(nullValue())); + } + vtc.completeNow(); + })); + } +} diff --git a/src/test/java/org/folio/rest/support/kafka/GroupedCollectedMessages.java b/src/test/java/org/folio/rest/support/kafka/GroupedCollectedMessages.java index 1be4e2d27..e2ee65b1a 100644 --- a/src/test/java/org/folio/rest/support/kafka/GroupedCollectedMessages.java +++ b/src/test/java/org/folio/rest/support/kafka/GroupedCollectedMessages.java @@ -23,7 +23,8 @@ void add(String key, EventMessage eventMessage) { } List messagesByGroupKey(String key) { - return collectedMessages.getOrDefault(key, emptyList()); + // copyOf avoids sporadic ConcurrentModificationException + return List.copyOf(collectedMessages.getOrDefault(key, emptyList())); } int groupCount() { diff --git a/src/test/java/org/folio/rest/support/messages/ItemEventMessageChecks.java b/src/test/java/org/folio/rest/support/messages/ItemEventMessageChecks.java index 1816b8887..0435f2318 100644 --- a/src/test/java/org/folio/rest/support/messages/ItemEventMessageChecks.java +++ b/src/test/java/org/folio/rest/support/messages/ItemEventMessageChecks.java @@ -63,7 +63,6 @@ public void updatedMessagePublished(JsonObject oldItem, final var itemId = getId(newItem); final var newInstanceId = getInstanceIdForItem(newItem); - awaitAtMost().until(() -> kafkaConsumer.getMessagesForItem(newInstanceId, itemId), EVENT_MESSAGE_MATCHERS.hasUpdateEventMessageFor( addInstanceIdToItem(oldItem, oldInstanceId), diff --git a/src/test/java/org/folio/services/CallNumberUtilsTest.java b/src/test/java/org/folio/services/CallNumberUtilsTest.java index 31de7a461..e846b972e 100644 --- a/src/test/java/org/folio/services/CallNumberUtilsTest.java +++ b/src/test/java/org/folio/services/CallNumberUtilsTest.java @@ -8,17 +8,55 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import io.vertx.core.Future; +import io.vertx.core.Vertx; +import io.vertx.core.json.JsonObject; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import io.vertx.sqlclient.Tuple; import java.util.Arrays; import java.util.Collections; import java.util.Random; +import org.folio.postgres.testing.PostgresTesterContainer; import org.folio.rest.jaxrs.model.HoldingsRecord; import org.folio.rest.jaxrs.model.Item; +import org.folio.rest.persist.PostgresClient; import org.folio.rest.support.EffectiveCallNumberComponentsUtil; +import org.folio.util.ResourceUtil; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +@ExtendWith(VertxExtension.class) public class CallNumberUtilsTest { + private static Vertx vertx; + + @BeforeAll + static void beforeAll(Vertx vertx, VertxTestContext vtc) { + CallNumberUtilsTest.vertx = vertx; + PostgresClient.setPostgresTester(new PostgresTesterContainer()); + var sql = "SET search_path TO 'public';\n" + + ResourceUtil.asString("/templates/db_scripts/instance-hr-item/write.sql"); + runSql(sql).onComplete(vtc.succeedingThenComplete()); + } + + static Future runSql(String sql) { + return PostgresClient.getInstance(vertx).runSQLFile(sql, true) + .compose(errors -> { + if (errors.isEmpty()) { + return Future.succeededFuture(); + } + return Future.failedFuture(errors.get(0)); + }); + } + + @AfterAll + static void afterAll() { + PostgresClient.stopPostgresTester(); + } @ParameterizedTest @CsvSource({ @@ -68,7 +106,8 @@ void inputForShelvingNumber( String chronology, String copy, String suffix, - String typeId + String typeId, + VertxTestContext vtc ) { Item item = new Item(); @@ -83,9 +122,15 @@ void inputForShelvingNumber( HoldingsRecord holdingsRecord = new HoldingsRecord(); EffectiveCallNumberComponentsUtil.setCallNumberComponents(item, holdingsRecord); - EffectiveCallNumberComponentsUtil.calculateAndSetEffectiveShelvingOrder(item); - assertThat(item.getEffectiveShelvingOrder(), is(desiredShelvingOrder)); + PostgresClient.getInstance(vertx) + .selectSingle("SELECT public.set_effective_shelving_order($1)", Tuple.of(JsonObject.mapFrom(item))) + .onComplete(vtc.succeeding(r -> { + EffectiveCallNumberComponentsUtil.calculateAndSetEffectiveShelvingOrder(item); + assertThat(item.getEffectiveShelvingOrder(), is(desiredShelvingOrder)); + assertThat(r.getJsonObject(0).getString("effectiveShelvingOrder"), is(desiredShelvingOrder)); + vtc.completeNow(); + })); } @Test @@ -138,9 +183,17 @@ void testSuDocSortingOrder() { }) void checkSuDocShelvingKey( String callNumber, - String expectedShelvingKey + String expectedShelvingKey, + VertxTestContext vtc ) { var shelvingKey = new SuDocCallNumber(callNumber).getShelfKey(); assertEquals(expectedShelvingKey, shelvingKey); + + PostgresClient.getInstance(vertx) + .selectSingle("SELECT public.su_doc_call_number($1)", Tuple.of(callNumber)) + .onComplete(vtc.succeeding(r -> { + assertThat(r.getString(0), is(expectedShelvingKey)); + vtc.completeNow(); + })); } } From a54fab923a62086f5240292211434ab023a0451f Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Mon, 11 Sep 2023 13:19:11 +0200 Subject: [PATCH 2/2] MODINVSTOR-400: Add missing item field descriptions for holdingsRecord2 Mention that it is a fake property for mod-graphql --- ramls/item.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ramls/item.json b/ramls/item.json index 5ccef8c44..afe550fe4 100644 --- a/ramls/item.json +++ b/ramls/item.json @@ -401,7 +401,7 @@ }, "holdingsRecord2": { "type": "object", - "description": "Item associated holdings record object.", + "description": "Fake property for mod-graphql to determine record relationships.", "folio:$ref": "holdingsrecord.json", "readonly": true, "folio:isVirtual": true,