OLMIS-8206: Reject concurrent requisition approval

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);
+    }
+  }
 }

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

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<Object, Object> redisTemplate(RedisProperties properties) {
     return redisTemplate;
   }
 
+  @Bean(destroyMethod = "shutdownNow")
+  public ScheduledExecutorService approvalLockRenewalScheduler() {
+    return Executors.newScheduledThreadPool(4);
+  }
+
   @Bean
   public LocalValidatorFactoryBean validator() {
     return new LocalValidatorFactoryBean();

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

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

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<String, String> 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<Long> 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<Long> 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<String, String> redisTemplate;
+  private final ScheduledExecutorService approvalLockRenewalScheduler;
+  private final Map<String, ScheduledFuture<?>> approvalLockRenewals = new ConcurrentHashMap<>();
   private HashOperations hashOperations;
+  private StringRedisTemplate lockRedisTemplate;
 
   @Autowired
-  public ProcessedRequestsRedisRepositoryImpl(RedisTemplate<String, String> redisTemplate) {
+  public ProcessedRequestsRedisRepositoryImpl(RedisTemplate<String, String> 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);
+    }
+  }
 }

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

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

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}
+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}

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.

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