From 59ff6dbbf8cdc528bc44e466f866c53c6a6c3574 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Wed, 19 Jun 2024 17:33:57 +0300 Subject: [PATCH 01/13] CIRC-2111 Initial implementation --- descriptors/ModuleDescriptor-template.json | 80 +++++++++++++ ramls/circulation-setting.json | 33 ++++++ ramls/circulation-settings.json | 23 ++++ ramls/circulation-settings.raml | 105 +++++++++++++++++ ramls/examples/circulation-setting.json | 7 ++ ramls/examples/circulation-settings.json | 12 ++ .../circulation/CirculationVerticle.java | 2 + .../domain/CirculationSetting.java | 32 +++++ .../CirculationSettingsRepository.java | 76 ++++++++++++ .../CirculationSettingsResource.java | 109 ++++++++++++++++++ .../folio/circulation/support/Clients.java | 13 +++ 11 files changed, 492 insertions(+) create mode 100644 ramls/circulation-setting.json create mode 100644 ramls/circulation-settings.json create mode 100644 ramls/circulation-settings.raml create mode 100644 ramls/examples/circulation-setting.json create mode 100644 ramls/examples/circulation-settings.json create mode 100644 src/main/java/org/folio/circulation/domain/CirculationSetting.java create mode 100644 src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java create mode 100644 src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json index b41b0ffe80..e127e3294a 100644 --- a/descriptors/ModuleDescriptor-template.json +++ b/descriptors/ModuleDescriptor-template.json @@ -682,6 +682,61 @@ } ] }, + { + "id": "circulation-settings", + "version": "1.0", + "handlers": [ + { + "methods": [ + "GET" + ], + "pathPattern": "/circulation/settings", + "permissionsRequired": [ + "circulation.settings.collection.get" + ], + "modulePermissions": [ + "circulation-storage.circulation-settings.collection.get" + ] + }, + { + "methods": ["GET"], + "pathPattern": "/circulation/settings/{id}", + "permissionsRequired": [ + "circulation.settings.item.get" + ], + "modulePermissions": [ + "circulation-storage.circulation-settings.item.get" + ] + }, { + "methods": ["PUT"], + "pathPattern": "/circulation/settings/{id}", + "permissionsRequired": [ + "circulation.settings.item.put" + ], + "modulePermissions": [ + "circulation-storage.circulation-settings.item.put" + ] + }, { + "methods": ["POST"], + "pathPattern": "/circulation/settings", + "permissionsRequired": [ + "circulation.settings.item.post" + ], + "modulePermissions": [ + "circulation-storage.circulation-settings.item.post" + ] + }, { + "methods": ["DELETE"], + "pathPattern": "/circulation/settings/{id}", + "permissionsRequired": [ + "circulation.settings.item.delete" + ], + "modulePermissions": [ + "circulation-storage.circulation-settings.item.delete" + ] + } + ] + }, { "id": "_timer", "version": "1.0", @@ -1518,6 +1573,31 @@ "displayName": "circulation settings - Read configuration", "description": "To read the configuration from mod settings." }, + { + "permissionName": "circulation.settings.collection.get", + "displayName": "circulation - get circulation settings", + "description": "get a collection of circulation settings" + }, + { + "permissionName": "circulation.settings.item.get", + "displayName": "circulation - get an individual circulation setting", + "description": "get an individual circulation setting by ID" + }, + { + "permissionName": "circulation.settings.item.put", + "displayName": "circulation - update circulation setting", + "description": "update circulation setting by ID" + }, + { + "permissionName": "circulation.settings.item.post", + "displayName": "circulation - create circulation setting", + "description": "create a new circulation setting" + }, + { + "permissionName": "circulation.settings.item.delete", + "displayName": "circulation - delete circulation setting", + "description": "delete circulation setting by ID" + }, { "permissionName": "circulation.all", "displayName": "circulation - all permissions", diff --git a/ramls/circulation-setting.json b/ramls/circulation-setting.json new file mode 100644 index 0000000000..0907442e8b --- /dev/null +++ b/ramls/circulation-setting.json @@ -0,0 +1,33 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "title": "Circulation Setting Schema", + "description": "Circulation setting", + "type": "object", + "properties": { + "id": { + "description": "ID of the circulation setting", + "type": "string", + "$ref": "raml-util/schemas/uuid.schema" + }, + "name": { + "description": "Circulation setting name", + "type": "string" + }, + "value": { + "description": "Circulation setting", + "type": "object", + "additionalProperties": true + }, + "metadata": { + "description": "Metadata about creation and changes, provided by the server (client should not provide)", + "type": "object", + "$ref": "raml-util/schemas/metadata.schema" + } + }, + "additionalProperties": false, + "required": [ + "id", + "name", + "value" + ] +} diff --git a/ramls/circulation-settings.json b/ramls/circulation-settings.json new file mode 100644 index 0000000000..99173df7ba --- /dev/null +++ b/ramls/circulation-settings.json @@ -0,0 +1,23 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Collection of Circulation settings", + "type": "object", + "properties": { + "circulationSettings": { + "description": "List of circulation settings", + "id": "circulationSettings", + "type": "array", + "items": { + "type": "object", + "$ref": "circulation-setting.json" + } + }, + "totalRecords": { + "type": "integer" + } + }, + "required": [ + "circulationSettings", + "totalRecords" + ] +} diff --git a/ramls/circulation-settings.raml b/ramls/circulation-settings.raml new file mode 100644 index 0000000000..9af4023eac --- /dev/null +++ b/ramls/circulation-settings.raml @@ -0,0 +1,105 @@ +#%RAML 1.0 +title: Circulation Settings +version: v1.0 +protocols: [ HTTP, HTTPS ] +baseUri: http://localhost:9130 + +documentation: + - title: Circulation Settings API + content: API for circulation settings + +traits: + language: !include raml-util/traits/language.raml + pageable: !include raml-util/traits/pageable.raml + searchable: !include raml-util/traits/searchable.raml + validate: !include raml-util/traits/validation.raml + +types: + circulation-setting: !include circulation-setting.json + circulation-settings: !include circulation-settings.json + errors: !include raml-util/schemas/errors.schema + parameters: !include raml-util/schemas/parameters.schema + +resourceTypes: + collection: !include raml-util/rtypes/collection.raml + collection-item: !include raml-util/rtypes/item-collection.raml + +/circulation/settings: + type: + collection: + exampleCollection: !include examples/circulation-settings.json + exampleItem: !include examples/circulation-setting.json + schemaCollection: circulation-settings + schemaItem: circulation-setting + post: + is: [validate] + description: Create a new circulation setting + body: + application/json: + type: circulation-setting + responses: + 201: + description: "Circulation setting has been created" + body: + application/json: + type: circulation-setting + 500: + description: "Internal server error" + body: + text/plain: + example: "Internal server error" + get: + is: [validate, pageable, searchable: { description: "with valid searchable fields", example: "id=497f6eca-6276-4993-bfeb-98cbbbba8f79" }] + description: Get all circulation settings + responses: + 200: + description: "Circulation settings successfully retreived" + body: + application/json: + type: circulation-settings + 500: + description: "Internal server error" + body: + text/plain: + example: "Internal server error" + /{circulationSettingId}: + type: + collection-item: + exampleItem: !include examples/circulation-setting.json + schema: circulation-setting + get: + responses: + 200: + description: "Circulation setting successfully retreived" + body: + application/json: + type: circulation-setting + 500: + description: "Internal server error" + body: + text/plain: + example: "Internal server error" + put: + is: [ validate ] + body: + application/json: + type: circulation-setting + responses: + 204: + description: "Circulation settings have been saved." + 500: + description: "Internal server error" + body: + text/plain: + example: "Internal server error" + delete: + is: [ validate ] + responses: + 204: + description: "Circulation settings deleted" + 500: + description: "Internal server error" + body: + text/plain: + example: "Internal server error" + diff --git a/ramls/examples/circulation-setting.json b/ramls/examples/circulation-setting.json new file mode 100644 index 0000000000..35f0aba430 --- /dev/null +++ b/ramls/examples/circulation-setting.json @@ -0,0 +1,7 @@ +{ + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f09", + "name": "Sample settings", + "value": { + "org.folio.circulation.settings": "true" + } +} diff --git a/ramls/examples/circulation-settings.json b/ramls/examples/circulation-settings.json new file mode 100644 index 0000000000..b9b7a9c8b2 --- /dev/null +++ b/ramls/examples/circulation-settings.json @@ -0,0 +1,12 @@ +{ + "circulationSettings": [ + { + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f09", + "name": "Sample settings", + "value": { + "org.folio.circulation.settings": "true" + } + } + ], + "totalRecords": 1 +} diff --git a/src/main/java/org/folio/circulation/CirculationVerticle.java b/src/main/java/org/folio/circulation/CirculationVerticle.java index cd6960808b..f0d2371440 100644 --- a/src/main/java/org/folio/circulation/CirculationVerticle.java +++ b/src/main/java/org/folio/circulation/CirculationVerticle.java @@ -10,6 +10,7 @@ import org.folio.circulation.resources.CheckInByBarcodeResource; import org.folio.circulation.resources.CheckOutByBarcodeResource; import org.folio.circulation.resources.CirculationRulesResource; +import org.folio.circulation.resources.CirculationSettingsResource; import org.folio.circulation.resources.ClaimItemReturnedResource; import org.folio.circulation.resources.DeclareClaimedReturnedItemAsMissingResource; import org.folio.circulation.resources.DeclareLostResource; @@ -150,6 +151,7 @@ public void start(Promise startFuture) { // Handlers new LoanRelatedFeeFineClosedHandlerResource(client).register(router); new FeeFineBalanceChangedHandlerResource(client).register(router); + new CirculationSettingsResource(client).register(router); server.requestHandler(router) .listen(config().getInteger("port"), result -> { diff --git a/src/main/java/org/folio/circulation/domain/CirculationSetting.java b/src/main/java/org/folio/circulation/domain/CirculationSetting.java new file mode 100644 index 0000000000..25f3f1bf28 --- /dev/null +++ b/src/main/java/org/folio/circulation/domain/CirculationSetting.java @@ -0,0 +1,32 @@ +package org.folio.circulation.domain; + +import static lombok.AccessLevel.PRIVATE; +import static org.folio.circulation.support.json.JsonPropertyFetcher.getObjectProperty; +import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; + +import io.vertx.core.json.JsonObject; +import lombok.AllArgsConstructor; +import lombok.Getter; +import lombok.ToString; + +@AllArgsConstructor(access = PRIVATE) +@ToString(onlyExplicitlyIncluded = true) +public class CirculationSetting { + @ToString.Include + @Getter + private final JsonObject representation; + + @Getter + private final String id; + + @Getter + private final String name; + + @Getter + private final JsonObject value; + + public static CirculationSetting from(JsonObject representation) { + return new CirculationSetting(representation, getProperty(representation, "id"), + getProperty(representation, "name"), getObjectProperty(representation, "value")); + } +} diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java new file mode 100644 index 0000000000..4795c30f34 --- /dev/null +++ b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java @@ -0,0 +1,76 @@ +package org.folio.circulation.infrastructure.storage; + +import static org.folio.circulation.support.http.ResponseMapping.forwardOnFailure; +import static org.folio.circulation.support.http.ResponseMapping.mapUsingJson; +import static org.folio.circulation.support.results.Result.failed; +import static org.folio.circulation.support.results.ResultBinding.flatMapResult; + +import java.lang.invoke.MethodHandles; +import java.util.concurrent.CompletableFuture; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.folio.circulation.domain.CirculationSetting; +import org.folio.circulation.domain.MultipleRecords; +import org.folio.circulation.support.Clients; +import org.folio.circulation.support.CollectionResourceClient; +import org.folio.circulation.support.FetchSingleRecord; +import org.folio.circulation.support.RecordNotFoundFailure; +import org.folio.circulation.support.http.client.ResponseInterpreter; +import org.folio.circulation.support.results.Result; + +public class CirculationSettingsRepository { + private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + public static final String RECORDS_PROPERTY_NAME = "circulationSettings"; + private final CollectionResourceClient circulationSettingsStorageClient; + + public CirculationSettingsRepository(Clients clients) { + circulationSettingsStorageClient = clients.circulationSettingsStorageClient(); + } + + public CompletableFuture> getById(String id) { + log.debug("getById:: parameters id: {}", id); + + return FetchSingleRecord.forRecord(RECORDS_PROPERTY_NAME) + .using(circulationSettingsStorageClient) + .mapTo(CirculationSetting::from) + .whenNotFound(failed(new RecordNotFoundFailure(RECORDS_PROPERTY_NAME, id))) + .fetch(id); + } + + public CompletableFuture>> findBy(String query) { + log.debug("findBy:: parameters query: {}", query); + + return circulationSettingsStorageClient.getManyWithRawQueryStringParameters(query) + .thenApply(flatMapResult(response -> + MultipleRecords.from(response, CirculationSetting::from, RECORDS_PROPERTY_NAME))); + } + + public CompletableFuture> create( + CirculationSetting circulationSetting) { + + log.debug("create:: parameters circulationSetting: {}", circulationSetting); + + final var storageCirculationSetting = circulationSetting.getRepresentation(); + + return circulationSettingsStorageClient.post(storageCirculationSetting) + .thenApply(interpreter()::flatMap); + } + + public CompletableFuture> update( + CirculationSetting circulationSetting) { + + log.debug("update:: parameters circulationSetting: {}", circulationSetting); + + final var storageCirculationSetting = circulationSetting.getRepresentation(); + + return circulationSettingsStorageClient.put(circulationSetting.getId(), storageCirculationSetting) + .thenApply(interpreter()::flatMap); + } + + private ResponseInterpreter interpreter() { + return new ResponseInterpreter() + .flatMapOn(201, mapUsingJson(CirculationSetting::from)) + .otherwise(forwardOnFailure()); + } +} diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java new file mode 100644 index 0000000000..579e523ae2 --- /dev/null +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -0,0 +1,109 @@ +package org.folio.circulation.resources; + +import static org.folio.circulation.infrastructure.storage.CirculationSettingsRepository.RECORDS_PROPERTY_NAME; +import static org.folio.circulation.support.results.MappingFunctions.toFixedValue; + +import java.lang.invoke.MethodHandles; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.folio.circulation.domain.CirculationSetting; +import org.folio.circulation.infrastructure.storage.CirculationSettingsRepository; +import org.folio.circulation.support.Clients; +import org.folio.circulation.support.http.server.JsonHttpResponse; +import org.folio.circulation.support.http.server.NoContentResponse; +import org.folio.circulation.support.http.server.WebContext; + +import io.vertx.core.http.HttpClient; +import io.vertx.ext.web.RoutingContext; + +public class CirculationSettingsResource extends CollectionResource { + private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + + public CirculationSettingsResource(HttpClient client) { + super(client, "/circulation/settings"); + } + + @Override + void create(RoutingContext routingContext) { + final var context = new WebContext(routingContext); + final var clients = Clients.create(context, client); + final var circulationSettingsRepository = new CirculationSettingsRepository(clients); + + final var incomingRepresentation = routingContext.body().asJsonObject(); + final var circulationSetting = CirculationSetting.from(incomingRepresentation); + + circulationSettingsRepository.create(circulationSetting) + .thenApply(r -> r.map(CirculationSetting::getRepresentation)) + .thenApply(r -> r.map(JsonHttpResponse::created)) + .thenAccept(context::writeResultToHttpResponse); + } + + @Override + void replace(RoutingContext routingContext) { + final var context = new WebContext(routingContext); + final var clients = Clients.create(context, client); + final var circulationSettingsRepository = new CirculationSettingsRepository(clients); + + final var incomingRepresentation = routingContext.body().asJsonObject(); + final var circulationSetting = CirculationSetting.from(incomingRepresentation); + + circulationSettingsRepository.update(circulationSetting) + .thenApply(r -> r.map(CirculationSetting::getRepresentation)) + .thenApply(r -> r.map(JsonHttpResponse::created)) + .thenAccept(context::writeResultToHttpResponse); + } + + @Override + void get(RoutingContext routingContext) { + final var context = new WebContext(routingContext); + final var clients = Clients.create(context, client); + final var circulationSettingsRepository = new CirculationSettingsRepository(clients); + + final var id = routingContext.request().getParam("id"); + log.debug("get:: Requested circulation setting ID: {}", id); + + circulationSettingsRepository.getById(id) + .thenApply(r -> r.map(CirculationSetting::getRepresentation)) + .thenApply(r -> r.map(JsonHttpResponse::ok)) + .thenAccept(context::writeResultToHttpResponse); + } + + @Override + void delete(RoutingContext routingContext) { + final var context = new WebContext(routingContext); + final var clients = Clients.create(context, client); + + String id = routingContext.request().getParam("id"); + + clients.loansStorage().delete(id) + .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) + .thenAccept(context::writeResultToHttpResponse); + } + + @Override + void getMany(RoutingContext routingContext) { + final var context = new WebContext(routingContext); + final var clients = Clients.create(context, client); + final var circulationSettingsRepository = new CirculationSettingsRepository(clients); + + final var query = routingContext.request().query(); + log.debug("get:: Requested circulation settings by query: {}", query); + + circulationSettingsRepository.findBy(query) + .thenApply(multipleLoanRecordsResult -> multipleLoanRecordsResult.map(multipleRecords -> + multipleRecords.asJson(CirculationSetting::getRepresentation, RECORDS_PROPERTY_NAME))) + .thenApply(r -> r.map(JsonHttpResponse::ok)) + .thenAccept(context::writeResultToHttpResponse); + } + + @Override + void empty(RoutingContext routingContext) { + WebContext context = new WebContext(routingContext); + Clients clients = Clients.create(context, client); + + clients.loansStorage().delete() + .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) + .thenAccept(context::writeResultToHttpResponse); + } +} diff --git a/src/main/java/org/folio/circulation/support/Clients.java b/src/main/java/org/folio/circulation/support/Clients.java index 3ffc41941a..404b433461 100644 --- a/src/main/java/org/folio/circulation/support/Clients.java +++ b/src/main/java/org/folio/circulation/support/Clients.java @@ -69,6 +69,7 @@ public class Clients { private final CollectionResourceClient checkOutLockStorageClient; private final CollectionResourceClient circulationItemClient; private final GetManyRecordsClient settingsStorageClient; + private final CollectionResourceClient circulationSettingsStorageClient; public static Clients create(WebContext context, HttpClient httpClient) { return new Clients(context.createHttpClient(httpClient), context); @@ -136,6 +137,7 @@ private Clients(OkapiHttpClient client, WebContext context) { checkOutLockStorageClient = createCheckoutLockClient(client, context); settingsStorageClient = createSettingsStorageClient(client, context); circulationItemClient = createCirculationItemClient(client, context); + circulationSettingsStorageClient = createCirculationSettingsStorageClient(client, context); } catch(MalformedURLException e) { throw new InvalidOkapiLocationException(context.getOkapiLocation(), e); @@ -374,6 +376,10 @@ public CollectionResourceClient circulationItemClient() { return circulationItemClient; } + public CollectionResourceClient circulationSettingsStorageClient() { + return circulationSettingsStorageClient; + } + private static CollectionResourceClient getCollectionResourceClient( OkapiHttpClient client, WebContext context, String path) @@ -801,6 +807,13 @@ private CollectionResourceClient createCirculationItemClient( return getCollectionResourceClient(client, context, "/circulation-item"); } + private CollectionResourceClient createCirculationSettingsStorageClient( + OkapiHttpClient client, WebContext context) throws MalformedURLException { + + return getCollectionResourceClient(client, context, + "/circulation-settings-storage/circulation-settings"); + } + private GetManyRecordsClient createSettingsStorageClient( OkapiHttpClient client, WebContext context) throws MalformedURLException { From 6fa6d43ff77af4cdb38408dc6e01f43787fb8f28 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Wed, 19 Jun 2024 18:09:07 +0300 Subject: [PATCH 02/13] CIRC-2111 Add interface dependency --- descriptors/ModuleDescriptor-template.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json index e127e3294a..5960cae4bb 100644 --- a/descriptors/ModuleDescriptor-template.json +++ b/descriptors/ModuleDescriptor-template.json @@ -1314,6 +1314,10 @@ { "id": "settings", "version": "1.0" + }, + { + "id": "circulation-settings-storage", + "version": "1.0" } ], "optional": [ From 7744cf36a2d72d517c6932ff00604b9bf1be69a8 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Wed, 19 Jun 2024 19:19:30 +0300 Subject: [PATCH 03/13] CIRC-2111 Fix delete method --- .../resources/CirculationSettingsResource.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 579e523ae2..f23d0dc7bb 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -1,9 +1,11 @@ package org.folio.circulation.resources; import static org.folio.circulation.infrastructure.storage.CirculationSettingsRepository.RECORDS_PROPERTY_NAME; +import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; import static org.folio.circulation.support.results.MappingFunctions.toFixedValue; import java.lang.invoke.MethodHandles; +import java.util.UUID; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -15,6 +17,7 @@ import org.folio.circulation.support.http.server.WebContext; import io.vertx.core.http.HttpClient; +import io.vertx.core.json.JsonObject; import io.vertx.ext.web.RoutingContext; public class CirculationSettingsResource extends CollectionResource { @@ -31,7 +34,9 @@ void create(RoutingContext routingContext) { final var circulationSettingsRepository = new CirculationSettingsRepository(clients); final var incomingRepresentation = routingContext.body().asJsonObject(); + setRandomIdIfMissing(incomingRepresentation); final var circulationSetting = CirculationSetting.from(incomingRepresentation); + log.debug("replace:: Creating circulation setting: {}", circulationSetting); circulationSettingsRepository.create(circulationSetting) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) @@ -47,6 +52,7 @@ void replace(RoutingContext routingContext) { final var incomingRepresentation = routingContext.body().asJsonObject(); final var circulationSetting = CirculationSetting.from(incomingRepresentation); + log.debug("replace:: Replacing circulation setting : {}", circulationSetting); circulationSettingsRepository.update(circulationSetting) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) @@ -75,8 +81,9 @@ void delete(RoutingContext routingContext) { final var clients = Clients.create(context, client); String id = routingContext.request().getParam("id"); + log.debug("delete:: Deleting circulation setting ID: {}", id); - clients.loansStorage().delete(id) + clients.circulationSettingsStorageClient().delete(id) .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); } @@ -106,4 +113,11 @@ void empty(RoutingContext routingContext) { .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); } + + private void setRandomIdIfMissing(JsonObject representation) { + final var providedId = getProperty(representation, "id"); + if (providedId == null) { + representation.put("id", UUID.randomUUID().toString()); + } + } } From 02870e1b99ad30db7375dff836bdff2328d96b82 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 16:50:24 +0300 Subject: [PATCH 04/13] CIRC-2111 Add more tests --- .../domain/CirculationSetting.java | 14 ++++ .../CirculationSettingsResource.java | 22 ++++- .../settings/CirculationSettingsTests.java | 84 +++++++++++++++++++ src/test/java/api/support/APITests.java | 3 + .../builders/CirculationSettingBuilder.java | 34 ++++++++ .../java/api/support/fakes/FakeOkapi.java | 7 ++ .../java/api/support/http/InterfaceUrls.java | 3 + .../java/api/support/http/ResourceClient.java | 4 + 8 files changed, 168 insertions(+), 3 deletions(-) create mode 100644 src/test/java/api/settings/CirculationSettingsTests.java create mode 100644 src/test/java/api/support/builders/CirculationSettingBuilder.java diff --git a/src/main/java/org/folio/circulation/domain/CirculationSetting.java b/src/main/java/org/folio/circulation/domain/CirculationSetting.java index 25f3f1bf28..bfdd91fee3 100644 --- a/src/main/java/org/folio/circulation/domain/CirculationSetting.java +++ b/src/main/java/org/folio/circulation/domain/CirculationSetting.java @@ -4,6 +4,11 @@ import static org.folio.circulation.support.json.JsonPropertyFetcher.getObjectProperty; import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; +import java.lang.invoke.MethodHandles; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + import io.vertx.core.json.JsonObject; import lombok.AllArgsConstructor; import lombok.Getter; @@ -12,6 +17,8 @@ @AllArgsConstructor(access = PRIVATE) @ToString(onlyExplicitlyIncluded = true) public class CirculationSetting { + private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + @ToString.Include @Getter private final JsonObject representation; @@ -26,6 +33,13 @@ public class CirculationSetting { private final JsonObject value; public static CirculationSetting from(JsonObject representation) { + if (getProperty(representation, "name") == null || + getObjectProperty(representation, "value") == null) { + + log.info("from:: Circulation setting JSON is malformed: {}", representation); + return null; + } + return new CirculationSetting(representation, getProperty(representation, "id"), getProperty(representation, "name"), getObjectProperty(representation, "value")); } diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index f23d0dc7bb..0ef7b2bbd4 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -1,11 +1,15 @@ package org.folio.circulation.resources; import static org.folio.circulation.infrastructure.storage.CirculationSettingsRepository.RECORDS_PROPERTY_NAME; +import static org.folio.circulation.support.ValidationErrorFailure.singleValidationError; import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; import static org.folio.circulation.support.results.MappingFunctions.toFixedValue; +import static org.folio.circulation.support.results.Result.ofAsync; +import static org.folio.circulation.support.results.Result.succeeded; import java.lang.invoke.MethodHandles; import java.util.UUID; +import java.util.function.Function; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -15,6 +19,7 @@ import org.folio.circulation.support.http.server.JsonHttpResponse; import org.folio.circulation.support.http.server.NoContentResponse; import org.folio.circulation.support.http.server.WebContext; +import org.folio.circulation.support.results.Result; import io.vertx.core.http.HttpClient; import io.vertx.core.json.JsonObject; @@ -38,7 +43,9 @@ void create(RoutingContext routingContext) { final var circulationSetting = CirculationSetting.from(incomingRepresentation); log.debug("replace:: Creating circulation setting: {}", circulationSetting); - circulationSettingsRepository.create(circulationSetting) + ofAsync(circulationSetting) + .thenApply(refuseWhenCirculationSettingIsInvalid()) + .thenCompose(r -> r.after(circulationSettingsRepository::create)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::created)) .thenAccept(context::writeResultToHttpResponse); @@ -54,12 +61,21 @@ void replace(RoutingContext routingContext) { final var circulationSetting = CirculationSetting.from(incomingRepresentation); log.debug("replace:: Replacing circulation setting : {}", circulationSetting); - circulationSettingsRepository.update(circulationSetting) + ofAsync(circulationSetting) + .thenApply(refuseWhenCirculationSettingIsInvalid()) + .thenCompose(r -> r.after(circulationSettingsRepository::update)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::created)) .thenAccept(context::writeResultToHttpResponse); } + private Function, Result> + refuseWhenCirculationSettingIsInvalid() { + + return r -> r.failWhen(circulationSetting -> succeeded(circulationSetting == null), + circulationSetting -> singleValidationError("Circulation setting JSON is malformed", "", "")); + } + @Override void get(RoutingContext routingContext) { final var context = new WebContext(routingContext); @@ -95,7 +111,7 @@ void getMany(RoutingContext routingContext) { final var circulationSettingsRepository = new CirculationSettingsRepository(clients); final var query = routingContext.request().query(); - log.debug("get:: Requested circulation settings by query: {}", query); + log.info("get:: Requested circulation settings by query: {}", query); circulationSettingsRepository.findBy(query) .thenApply(multipleLoanRecordsResult -> multipleLoanRecordsResult.map(multipleRecords -> diff --git a/src/test/java/api/settings/CirculationSettingsTests.java b/src/test/java/api/settings/CirculationSettingsTests.java new file mode 100644 index 0000000000..dd79417389 --- /dev/null +++ b/src/test/java/api/settings/CirculationSettingsTests.java @@ -0,0 +1,84 @@ +package api.settings; + +import static api.support.http.InterfaceUrls.circulationSettingsUrl; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +import org.junit.jupiter.api.Test; + +import api.support.APITests; +import api.support.builders.CirculationSettingBuilder; +import api.support.http.CqlQuery; +import io.vertx.core.json.JsonObject; + +public class CirculationSettingsTests extends APITests { + + @Test + void crudOperationsTest() { + // Testing POST method + final var setting = circulationSettingsClient.create(new CirculationSettingBuilder() + .withName("initial-name") + .withValue(new JsonObject().put("initial-key", "initial-value"))); + final var settingId = setting.getId(); + + // Testing GET (individual setting) method + final var settingById = circulationSettingsClient.get(settingId); + assertThat(settingById.getJson().getString("name"), is("initial-name")); + assertThat(settingById.getJson().getJsonObject("value").getString("initial-key"), + is("initial-value")); + + // Testing GET (all) method + final var anotherSetting = circulationSettingsClient.create(new CirculationSettingBuilder() + .withName("another-name") + .withValue(new JsonObject().put("another-key", "another-value"))); + final var allSettings = circulationSettingsClient.getMany(CqlQuery.noQuery()); + assertThat(allSettings.size(), is(2)); + + // Testing DELETE method + circulationSettingsClient.delete(anotherSetting.getId()); + final var allSettingsAfterDeletion = circulationSettingsClient.getMany(CqlQuery.noQuery()); + assertThat(allSettingsAfterDeletion.size(), is(1)); + assertThat(allSettingsAfterDeletion.getFirst().getString("name"), is("initial-name")); + assertThat(allSettingsAfterDeletion.getFirst().getJsonObject("value").getString("initial-key"), + is("initial-value")); + + // Testing PUT method + circulationSettingsClient.replace(settingId, new CirculationSettingBuilder() + .withId(settingId) + .withName("new-name") + .withValue(new JsonObject().put("new-key", "new-value"))); + + final var updatedSetting = circulationSettingsClient.get(settingId); + + assertThat(updatedSetting.getJson().getString("name"), is("new-name")); + assertThat(updatedSetting.getJson().getJsonObject("value").getString("new-key"), + is("new-value")); + } + + @Test + void invalidRequestsTest() { + final var setting = circulationSettingsClient.create(new CirculationSettingBuilder() + .withName("initial-name") + .withValue(new JsonObject().put("initial-key", "initial-value"))); + + // Testing GET with invalid ID + restAssuredClient.get(circulationSettingsUrl("/" + randomId()), 404, + "get-circulation-setting"); + + // Testing DELETE with invalid ID + restAssuredClient.delete(circulationSettingsUrl("/" + randomId()), 204, + "delete-circulation-setting"); + + // Testing PUT with malformed JSON + var putErrors = restAssuredClient.put("{\"invalid-field\": \"invalid-value\"}", + circulationSettingsUrl("/" + randomId()), 422, "put-circulation-setting"); + assertThat(putErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), + is("Circulation setting JSON is malformed")); + + // Testing POST with malformed JSON + var postErrors = restAssuredClient.post("{\"invalid-field\": \"invalid-value\"}", + circulationSettingsUrl(""), 422, "put-circulation-setting"); + assertThat(postErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), + is("Circulation setting JSON is malformed")); + } +} diff --git a/src/test/java/api/support/APITests.java b/src/test/java/api/support/APITests.java index ddc2dce289..b872ce7165 100644 --- a/src/test/java/api/support/APITests.java +++ b/src/test/java/api/support/APITests.java @@ -194,6 +194,9 @@ public abstract class APITests { protected final ResourceClient actualCostRecordsClient = ResourceClient.forActualCostRecordsStorage(); + protected final ResourceClient circulationSettingsClient = + ResourceClient.forCirculationSettings(); + protected final ServicePointsFixture servicePointsFixture = new ServicePointsFixture(servicePointsClient); diff --git a/src/test/java/api/support/builders/CirculationSettingBuilder.java b/src/test/java/api/support/builders/CirculationSettingBuilder.java new file mode 100644 index 0000000000..a33fb99345 --- /dev/null +++ b/src/test/java/api/support/builders/CirculationSettingBuilder.java @@ -0,0 +1,34 @@ +package api.support.builders; + +import java.util.UUID; + +import io.vertx.core.json.JsonObject; +import lombok.AllArgsConstructor; +import lombok.NoArgsConstructor; +import lombok.With; + +@NoArgsConstructor +@AllArgsConstructor +@With +public class CirculationSettingBuilder extends JsonBuilder implements Builder { + private UUID id = null; + private String name = null; + private JsonObject value = null; + + @Override + public JsonObject create() { + JsonObject circulationSetting = new JsonObject(); + + if (id != null) { + put(circulationSetting, "id", id); + } + if (name != null) { + put(circulationSetting, "name", name); + } + if (value != null) { + put(circulationSetting, "value", value); + } + + return circulationSetting; + } +} diff --git a/src/test/java/api/support/fakes/FakeOkapi.java b/src/test/java/api/support/fakes/FakeOkapi.java index 9cdfb40600..5cbecb8542 100644 --- a/src/test/java/api/support/fakes/FakeOkapi.java +++ b/src/test/java/api/support/fakes/FakeOkapi.java @@ -415,6 +415,13 @@ public void start(Promise startFuture) throws IOException { .withChangeMetadata() .create().register(router); + new FakeStorageModuleBuilder() + .withRecordName("circulationSettings") + .withCollectionPropertyName("circulationSettings") + .withRootPath("/circulation-settings-storage/circulation-settings") + .withChangeMetadata() + .create().register(router); + new FakeFeeFineOperationsModule().register(router); server.requestHandler(router) diff --git a/src/test/java/api/support/http/InterfaceUrls.java b/src/test/java/api/support/http/InterfaceUrls.java index fedf39d696..59d35de534 100644 --- a/src/test/java/api/support/http/InterfaceUrls.java +++ b/src/test/java/api/support/http/InterfaceUrls.java @@ -334,4 +334,7 @@ public static URL settingsStorageUrl() { return APITestContext.viaOkapiModuleUrl("/settings/entries"); } + public static URL circulationSettingsUrl(String subPath) { + return circulationModuleUrl("/circulation/settings" + subPath); + } } diff --git a/src/test/java/api/support/http/ResourceClient.java b/src/test/java/api/support/http/ResourceClient.java index 844fff542d..8e9b7d63be 100644 --- a/src/test/java/api/support/http/ResourceClient.java +++ b/src/test/java/api/support/http/ResourceClient.java @@ -272,6 +272,10 @@ public static ResourceClient forActualCostRecordsStorage() { return new ResourceClient(InterfaceUrls::actualCostRecordsStorageUrl, "actualCostRecords"); } + public static ResourceClient forCirculationSettings() { + return new ResourceClient(InterfaceUrls::circulationSettingsUrl, "circulationSettings"); + } + private ResourceClient(UrlMaker urlMaker, String collectionArrayPropertyName) { this.urlMaker = urlMaker; this.collectionArrayPropertyName = collectionArrayPropertyName; From 042bd4fba6ec7a81a29fed257f03ea3455eb08d8 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 18:09:23 +0300 Subject: [PATCH 05/13] CIRC-2111 Validate UUID - GET and DELETE --- .../CirculationSettingsResource.java | 45 ++++++++++++------- .../settings/CirculationSettingsTests.java | 8 +++- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 0ef7b2bbd4..2cd0e28ae8 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -69,23 +69,16 @@ void replace(RoutingContext routingContext) { .thenAccept(context::writeResultToHttpResponse); } - private Function, Result> - refuseWhenCirculationSettingIsInvalid() { - - return r -> r.failWhen(circulationSetting -> succeeded(circulationSetting == null), - circulationSetting -> singleValidationError("Circulation setting JSON is malformed", "", "")); - } - @Override void get(RoutingContext routingContext) { final var context = new WebContext(routingContext); final var clients = Clients.create(context, client); final var circulationSettingsRepository = new CirculationSettingsRepository(clients); - final var id = routingContext.request().getParam("id"); - log.debug("get:: Requested circulation setting ID: {}", id); - - circulationSettingsRepository.getById(id) + ofAsync(routingContext.request().getParam("id")) + .thenApply(refuseWhenIdIsInvalid()) + .thenApply(r -> r.map(providedId -> UUID.fromString(providedId).toString())) + .thenCompose(r -> r.after(circulationSettingsRepository::getById)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::ok)) .thenAccept(context::writeResultToHttpResponse); @@ -96,10 +89,10 @@ void delete(RoutingContext routingContext) { final var context = new WebContext(routingContext); final var clients = Clients.create(context, client); - String id = routingContext.request().getParam("id"); - log.debug("delete:: Deleting circulation setting ID: {}", id); - - clients.circulationSettingsStorageClient().delete(id) + ofAsync(routingContext.request().getParam("id")) + .thenApply(refuseWhenIdIsInvalid()) + .thenApply(r -> r.map(providedId -> UUID.fromString(providedId).toString())) + .thenCompose(r -> r.after(clients.circulationSettingsStorageClient()::delete)) .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); } @@ -136,4 +129,26 @@ private void setRandomIdIfMissing(JsonObject representation) { representation.put("id", UUID.randomUUID().toString()); } } + + private Function, Result> + refuseWhenCirculationSettingIsInvalid() { + + return r -> r.failWhen(circulationSetting -> succeeded(circulationSetting == null), + circulationSetting -> singleValidationError("Circulation setting JSON is malformed", "", "")); + } + + private Function, Result> refuseWhenIdIsInvalid() { + return r -> r.failWhen(id -> succeeded(!uuidIsValid(id)), + circulationSetting -> singleValidationError("Circulation setting ID is not a valid UUID", + "", "")); + } + + private boolean uuidIsValid(String providedId) { + try { + return providedId != null && providedId.equals(UUID.fromString(providedId).toString()); + } catch(IllegalArgumentException e) { + log.debug("refuseWhenIdIsInvalid:: Invalid UUID"); + return false; + } + } } diff --git a/src/test/java/api/settings/CirculationSettingsTests.java b/src/test/java/api/settings/CirculationSettingsTests.java index dd79417389..35587fb7af 100644 --- a/src/test/java/api/settings/CirculationSettingsTests.java +++ b/src/test/java/api/settings/CirculationSettingsTests.java @@ -61,10 +61,16 @@ void invalidRequestsTest() { .withName("initial-name") .withValue(new JsonObject().put("initial-key", "initial-value"))); - // Testing GET with invalid ID + // Testing GET with wrong UUID restAssuredClient.get(circulationSettingsUrl("/" + randomId()), 404, "get-circulation-setting"); + // Testing GET with invalid ID (not a UUID) + var getErrors = restAssuredClient.get(circulationSettingsUrl("/not-a-uuid"), 422, + "get-circulation-setting"); + assertThat(getErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), + is("Circulation setting ID is not a valid UUID")); + // Testing DELETE with invalid ID restAssuredClient.delete(circulationSettingsUrl("/" + randomId()), 204, "delete-circulation-setting"); From 3807aba11c635e7bb01daa02434bfe6874dbe7c2 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 18:55:23 +0300 Subject: [PATCH 06/13] CIRC-2111 Remove query logging --- .../infrastructure/storage/CirculationSettingsRepository.java | 1 - .../folio/circulation/resources/CirculationSettingsResource.java | 1 - 2 files changed, 2 deletions(-) diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java index 4795c30f34..e193484698 100644 --- a/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java +++ b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java @@ -39,7 +39,6 @@ public CompletableFuture> getById(String id) { } public CompletableFuture>> findBy(String query) { - log.debug("findBy:: parameters query: {}", query); return circulationSettingsStorageClient.getManyWithRawQueryStringParameters(query) .thenApply(flatMapResult(response -> diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 2cd0e28ae8..84cfe8ead1 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -104,7 +104,6 @@ void getMany(RoutingContext routingContext) { final var circulationSettingsRepository = new CirculationSettingsRepository(clients); final var query = routingContext.request().query(); - log.info("get:: Requested circulation settings by query: {}", query); circulationSettingsRepository.findBy(query) .thenApply(multipleLoanRecordsResult -> multipleLoanRecordsResult.map(multipleRecords -> From aaa275934451516c712280d5f1cffeb37367d664 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 19:21:13 +0300 Subject: [PATCH 07/13] CIRC-2111 Fix code smells --- src/test/java/api/settings/CirculationSettingsTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/api/settings/CirculationSettingsTests.java b/src/test/java/api/settings/CirculationSettingsTests.java index 35587fb7af..859555b8d6 100644 --- a/src/test/java/api/settings/CirculationSettingsTests.java +++ b/src/test/java/api/settings/CirculationSettingsTests.java @@ -11,7 +11,7 @@ import api.support.http.CqlQuery; import io.vertx.core.json.JsonObject; -public class CirculationSettingsTests extends APITests { +class CirculationSettingsTests extends APITests { @Test void crudOperationsTest() { @@ -57,7 +57,7 @@ void crudOperationsTest() { @Test void invalidRequestsTest() { - final var setting = circulationSettingsClient.create(new CirculationSettingBuilder() + circulationSettingsClient.create(new CirculationSettingBuilder() .withName("initial-name") .withValue(new JsonObject().put("initial-key", "initial-value"))); From 3a87757ca8ce09b6f34fc4c63a2731d78c8e2af0 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 19:38:28 +0300 Subject: [PATCH 08/13] CIRC-2111 Remove redundant UUID parsing --- .../circulation/resources/CirculationSettingsResource.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 84cfe8ead1..a0c80980ef 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -77,7 +77,6 @@ void get(RoutingContext routingContext) { ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) - .thenApply(r -> r.map(providedId -> UUID.fromString(providedId).toString())) .thenCompose(r -> r.after(circulationSettingsRepository::getById)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::ok)) @@ -91,7 +90,6 @@ void delete(RoutingContext routingContext) { ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) - .thenApply(r -> r.map(providedId -> UUID.fromString(providedId).toString())) .thenCompose(r -> r.after(clients.circulationSettingsStorageClient()::delete)) .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); @@ -146,7 +144,7 @@ private boolean uuidIsValid(String providedId) { try { return providedId != null && providedId.equals(UUID.fromString(providedId).toString()); } catch(IllegalArgumentException e) { - log.debug("refuseWhenIdIsInvalid:: Invalid UUID"); + log.debug("uuidIsValid:: Invalid UUID"); return false; } } From 5a9f605e77f22319d369c6ca448b35d496224c0e Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Thu, 20 Jun 2024 20:48:58 +0300 Subject: [PATCH 09/13] CIRC-2111 Add validation tests --- .../domain/CirculationSetting.java | 23 ++++++++--- .../CirculationSettingsResource.java | 10 ++--- .../settings/CirculationSettingsTests.java | 38 +++++++++++++------ 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/folio/circulation/domain/CirculationSetting.java b/src/main/java/org/folio/circulation/domain/CirculationSetting.java index bfdd91fee3..9be14118d4 100644 --- a/src/main/java/org/folio/circulation/domain/CirculationSetting.java +++ b/src/main/java/org/folio/circulation/domain/CirculationSetting.java @@ -5,6 +5,7 @@ import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; import java.lang.invoke.MethodHandles; +import java.util.Set; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -19,6 +20,11 @@ public class CirculationSetting { private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + public static final String ID_FIELD = "id"; + public static final String NAME_FIELD = "name"; + public static final String VALUE_FIELD = "value"; + public static final String METADATA_FIELD = "metadata"; + @ToString.Include @Getter private final JsonObject representation; @@ -33,14 +39,21 @@ public class CirculationSetting { private final JsonObject value; public static CirculationSetting from(JsonObject representation) { - if (getProperty(representation, "name") == null || - getObjectProperty(representation, "value") == null) { + if (getProperty(representation, ID_FIELD) == null || + getProperty(representation, NAME_FIELD) == null || + getObjectProperty(representation, VALUE_FIELD) == null || + !containsOnlyKnownFields(representation)) { - log.info("from:: Circulation setting JSON is malformed: {}", representation); + log.info("from:: Circulation setting JSON is invalid: {}", representation); return null; } - return new CirculationSetting(representation, getProperty(representation, "id"), - getProperty(representation, "name"), getObjectProperty(representation, "value")); + return new CirculationSetting(representation, getProperty(representation, ID_FIELD), + getProperty(representation, NAME_FIELD), getObjectProperty(representation, VALUE_FIELD)); + } + + private static boolean containsOnlyKnownFields(JsonObject representation) { + return Set.of(ID_FIELD, NAME_FIELD, VALUE_FIELD, METADATA_FIELD) + .containsAll(representation.fieldNames()); } } diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index a0c80980ef..d2bc614511 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -120,27 +120,27 @@ void empty(RoutingContext routingContext) { .thenAccept(context::writeResultToHttpResponse); } - private void setRandomIdIfMissing(JsonObject representation) { + private static void setRandomIdIfMissing(JsonObject representation) { final var providedId = getProperty(representation, "id"); if (providedId == null) { representation.put("id", UUID.randomUUID().toString()); } } - private Function, Result> + private static Function, Result> refuseWhenCirculationSettingIsInvalid() { return r -> r.failWhen(circulationSetting -> succeeded(circulationSetting == null), - circulationSetting -> singleValidationError("Circulation setting JSON is malformed", "", "")); + circulationSetting -> singleValidationError("Circulation setting JSON is invalid", "", "")); } - private Function, Result> refuseWhenIdIsInvalid() { + private static Function, Result> refuseWhenIdIsInvalid() { return r -> r.failWhen(id -> succeeded(!uuidIsValid(id)), circulationSetting -> singleValidationError("Circulation setting ID is not a valid UUID", "", "")); } - private boolean uuidIsValid(String providedId) { + private static boolean uuidIsValid(String providedId) { try { return providedId != null && providedId.equals(UUID.fromString(providedId).toString()); } catch(IllegalArgumentException e) { diff --git a/src/test/java/api/settings/CirculationSettingsTests.java b/src/test/java/api/settings/CirculationSettingsTests.java index 859555b8d6..da176eb5e9 100644 --- a/src/test/java/api/settings/CirculationSettingsTests.java +++ b/src/test/java/api/settings/CirculationSettingsTests.java @@ -13,6 +13,12 @@ class CirculationSettingsTests extends APITests { + public static final String NAME = "name"; + public static final String VALUE = "value"; + public static final String ERRORS = "errors"; + public static final String MESSAGE = "message"; + public static final String INVALID_JSON_MESSAGE = "Circulation setting JSON is invalid"; + @Test void crudOperationsTest() { // Testing POST method @@ -23,8 +29,8 @@ void crudOperationsTest() { // Testing GET (individual setting) method final var settingById = circulationSettingsClient.get(settingId); - assertThat(settingById.getJson().getString("name"), is("initial-name")); - assertThat(settingById.getJson().getJsonObject("value").getString("initial-key"), + assertThat(settingById.getJson().getString(NAME), is("initial-name")); + assertThat(settingById.getJson().getJsonObject(VALUE).getString("initial-key"), is("initial-value")); // Testing GET (all) method @@ -38,8 +44,8 @@ void crudOperationsTest() { circulationSettingsClient.delete(anotherSetting.getId()); final var allSettingsAfterDeletion = circulationSettingsClient.getMany(CqlQuery.noQuery()); assertThat(allSettingsAfterDeletion.size(), is(1)); - assertThat(allSettingsAfterDeletion.getFirst().getString("name"), is("initial-name")); - assertThat(allSettingsAfterDeletion.getFirst().getJsonObject("value").getString("initial-key"), + assertThat(allSettingsAfterDeletion.getFirst().getString(NAME), is("initial-name")); + assertThat(allSettingsAfterDeletion.getFirst().getJsonObject(VALUE).getString("initial-key"), is("initial-value")); // Testing PUT method @@ -50,8 +56,8 @@ void crudOperationsTest() { final var updatedSetting = circulationSettingsClient.get(settingId); - assertThat(updatedSetting.getJson().getString("name"), is("new-name")); - assertThat(updatedSetting.getJson().getJsonObject("value").getString("new-key"), + assertThat(updatedSetting.getJson().getString(NAME), is("new-name")); + assertThat(updatedSetting.getJson().getJsonObject(VALUE).getString("new-key"), is("new-value")); } @@ -68,7 +74,7 @@ void invalidRequestsTest() { // Testing GET with invalid ID (not a UUID) var getErrors = restAssuredClient.get(circulationSettingsUrl("/not-a-uuid"), 422, "get-circulation-setting"); - assertThat(getErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), + assertThat(getErrors.getJson().getJsonArray(ERRORS).getJsonObject(0).getString(MESSAGE), is("Circulation setting ID is not a valid UUID")); // Testing DELETE with invalid ID @@ -78,13 +84,23 @@ void invalidRequestsTest() { // Testing PUT with malformed JSON var putErrors = restAssuredClient.put("{\"invalid-field\": \"invalid-value\"}", circulationSettingsUrl("/" + randomId()), 422, "put-circulation-setting"); - assertThat(putErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), - is("Circulation setting JSON is malformed")); + assertThat(putErrors.getJson().getJsonArray(ERRORS).getJsonObject(0).getString(MESSAGE), + is(INVALID_JSON_MESSAGE)); + + var putErrorsNoValue = restAssuredClient.put("{\"name\": \"test-name\"}", + circulationSettingsUrl("/" + randomId()), 422, "put-circulation-setting"); + assertThat(putErrorsNoValue.getJson().getJsonArray(ERRORS).getJsonObject(0).getString(MESSAGE), + is(INVALID_JSON_MESSAGE)); // Testing POST with malformed JSON var postErrors = restAssuredClient.post("{\"invalid-field\": \"invalid-value\"}", circulationSettingsUrl(""), 422, "put-circulation-setting"); - assertThat(postErrors.getJson().getJsonArray("errors").getJsonObject(0).getString("message"), - is("Circulation setting JSON is malformed")); + assertThat(postErrors.getJson().getJsonArray(ERRORS).getJsonObject(0).getString(MESSAGE), + is(INVALID_JSON_MESSAGE)); + + var postErrorsNoValue = restAssuredClient.put("{\"name\": \"test-name\"}", + circulationSettingsUrl("/" + randomId()), 422, "put-circulation-setting"); + assertThat(postErrorsNoValue.getJson().getJsonArray(ERRORS).getJsonObject(0).getString(MESSAGE), + is(INVALID_JSON_MESSAGE)); } } From a28019963b34d9aa98e8d1a4e49963b97f6877ce Mon Sep 17 00:00:00 2001 From: Alexander Kurash Date: Fri, 21 Jun 2024 12:26:42 +0300 Subject: [PATCH 10/13] CIRC-2111 Apply suggestions from code review Co-authored-by: OleksandrVidinieiev <56632770+OleksandrVidinieiev@users.noreply.github.com> --- ramls/circulation-settings.raml | 4 ++-- .../infrastructure/storage/CirculationSettingsRepository.java | 1 - .../circulation/resources/CirculationSettingsResource.java | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ramls/circulation-settings.raml b/ramls/circulation-settings.raml index 9af4023eac..731b9187c6 100644 --- a/ramls/circulation-settings.raml +++ b/ramls/circulation-settings.raml @@ -86,14 +86,14 @@ resourceTypes: type: circulation-setting responses: 204: - description: "Circulation settings have been saved." + description: "Circulation settings have been saved" 500: description: "Internal server error" body: text/plain: example: "Internal server error" delete: - is: [ validate ] + is: [validate] responses: 204: description: "Circulation settings deleted" diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java index e193484698..8125f1761b 100644 --- a/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java +++ b/src/main/java/org/folio/circulation/infrastructure/storage/CirculationSettingsRepository.java @@ -39,7 +39,6 @@ public CompletableFuture> getById(String id) { } public CompletableFuture>> findBy(String query) { - return circulationSettingsStorageClient.getManyWithRawQueryStringParameters(query) .thenApply(flatMapResult(response -> MultipleRecords.from(response, CirculationSetting::from, RECORDS_PROPERTY_NAME))); diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index d2bc614511..8e9cc35da7 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -41,7 +41,7 @@ void create(RoutingContext routingContext) { final var incomingRepresentation = routingContext.body().asJsonObject(); setRandomIdIfMissing(incomingRepresentation); final var circulationSetting = CirculationSetting.from(incomingRepresentation); - log.debug("replace:: Creating circulation setting: {}", circulationSetting); + log.debug("create:: Creating circulation setting: {}", () -> circulationSetting); ofAsync(circulationSetting) .thenApply(refuseWhenCirculationSettingIsInvalid()) @@ -59,7 +59,7 @@ void replace(RoutingContext routingContext) { final var incomingRepresentation = routingContext.body().asJsonObject(); final var circulationSetting = CirculationSetting.from(incomingRepresentation); - log.debug("replace:: Replacing circulation setting : {}", circulationSetting); + log.debug("replace:: Replacing circulation setting : {}", () -> circulationSetting); ofAsync(circulationSetting) .thenApply(refuseWhenCirculationSettingIsInvalid()) From 007de1b056f5337c82eb162aa26d2d95e8b7600e Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Fri, 21 Jun 2024 13:37:03 +0300 Subject: [PATCH 11/13] CIRC-2111 Add param logging --- .../circulation/domain/CirculationSetting.java | 13 ++++++------- .../resources/CirculationSettingsResource.java | 7 ++++++- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/folio/circulation/domain/CirculationSetting.java b/src/main/java/org/folio/circulation/domain/CirculationSetting.java index 9be14118d4..03f254f1ed 100644 --- a/src/main/java/org/folio/circulation/domain/CirculationSetting.java +++ b/src/main/java/org/folio/circulation/domain/CirculationSetting.java @@ -39,17 +39,16 @@ public class CirculationSetting { private final JsonObject value; public static CirculationSetting from(JsonObject representation) { - if (getProperty(representation, ID_FIELD) == null || - getProperty(representation, NAME_FIELD) == null || - getObjectProperty(representation, VALUE_FIELD) == null || - !containsOnlyKnownFields(representation)) { + final var id = getProperty(representation, ID_FIELD); + final var name = getProperty(representation, NAME_FIELD); + final var value = getObjectProperty(representation, VALUE_FIELD); - log.info("from:: Circulation setting JSON is invalid: {}", representation); + if (id == null || name == null || value == null || !containsOnlyKnownFields(representation)) { + log.warn("from:: Circulation setting JSON is invalid: {}", representation); return null; } - return new CirculationSetting(representation, getProperty(representation, ID_FIELD), - getProperty(representation, NAME_FIELD), getObjectProperty(representation, VALUE_FIELD)); + return new CirculationSetting(representation, id, name, value); } private static boolean containsOnlyKnownFields(JsonObject representation) { diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 8e9cc35da7..67eee1ee24 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -75,6 +75,8 @@ void get(RoutingContext routingContext) { final var clients = Clients.create(context, client); final var circulationSettingsRepository = new CirculationSettingsRepository(clients); + log.debug("get:: parameters id: {}", routingContext.request().getParam("id")); + ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) .thenCompose(r -> r.after(circulationSettingsRepository::getById)) @@ -88,6 +90,8 @@ void delete(RoutingContext routingContext) { final var context = new WebContext(routingContext); final var clients = Clients.create(context, client); + log.debug("get:: parameters id: {}", routingContext.request().getParam("id")); + ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) .thenCompose(r -> r.after(clients.circulationSettingsStorageClient()::delete)) @@ -102,6 +106,7 @@ void getMany(RoutingContext routingContext) { final var circulationSettingsRepository = new CirculationSettingsRepository(clients); final var query = routingContext.request().query(); + log.debug("get:: parameters id: {}", query); circulationSettingsRepository.findBy(query) .thenApply(multipleLoanRecordsResult -> multipleLoanRecordsResult.map(multipleRecords -> @@ -144,7 +149,7 @@ private static boolean uuidIsValid(String providedId) { try { return providedId != null && providedId.equals(UUID.fromString(providedId).toString()); } catch(IllegalArgumentException e) { - log.debug("uuidIsValid:: Invalid UUID"); + log.warn("uuidIsValid:: Invalid UUID"); return false; } } From 82a8de3bfd099a4b3c555650c18177da960e73d3 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Fri, 21 Jun 2024 14:08:02 +0300 Subject: [PATCH 12/13] CIRC-2111 Improve logging --- .../resources/CirculationSettingsResource.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 67eee1ee24..22d76bc1fa 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -1,5 +1,6 @@ package org.folio.circulation.resources; +import static org.apache.logging.log4j.Level.DEBUG; import static org.folio.circulation.infrastructure.storage.CirculationSettingsRepository.RECORDS_PROPERTY_NAME; import static org.folio.circulation.support.ValidationErrorFailure.singleValidationError; import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; @@ -11,6 +12,7 @@ import java.util.UUID; import java.util.function.Function; +import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.folio.circulation.domain.CirculationSetting; @@ -75,10 +77,9 @@ void get(RoutingContext routingContext) { final var clients = Clients.create(context, client); final var circulationSettingsRepository = new CirculationSettingsRepository(clients); - log.debug("get:: parameters id: {}", routingContext.request().getParam("id")); - ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) + .thenApply(r -> r.map(id -> logAndReturn(id, DEBUG, "get:: parameters id: {}", id))) .thenCompose(r -> r.after(circulationSettingsRepository::getById)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::ok)) @@ -90,10 +91,9 @@ void delete(RoutingContext routingContext) { final var context = new WebContext(routingContext); final var clients = Clients.create(context, client); - log.debug("get:: parameters id: {}", routingContext.request().getParam("id")); - ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) + .thenApply(r -> r.map(id -> logAndReturn(id, DEBUG, "delete:: parameters id: {}", id))) .thenCompose(r -> r.after(clients.circulationSettingsStorageClient()::delete)) .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); @@ -106,7 +106,7 @@ void getMany(RoutingContext routingContext) { final var circulationSettingsRepository = new CirculationSettingsRepository(clients); final var query = routingContext.request().query(); - log.debug("get:: parameters id: {}", query); + log.debug("get:: parameters id: {}", () -> query); circulationSettingsRepository.findBy(query) .thenApply(multipleLoanRecordsResult -> multipleLoanRecordsResult.map(multipleRecords -> @@ -153,4 +153,11 @@ private static boolean uuidIsValid(String providedId) { return false; } } + + private static T logAndReturn(T returnValue, Level level, String message, + Object... params) { + + log.log(level, message, params); + return returnValue; + } } From 10b4506dcaacf0bad222df936b63c213d0c5b070 Mon Sep 17 00:00:00 2001 From: alexanderkurash Date: Fri, 21 Jun 2024 15:11:55 +0300 Subject: [PATCH 13/13] CIRC-2111 Use peek --- .../resources/CirculationSettingsResource.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java index 22d76bc1fa..bfc6e989b6 100644 --- a/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java +++ b/src/main/java/org/folio/circulation/resources/CirculationSettingsResource.java @@ -1,6 +1,5 @@ package org.folio.circulation.resources; -import static org.apache.logging.log4j.Level.DEBUG; import static org.folio.circulation.infrastructure.storage.CirculationSettingsRepository.RECORDS_PROPERTY_NAME; import static org.folio.circulation.support.ValidationErrorFailure.singleValidationError; import static org.folio.circulation.support.json.JsonPropertyFetcher.getProperty; @@ -12,7 +11,6 @@ import java.util.UUID; import java.util.function.Function; -import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.folio.circulation.domain.CirculationSetting; @@ -79,7 +77,7 @@ void get(RoutingContext routingContext) { ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) - .thenApply(r -> r.map(id -> logAndReturn(id, DEBUG, "get:: parameters id: {}", id))) + .thenApply(r -> r.peek(id -> log.debug("get:: parameters id: {}", id))) .thenCompose(r -> r.after(circulationSettingsRepository::getById)) .thenApply(r -> r.map(CirculationSetting::getRepresentation)) .thenApply(r -> r.map(JsonHttpResponse::ok)) @@ -93,7 +91,7 @@ void delete(RoutingContext routingContext) { ofAsync(routingContext.request().getParam("id")) .thenApply(refuseWhenIdIsInvalid()) - .thenApply(r -> r.map(id -> logAndReturn(id, DEBUG, "delete:: parameters id: {}", id))) + .thenApply(r -> r.peek(id -> log.debug("delete:: parameters id: {}", id))) .thenCompose(r -> r.after(clients.circulationSettingsStorageClient()::delete)) .thenApply(r -> r.map(toFixedValue(NoContentResponse::noContent))) .thenAccept(context::writeResultToHttpResponse); @@ -153,11 +151,4 @@ private static boolean uuidIsValid(String providedId) { return false; } } - - private static T logAndReturn(T returnValue, Level level, String message, - Object... params) { - - log.log(level, message, params); - return returnValue; - } }