diff --git a/src/integration-test/java/org/openlmis/requisition/repository/ProcessedRequestsRedisRepositoryIntegrationTest.java b/src/integration-test/java/org/openlmis/requisition/repository/ProcessedRequestsRedisRepositoryIntegrationTest.java index 0a15eace1..5e7d670ca 100644 --- a/src/integration-test/java/org/openlmis/requisition/repository/ProcessedRequestsRedisRepositoryIntegrationTest.java +++ b/src/integration-test/java/org/openlmis/requisition/repository/ProcessedRequestsRedisRepositoryIntegrationTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -73,4 +74,52 @@ public void shouldUpdateLocationForIdempotencyKey() { redisRepository.addOrUpdate(idempotencyKey1, resource2); assertEquals(resource2, redisRepository.findByIdempotencyKey(idempotencyKey1)); } + + @Test + public void shouldLockRequisitionForApprovalOnlyOnceUntilUnlocked() { + UUID requisitionId = UUID.randomUUID(); + String token = null; + try { + token = redisRepository.lockRequisitionForApproval(requisitionId); + assertNotNull(token); + assertNull(redisRepository.lockRequisitionForApproval(requisitionId)); + + assertTrue(redisRepository.unlockRequisitionForApproval(requisitionId, token)); + token = redisRepository.lockRequisitionForApproval(requisitionId); + assertNotNull(token); + } finally { + if (token != null) { + redisRepository.unlockRequisitionForApproval(requisitionId, token); + } + } + } + + @Test + public void shouldNotUnlockApprovalWhenTokenDoesNotMatch() { + // OLMIS-8206: an approval whose lock expired and was taken over must not release the new lock. + UUID requisitionId = UUID.randomUUID(); + String token = redisRepository.lockRequisitionForApproval(requisitionId); + try { + assertNotNull(token); + assertFalse(redisRepository.unlockRequisitionForApproval(requisitionId, "stale-token")); + // the real lock is still held, so it cannot be acquired again + assertNull(redisRepository.lockRequisitionForApproval(requisitionId)); + } finally { + redisRepository.unlockRequisitionForApproval(requisitionId, token); + } + } + + @Test + public void shouldRenewApprovalLockOnlyForMatchingToken() { + // OLMIS-8206: only the current holder may extend the lock's expiry. + UUID requisitionId = UUID.randomUUID(); + String token = redisRepository.lockRequisitionForApproval(requisitionId); + try { + assertNotNull(token); + assertTrue(redisRepository.renewApprovalLock(requisitionId, token)); + assertFalse(redisRepository.renewApprovalLock(requisitionId, "stale-token")); + } finally { + redisRepository.unlockRequisitionForApproval(requisitionId, token); + } + } } diff --git a/src/integration-test/java/org/openlmis/requisition/web/RequisitionControllerIntegrationTest.java b/src/integration-test/java/org/openlmis/requisition/web/RequisitionControllerIntegrationTest.java index f3be6ac36..2ff0534c9 100644 --- a/src/integration-test/java/org/openlmis/requisition/web/RequisitionControllerIntegrationTest.java +++ b/src/integration-test/java/org/openlmis/requisition/web/RequisitionControllerIntegrationTest.java @@ -204,6 +204,8 @@ public void setUp() { mockSearchSupervisoryNodeByProgramAndFacility(); when(processedRequestsRedisRepository.exists(any())).thenReturn(false); + when(processedRequestsRedisRepository.lockRequisitionForApproval(anyUuid())) + .thenReturn(UUID.randomUUID().toString()); when(approvedProductReferenceDataService.getApprovedProducts(anyUuid(), anyUuid())) .thenReturn(new ApproveProductsAggregator(emptyList(), UUID.randomUUID())); diff --git a/src/main/java/org/openlmis/requisition/Application.java b/src/main/java/org/openlmis/requisition/Application.java index a8bf23985..fe806b559 100644 --- a/src/main/java/org/openlmis/requisition/Application.java +++ b/src/main/java/org/openlmis/requisition/Application.java @@ -18,6 +18,8 @@ import java.time.Clock; import java.time.ZoneId; import java.util.Locale; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import javax.annotation.PostConstruct; import org.flywaydb.core.Flyway; import org.flywaydb.core.api.callback.Callback; @@ -211,6 +213,11 @@ RedisTemplate redisTemplate(RedisProperties properties) { return redisTemplate; } + @Bean(destroyMethod = "shutdownNow") + public ScheduledExecutorService approvalLockRenewalScheduler() { + return Executors.newScheduledThreadPool(4); + } + @Bean public LocalValidatorFactoryBean validator() { return new LocalValidatorFactoryBean(); diff --git a/src/main/java/org/openlmis/requisition/i18n/MessageKeys.java b/src/main/java/org/openlmis/requisition/i18n/MessageKeys.java index eac2b2491..a01f12546 100644 --- a/src/main/java/org/openlmis/requisition/i18n/MessageKeys.java +++ b/src/main/java/org/openlmis/requisition/i18n/MessageKeys.java @@ -84,6 +84,8 @@ public abstract class MessageKeys { + ".submittedRequisitionsNotFound"; public static final String ERROR_REQUISITION_MUST_BE_AUTHORIZED = ERROR_PREFIX + ".approve.requisitionMustBeAuthorized"; + public static final String ERROR_APPROVAL_IN_PROGRESS = ERROR_PREFIX + + ".approve.inProgress"; public static final String ERROR_NO_PERMISSION_TO_APPROVE_REQUISITION = ERROR_PREFIX + ".approve.noPermissionForSupervisoryNode"; public static final String ERROR_PERIOD_END_DATE_WRONG = ERROR_PREFIX diff --git a/src/main/java/org/openlmis/requisition/repository/custom/ProcessedRequestsRedisRepository.java b/src/main/java/org/openlmis/requisition/repository/custom/ProcessedRequestsRedisRepository.java index 097d523fe..9df780638 100644 --- a/src/main/java/org/openlmis/requisition/repository/custom/ProcessedRequestsRedisRepository.java +++ b/src/main/java/org/openlmis/requisition/repository/custom/ProcessedRequestsRedisRepository.java @@ -24,4 +24,16 @@ public interface ProcessedRequestsRedisRepository { UUID findByIdempotencyKey(UUID resourceId); void addOrUpdate(UUID key, UUID resourceId); + + /** + * Atomically takes the approval lock for the requisition and returns a unique owner token, + * or null if the lock is already held by someone else. + */ + String lockRequisitionForApproval(UUID requisitionId); + + /** Releases the approval lock only if the given token still owns it; true when released. */ + boolean unlockRequisitionForApproval(UUID requisitionId, String token); + + /** Extends the lock's expiry only if the given token still owns it; true when renewed. */ + boolean renewApprovalLock(UUID requisitionId, String token); } diff --git a/src/main/java/org/openlmis/requisition/repository/custom/impl/ProcessedRequestsRedisRepositoryImpl.java b/src/main/java/org/openlmis/requisition/repository/custom/impl/ProcessedRequestsRedisRepositoryImpl.java index bb859a83e..c93f12253 100644 --- a/src/main/java/org/openlmis/requisition/repository/custom/impl/ProcessedRequestsRedisRepositoryImpl.java +++ b/src/main/java/org/openlmis/requisition/repository/custom/impl/ProcessedRequestsRedisRepositoryImpl.java @@ -15,32 +15,68 @@ package org.openlmis.requisition.repository.custom.impl; +import java.util.Collections; +import java.util.Map; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import javax.annotation.PostConstruct; import org.apache.commons.lang3.StringUtils; import org.openlmis.requisition.repository.custom.ProcessedRequestsRedisRepository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; import org.springframework.data.redis.core.HashOperations; import org.springframework.data.redis.core.RedisTemplate; +import org.springframework.data.redis.core.StringRedisTemplate; +import org.springframework.data.redis.core.script.DefaultRedisScript; +import org.springframework.data.redis.core.script.RedisScript; import org.springframework.stereotype.Repository; @Repository public class ProcessedRequestsRedisRepositoryImpl implements ProcessedRequestsRedisRepository { private static final String HASH_KEY = "PROCESSED_REQUESTS"; + private static final String APPROVE_LOCK_PREFIX = "REQUISITION_APPROVE_LOCK:"; - private RedisTemplate redisTemplate; + // Release/renew only when the stored value still equals our token, so an approval that outlived + // its TTL cannot delete or extend a lock that a later approval has already taken over. + private static final RedisScript UNLOCK_SCRIPT = new DefaultRedisScript<>( + "if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) " + + "else return 0 end", Long.class); + private static final RedisScript RENEW_SCRIPT = new DefaultRedisScript<>( + "if redis.call('get', KEYS[1]) == ARGV[1] " + + "then return redis.call('pexpire', KEYS[1], ARGV[2]) else return 0 end", Long.class); + + private static final Logger LOGGER = + LoggerFactory.getLogger(ProcessedRequestsRedisRepositoryImpl.class); + + @Value("${approval.lock.timeoutMinutes}") + private long approvalLockTimeoutMinutes; + + private final RedisTemplate redisTemplate; + private final ScheduledExecutorService approvalLockRenewalScheduler; + private final Map> approvalLockRenewals = new ConcurrentHashMap<>(); private HashOperations hashOperations; + private StringRedisTemplate lockRedisTemplate; @Autowired - public ProcessedRequestsRedisRepositoryImpl(RedisTemplate redisTemplate) { + public ProcessedRequestsRedisRepositoryImpl(RedisTemplate redisTemplate, + ScheduledExecutorService approvalLockRenewalScheduler) { this.redisTemplate = redisTemplate; + this.approvalLockRenewalScheduler = approvalLockRenewalScheduler; } @PostConstruct private void init() { hashOperations = redisTemplate.opsForHash(); + // Plain-string template for the lock keys so the Lua scripts compare tokens and pass the + // expiry as readable values; the shared template uses JDK serialization for idempotency data. + lockRedisTemplate = new StringRedisTemplate(redisTemplate.getConnectionFactory()); + lockRedisTemplate.afterPropertiesSet(); } @Override @@ -61,4 +97,53 @@ public void addOrUpdate(UUID key, UUID resourceId) { ? StringUtils.EMPTY : resourceId.toString()); redisTemplate.expire(key.toString(), 24, TimeUnit.HOURS); } + + @Override + public String lockRequisitionForApproval(UUID requisitionId) { + // Atomic acquire-if-absent with a TTL crash safety-net. The value is a per-acquisition token + // so release and renewal can tell whether the lock sitting here is still ours. + String token = UUID.randomUUID().toString(); + Boolean acquired = lockRedisTemplate.opsForValue().setIfAbsent( + APPROVE_LOCK_PREFIX + requisitionId, token, + approvalLockTimeoutMinutes, TimeUnit.MINUTES); + if (!Boolean.TRUE.equals(acquired)) { + return null; + } + // Keep the lock alive while the approval runs; if this node dies the renewal stops and the + // TTL frees the lock on its own. + long intervalMillis = TimeUnit.MINUTES.toMillis(approvalLockTimeoutMinutes) / 3; + approvalLockRenewals.put(token, approvalLockRenewalScheduler.scheduleWithFixedDelay( + () -> renewQuietly(requisitionId, token), intervalMillis, intervalMillis, + TimeUnit.MILLISECONDS)); + return token; + } + + @Override + public boolean unlockRequisitionForApproval(UUID requisitionId, String token) { + ScheduledFuture renewal = approvalLockRenewals.remove(token); + if (renewal != null) { + renewal.cancel(false); + } + Long released = lockRedisTemplate.execute(UNLOCK_SCRIPT, + Collections.singletonList(APPROVE_LOCK_PREFIX + requisitionId), token); + return Long.valueOf(1).equals(released); + } + + @Override + public boolean renewApprovalLock(UUID requisitionId, String token) { + long leaseMillis = TimeUnit.MINUTES.toMillis(approvalLockTimeoutMinutes); + Long renewed = lockRedisTemplate.execute(RENEW_SCRIPT, + Collections.singletonList(APPROVE_LOCK_PREFIX + requisitionId), + token, Long.toString(leaseMillis)); + return Long.valueOf(1).equals(renewed); + } + + private void renewQuietly(UUID requisitionId, String token) { + try { + renewApprovalLock(requisitionId, token); + } catch (RuntimeException ex) { + // Swallow so a transient Redis error does not cancel the recurring renewal task. + LOGGER.warn("Could not renew approval lock for requisition {}", requisitionId, ex); + } + } } diff --git a/src/main/java/org/openlmis/requisition/web/BaseRequisitionController.java b/src/main/java/org/openlmis/requisition/web/BaseRequisitionController.java index 9c4ed6aa7..a733c2fbf 100644 --- a/src/main/java/org/openlmis/requisition/web/BaseRequisitionController.java +++ b/src/main/java/org/openlmis/requisition/web/BaseRequisitionController.java @@ -18,6 +18,7 @@ import static org.apache.commons.lang3.BooleanUtils.isNotTrue; import static org.apache.commons.lang3.BooleanUtils.isTrue; import static org.apache.commons.lang3.StringUtils.isNotEmpty; +import static org.openlmis.requisition.i18n.MessageKeys.ERROR_APPROVAL_IN_PROGRESS; import static org.openlmis.requisition.i18n.MessageKeys.ERROR_FACILITY_NOT_FOUND; import static org.openlmis.requisition.i18n.MessageKeys.ERROR_ID_MISMATCH; import static org.openlmis.requisition.i18n.MessageKeys.ERROR_PERIOD_END_DATE_WRONG; @@ -109,6 +110,8 @@ import org.springframework.beans.factory.annotation.Value; import org.springframework.context.i18n.LocaleContextHolder; import org.springframework.http.HttpHeaders; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; @SuppressWarnings("PMD.TooManyMethods") public abstract class BaseRequisitionController extends BaseController { @@ -639,6 +642,44 @@ void validateIdempotencyKey(HttpServletRequest request, Profiler profiler) { } } + /** + * Locks the requisition before approval so two approvals cannot run at once. Returns the owner + * token needed to release it; the repository renews the lock until it is released. + */ + String acquireApprovalLock(UUID requisitionId, Profiler profiler) { + profiler.start("ACQUIRE_APPROVAL_LOCK"); + String token = processedRequestsRedisRepository.lockRequisitionForApproval(requisitionId); + if (token == null) { + throw new IdempotencyKeyException(new Message(ERROR_APPROVAL_IN_PROGRESS, requisitionId)); + } + return token; + } + + void releaseApprovalLock(UUID requisitionId, String token) { + processedRequestsRedisRepository.unlockRequisitionForApproval(requisitionId, token); + } + + /** + * Releases the lock only after the transaction ends, so a concurrent approval cannot see + * uncommitted requisition status. + */ + // S1604: TransactionSynchronization has only default methods in Spring 5.2, so it is not a + // functional interface and this anonymous class cannot be rewritten as a lambda. + @SuppressWarnings("java:S1604") + void releaseApprovalLockAfterTransaction(UUID requisitionId, String token) { + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.registerSynchronization( + new TransactionSynchronization() { + @Override + public void afterCompletion(int status) { + releaseApprovalLock(requisitionId, token); + } + }); + } else { + releaseApprovalLock(requisitionId, token); + } + } + void addLocationHeader(HttpServletRequest request, HttpServletResponse response, UUID requisitionId, Profiler profiler) { profiler.start("ADD_LOCATION_HEADER"); diff --git a/src/main/java/org/openlmis/requisition/web/RequisitionController.java b/src/main/java/org/openlmis/requisition/web/RequisitionController.java index d8f13add2..26bbc455c 100644 --- a/src/main/java/org/openlmis/requisition/web/RequisitionController.java +++ b/src/main/java/org/openlmis/requisition/web/RequisitionController.java @@ -433,6 +433,16 @@ public BasicRequisitionDto approveRequisition( Profiler profiler = getProfiler("APPROVE_REQUISITION", requisitionId); + String approvalLockToken = acquireApprovalLock(requisitionId, profiler); + try { + return doApproveRequisition(requisitionId, request, response, profiler); + } finally { + releaseApprovalLockAfterTransaction(requisitionId, approvalLockToken); + } + } + + private BasicRequisitionDto doApproveRequisition(UUID requisitionId, + HttpServletRequest request, HttpServletResponse response, Profiler profiler) { Requisition requisition = findRequisition(requisitionId, profiler); UserDto user = getCurrentUser(profiler); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 118ae4a58..8f0360e18 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -88,4 +88,8 @@ request.maxUrlLength=2000 unskip.requisition.item.when.approving=${UNSKIP_REQUISITION_ITEM_WHEN_APPROVING:false} -transferDataToStockManagement.enabled=${TRANSFER_DATA_TO_STOCK_MANAGEMENT:true} \ No newline at end of file +transferDataToStockManagement.enabled=${TRANSFER_DATA_TO_STOCK_MANAGEMENT:true} + +# TTL (minutes) of the per-requisition approval lock. The lock is renewed while an approval runs, +# so this is the grace period after a crash before the lock frees itself, not a cap on approval time. +approval.lock.timeoutMinutes=${APPROVAL_LOCK_TIMEOUT_MINUTES:30} \ No newline at end of file diff --git a/src/main/resources/messages_en.properties b/src/main/resources/messages_en.properties index 4b44baced..9d13fe48d 100644 --- a/src/main/resources/messages_en.properties +++ b/src/main/resources/messages_en.properties @@ -11,6 +11,7 @@ requisition.error.submit.fieldsMustHaveValues=Cannot submit requisition: {0}, Th requisition.error.update.canNotUpdateWithStatus=Cannot update a requisition with status: {0}. requisition.error.periodsForInitiate.missingParameters=Facility and program are required to return processing periods. requisition.error.approve.requisitionMustBeAuthorized=Cannot approve requisition: {0}. The requisition must be authorized in order to be eligible for approval. +requisition.error.approve.inProgress=An approval of requisition {0} is already in progress. Please wait for it to finish before trying again. requisition.error.approve.noPermissionForSupervisoryNode=Cannot approve this requisition because you do not have right for this supervisory node. requisition.error.approve.errorPeriodEndDateWrong=You cannot submit/authorize/approve a regular requisition before this period''s end date {0}. Please wait until the period ends and submit final, complete data accurate as of the period end date. requisition.error.reject.requisitionMustBeWaitingForApproval=Cannot reject requisition: {0}. Requisition must be waiting for approval to be rejected. diff --git a/src/test/java/org/openlmis/requisition/web/RequisitionControllerTest.java b/src/test/java/org/openlmis/requisition/web/RequisitionControllerTest.java index 338774ff5..4d9407c3b 100644 --- a/src/test/java/org/openlmis/requisition/web/RequisitionControllerTest.java +++ b/src/test/java/org/openlmis/requisition/web/RequisitionControllerTest.java @@ -35,6 +35,7 @@ import static org.mockito.Matchers.anyMap; import static org.mockito.Matchers.anyMapOf; import static org.mockito.Matchers.anySetOf; +import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; @@ -343,6 +344,8 @@ public void setUp() { when(processedRequestsRedisRepository.exists(any())) .thenReturn(false); + when(processedRequestsRedisRepository.lockRequisitionForApproval(any(UUID.class))) + .thenReturn(UUID.randomUUID().toString()); when(request.getHeader(IDEMPOTENCY_KEY_HEADER)) .thenReturn(null); @@ -1096,6 +1099,34 @@ public void shouldNotAutomaticallyConvertIfSupplyingFacilityDoesNotSupportProgra verify(requisitionService, never()).convertToOrder(anyList(), any(UserDto.class)); } + @Test(expected = IdempotencyKeyException.class) + public void shouldRejectApprovalWhenAnotherApprovalIsInProgress() { + // OLMIS-8206 (W1): a concurrent approval of the same requisition is rejected with 409 + when(processedRequestsRedisRepository.lockRequisitionForApproval(any(UUID.class))) + .thenReturn(null); + + requisitionController.approveRequisition(authorizedRequsition.getId(), request, response); + } + + @Test + public void shouldReleaseApprovalLockWhenApprovalFails() { + // OLMIS-8206 (W1): the approval lock must still be released when the approval throws + mockSupervisoryNodeForApprove(); + doThrow(mock(PermissionMessageException.class)).when(requisitionService) + .validateCanApproveRequisition(any(Requisition.class), any(UUID.class)); + + boolean approvalFailed = false; + try { + requisitionController.approveRequisition(authorizedRequsition.getId(), request, response); + } catch (PermissionMessageException expected) { + approvalFailed = true; + } + + assertTrue("expected the approval to fail", approvalFailed); + verify(processedRequestsRedisRepository) + .unlockRequisitionForApproval(eq(authorizedRequsition.getId()), anyString()); + } + @Test(expected = PermissionMessageException.class) public void shouldNotApproveIfHasNoPermission() { mockSupervisoryNodeForApprove();