From 4414f1d6eefeb31c41825d61382e0519afc96388 Mon Sep 17 00:00:00 2001 From: Saba-Zedginidze-EPAM <148070844+Saba-Zedginidze-EPAM@users.noreply.github.com> Date: Thu, 12 Dec 2024 17:20:11 +0400 Subject: [PATCH] [CIRC-2199] Avoid self-invocation for request rules while moving requests (#1522) * [EUREKA-430] Avoid self-invocation for request rules while moving requests * [EUREKA-430] Remove unnecessary new constructor * [EUREKA-430] Add unit tests * [EUREKA-430] Improve tests --- .../requests/RequestPolicyRepository.java | 40 +++-- .../circulation/rules/ExecutableRules.java | 9 +- .../requests/RequestPolicyRepositoryTest.java | 166 ++++++++++++++++++ 3 files changed, 194 insertions(+), 21 deletions(-) create mode 100644 src/test/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepositoryTest.java diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java index 42a1fe5abf..33b0bcdc02 100644 --- a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java +++ b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java @@ -4,6 +4,7 @@ import static java.util.concurrent.CompletableFuture.completedFuture; import static java.util.function.Function.identity; import static java.util.stream.Collectors.toMap; +import static org.folio.circulation.rules.ExecutableRules.MATCH_FAIL_MSG_REGEX; import static org.folio.circulation.support.AsyncCoordinationUtil.allOf; import static org.folio.circulation.support.fetching.RecordFetching.findWithMultipleCqlIndexValues; import static org.folio.circulation.support.results.CommonFailures.failedDueToServerError; @@ -31,13 +32,14 @@ import org.folio.circulation.domain.User; import org.folio.circulation.domain.policy.RequestPolicy; import org.folio.circulation.rules.CirculationRuleCriteria; -import org.folio.circulation.support.CirculationRulesClient; +import org.folio.circulation.rules.CirculationRuleMatch; +import org.folio.circulation.rules.CirculationRulesProcessor; +import org.folio.circulation.rules.RulesExecutionParameters; import org.folio.circulation.support.Clients; import org.folio.circulation.support.CollectionResourceClient; import org.folio.circulation.support.FindWithMultipleCqlIndexValues; -import org.folio.circulation.support.ForwardOnFailure; +import org.folio.circulation.support.ServerErrorFailure; import org.folio.circulation.support.SingleRecordFetcher; -import org.folio.circulation.support.http.client.Response; import org.folio.circulation.support.results.Result; import io.vertx.core.json.JsonObject; @@ -45,12 +47,12 @@ public class RequestPolicyRepository { private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); - private final CirculationRulesClient circulationRequestRulesClient; + private final CirculationRulesProcessor circulationRulesProcessor; private final CollectionResourceClient requestPoliciesStorageClient; public RequestPolicyRepository(Clients clients) { - this.circulationRequestRulesClient = clients.circulationRequestRules(); this.requestPoliciesStorageClient = clients.requestPoliciesStorage(); + this.circulationRulesProcessor = clients.circulationRulesProcessor(); } public CompletableFuture> lookupRequestPolicy( @@ -168,24 +170,26 @@ private CompletableFuture> lookupRequestPolicyId(String materialT log.debug("lookupRequestPolicyId:: parameters materialTypeId: {}, patronGroupId: {}," + "loanTypeId: {}, locationId: {}", materialTypeId, patronGroupId, loanTypeId, locationId); - return circulationRequestRulesClient.applyRules(loanTypeId, locationId, - materialTypeId, patronGroupId) - .thenComposeAsync(r -> r.after(this::processRulesResponse)); + var params = new RulesExecutionParameters(loanTypeId, locationId, materialTypeId, patronGroupId, null); + return circulationRulesProcessor.getRequestPolicyAndMatch(params) + .thenCompose(this::processRulesResponse); } - private CompletableFuture> processRulesResponse(Response response) { - log.debug("processRulesResponse:: parameters response: {}", response); + private CompletableFuture> processRulesResponse(Result response) { + log.debug("processRulesResponse:: parameters response successful: {}", response.succeeded()); final CompletableFuture> future = new CompletableFuture<>(); - if (response.getStatusCode() == 404) { - log.info("processRulesResponse:: no matching request rules found"); - future.complete(failedDueToServerError("Unable to find matching request rules")); - } else if (response.getStatusCode() != 200) { - log.info("processRulesResponse:: failed to apply request rules"); - future.complete(failed(new ForwardOnFailure(response))); - } else { + if (response.succeeded()) { log.info("processRulesResponse:: successfully applied request rules"); - future.complete(succeeded(response.getJson().getString("requestPolicyId"))); + future.complete(succeeded(response.value().getPolicyId())); + } else { + if (response.cause() instanceof ServerErrorFailure e && e.getReason().matches(MATCH_FAIL_MSG_REGEX)) { + log.info("processRulesResponse:: no matching request rules found"); + future.complete(failedDueToServerError("Unable to find matching request rules")); + } else { + log.info("processRulesResponse:: failed to apply request rules"); + future.complete(failed(response.cause())); + } } return future; diff --git a/src/main/java/org/folio/circulation/rules/ExecutableRules.java b/src/main/java/org/folio/circulation/rules/ExecutableRules.java index 5809348651..12d49f85fd 100644 --- a/src/main/java/org/folio/circulation/rules/ExecutableRules.java +++ b/src/main/java/org/folio/circulation/rules/ExecutableRules.java @@ -21,6 +21,11 @@ public class ExecutableRules { private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + public static final String MATCH_FAIL_MSG = + "Executing circulation rules: `%s` with parameters: `%s` to determine %s did not find a match"; + public static final String MATCH_FAIL_MSG_REGEX + = "Executing circulation rules: `.*` with parameters: `.*` to determine .* did not find a match"; + @Getter() private final String text; private final Drools drools; @@ -75,9 +80,7 @@ private Result determinePolicy(RulesExecutionParameters pa private Function fail( RulesExecutionParameters parameters, String policyType) { - return match -> new ServerErrorFailure(format( - "Executing circulation rules: `%s` with parameters: `%s` to determine %s did not find a match", - text, parameters, policyType)); + return match -> new ServerErrorFailure(format(MATCH_FAIL_MSG, text, parameters, policyType)); } private Result noMatch(CirculationRuleMatch match) { diff --git a/src/test/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepositoryTest.java b/src/test/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepositoryTest.java new file mode 100644 index 0000000000..26013fbfe1 --- /dev/null +++ b/src/test/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepositoryTest.java @@ -0,0 +1,166 @@ +package org.folio.circulation.infrastructure.storage.requests; + +import static org.folio.circulation.rules.ExecutableRules.MATCH_FAIL_MSG; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; + +import org.folio.circulation.domain.Holdings; +import org.folio.circulation.domain.Item; +import org.folio.circulation.domain.LoanType; +import org.folio.circulation.domain.Location; +import org.folio.circulation.domain.MaterialType; +import org.folio.circulation.domain.Request; +import org.folio.circulation.domain.RequestAndRelatedRecords; +import org.folio.circulation.domain.User; +import org.folio.circulation.domain.policy.RequestPolicy; +import org.folio.circulation.rules.AppliedRuleConditions; +import org.folio.circulation.rules.CirculationRuleMatch; +import org.folio.circulation.rules.CirculationRulesProcessor; +import org.folio.circulation.rules.RulesExecutionParameters; +import org.folio.circulation.support.Clients; +import org.folio.circulation.support.CollectionResourceClient; +import org.folio.circulation.support.ServerErrorFailure; +import org.folio.circulation.support.http.client.Response; +import org.folio.circulation.support.results.Result; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import io.vertx.core.json.JsonObject; + +@ExtendWith(MockitoExtension.class) +public class RequestPolicyRepositoryTest { + + public static final String SAMPLE_POLICY_ID = UUID.randomUUID().toString(); + + @Mock + private CirculationRulesProcessor circulationRulesProcessor; + @Mock + private CollectionResourceClient requestPoliciesStorageClient; + @Mock + private Clients clients; + + private RequestPolicyRepository requestPolicyRepository; + + private Item item; + private User user; + private Request request; + private RequestAndRelatedRecords requestAndRelatedRecords; + + @BeforeEach + void setUp() { + doReturn(requestPoliciesStorageClient).when(clients).requestPoliciesStorage(); + doReturn(circulationRulesProcessor).when(clients).circulationRulesProcessor(); + requestPolicyRepository = new RequestPolicyRepository(clients); + item = Item.from(JsonObject.of( + "materialType", asJson(MaterialType.unknown()), + "loadType", asJson(LoanType.unknown()), + "location", asJson(Location.unknown()), + "holdings", asJson(Holdings.unknown())) + ); + user = new User(JsonObject.of("patronGroup", "sample-group")); + request = Request.from(JsonObject.of()).withItem(item).withRequester(user); + requestAndRelatedRecords = new RequestAndRelatedRecords(request); + } + + @Test + void testLookupRequestPolicy() throws ExecutionException, InterruptedException { + var ruleMatch = new CirculationRuleMatch(SAMPLE_POLICY_ID, mock(AppliedRuleConditions.class)); + when(circulationRulesProcessor.getRequestPolicyAndMatch(any(RulesExecutionParameters.class))) + .thenReturn(CompletableFuture.completedFuture(Result.succeeded(ruleMatch))); + + var policy = RequestPolicy.from(JsonObject.of("id", SAMPLE_POLICY_ID)); + when(requestPoliciesStorageClient.get(SAMPLE_POLICY_ID)) + .thenReturn(CompletableFuture.completedFuture(Result.succeeded(asResponse(policy)))); + + var result = requestPolicyRepository.lookupRequestPolicy(requestAndRelatedRecords).get().value(); + + assertEquals(SAMPLE_POLICY_ID, result.getRequestPolicy().getId()); + assertEquals(item, result.getRequest().getItem()); + assertEquals(user, result.getRequest().getUser()); + verify(circulationRulesProcessor).getRequestPolicyAndMatch(any(RulesExecutionParameters.class)); + verify(requestPoliciesStorageClient).get(SAMPLE_POLICY_ID); + } + + @Test + void testLookupRequestPolicyWhenItemIsNull() throws ExecutionException, InterruptedException { + var result = requestPolicyRepository.lookupRequestPolicy( + new RequestAndRelatedRecords(request.withItem(Item.from(null)))).get(); + + assertTrue(result.failed()); + assertEquals(ServerErrorFailure.class, result.cause().getClass()); + var cause = (ServerErrorFailure) result.cause(); + assertEquals("Unable to find matching request rules for unknown item", cause.getReason()); + verifyNoInteractions(circulationRulesProcessor); + verifyNoInteractions(requestPoliciesStorageClient); + } + + @Test + void testLookupRequestPolicyWhenRequestPolicyIdNotFound() throws ExecutionException, InterruptedException { + when(circulationRulesProcessor.getRequestPolicyAndMatch(any(RulesExecutionParameters.class))) + .thenReturn(CompletableFuture.completedFuture(Result.failed( + new ServerErrorFailure(MATCH_FAIL_MSG.formatted("rules", "params", "request policy"))))); + + var result = requestPolicyRepository.lookupRequestPolicy(requestAndRelatedRecords).get(); + + assertTrue(result.failed()); + assertEquals(ServerErrorFailure.class, result.cause().getClass()); + var cause = (ServerErrorFailure) result.cause(); + assertEquals("Unable to find matching request rules", cause.getReason()); + verify(circulationRulesProcessor).getRequestPolicyAndMatch(any(RulesExecutionParameters.class)); + verifyNoInteractions(requestPoliciesStorageClient); + } + + @Test + void testLookupRequestPolicyWhenExceptionThrownDuringFetchingRequestPolicyId() throws ExecutionException, InterruptedException { + var cause = new ServerErrorFailure("Internal Server Error"); + when(circulationRulesProcessor.getRequestPolicyAndMatch(any(RulesExecutionParameters.class))) + .thenReturn(CompletableFuture.completedFuture(Result.failed(cause))); + + var result = requestPolicyRepository.lookupRequestPolicy(requestAndRelatedRecords).get(); + + assertTrue(result.failed()); + assertEquals(cause, result.cause()); + verify(circulationRulesProcessor).getRequestPolicyAndMatch(any(RulesExecutionParameters.class)); + verifyNoInteractions(requestPoliciesStorageClient); + } + + @Test + void testLookupRequestPolicyWhenRequestPolicyNotFound() throws ExecutionException, InterruptedException { + var ruleMatch = new CirculationRuleMatch(SAMPLE_POLICY_ID, mock(AppliedRuleConditions.class)); + when(circulationRulesProcessor.getRequestPolicyAndMatch(any(RulesExecutionParameters.class))) + .thenReturn(CompletableFuture.completedFuture(Result.succeeded(ruleMatch))); + + var cause = new ServerErrorFailure("Not Found"); + when(requestPoliciesStorageClient.get(SAMPLE_POLICY_ID)) + .thenReturn(CompletableFuture.completedFuture(Result.failed(cause))); + + var result = requestPolicyRepository.lookupRequestPolicy(requestAndRelatedRecords).get(); + + assertTrue(result.failed()); + assertEquals(cause, result.cause()); + verify(circulationRulesProcessor).getRequestPolicyAndMatch(any(RulesExecutionParameters.class)); + verify(requestPoliciesStorageClient).get(SAMPLE_POLICY_ID); + } + + private static JsonObject asJson(T entity) { + return JsonObject.mapFrom(entity); + } + + private static Response asResponse(T entity) { + return new Response(200, JsonObject.mapFrom(entity).encode(), "application/json"); + } + +}