diff --git a/payment/src/main/java/org/killbill/billing/payment/core/sm/PaymentAutomatonRunner.java b/payment/src/main/java/org/killbill/billing/payment/core/sm/PaymentAutomatonRunner.java
index 4353d9e..308e6a2 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/sm/PaymentAutomatonRunner.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/sm/PaymentAutomatonRunner.java
@@ -125,20 +125,23 @@ public class PaymentAutomatonRunner {
final CallContext callContext, final InternalCallContext internalCallContext) throws PaymentApiException {
final DateTime utcNow = clock.getUTCNow();
- final PaymentStateContext paymentStateContext = new PaymentStateContext(isApiPayment, paymentId, transactionId, attemptId, paymentExternalKey, paymentTransactionExternalKey, transactionType,
+ // Retrieve the payment id from the payment external key if needed
+ final UUID effectivePaymentId = paymentId != null ? paymentId : retrievePaymentId(paymentExternalKey, internalCallContext);
+
+ final PaymentStateContext paymentStateContext = new PaymentStateContext(isApiPayment, effectivePaymentId, transactionId, attemptId, paymentExternalKey, paymentTransactionExternalKey, transactionType,
account, paymentMethodId, amount, currency, shouldLockAccount, overridePluginOperationResult, properties, internalCallContext, callContext);
final PaymentAutomatonDAOHelper daoHelper = new PaymentAutomatonDAOHelper(paymentStateContext, utcNow, paymentDao, pluginRegistry, internalCallContext, eventBus, paymentSMHelper);
final UUID effectivePaymentMethodId;
final String currentStateName;
- if (paymentId != null) {
+ if (effectivePaymentId != null) {
final PaymentModelDao paymentModelDao = daoHelper.getPayment();
effectivePaymentMethodId = paymentModelDao.getPaymentMethodId();
currentStateName = paymentModelDao.getLastSuccessStateName() != null ? paymentModelDao.getLastSuccessStateName() : paymentSMHelper.getInitStateNameForTransaction();
// Check for illegal states (should never happen)
- Preconditions.checkState(currentStateName != null, "State name cannot be null for payment " + paymentId);
+ Preconditions.checkState(currentStateName != null, "State name cannot be null for payment " + effectivePaymentId);
Preconditions.checkState(paymentMethodId == null || effectivePaymentMethodId.equals(paymentMethodId), "Specified payment method id " + paymentMethodId + " doesn't match the one on the payment " + effectivePaymentMethodId);
} else {
// If the payment method is not specified, retrieve the default one on the account; it could still be null, in which case
@@ -242,4 +245,13 @@ public class PaymentAutomatonRunner {
return invoiceProperty == null || invoiceProperty.getValue() == null ? null : invoiceProperty.getValue().toString();
}
+
+ private UUID retrievePaymentId(@Nullable final String paymentExternalKey, final InternalCallContext internalCallContext) {
+ if (paymentExternalKey == null) {
+ return null;
+ }
+
+ final PaymentModelDao payment = paymentDao.getPaymentByExternalKey(paymentExternalKey, internalCallContext);
+ return payment == null ? null : payment.getId();
+ }
}
diff --git a/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentLeavingStateCallback.java b/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentLeavingStateCallback.java
index 6fb17d7..edc6f70 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentLeavingStateCallback.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentLeavingStateCallback.java
@@ -71,21 +71,12 @@ public abstract class PaymentLeavingStateCallback implements LeavingStateCallbac
existingPaymentTransactions = ImmutableList.of();
}
+ // Validate the payment transactions belong to the right payment
+ validatePaymentId(existingPaymentTransactions);
+
// Validate some constraints on the unicity of that paymentTransactionExternalKey
validateUniqueTransactionExternalKey(existingPaymentTransactions);
- // Handle UNKNOWN cases, where we skip the whole state machine and let the getPayment (through Janitor) logic refresh the state.
- final PaymentTransactionModelDao unknownPaymentTransaction = getUnknownPaymentTransaction(existingPaymentTransactions);
- if (unknownPaymentTransaction != null) {
- // Reset the attemptId on the existing paymentTransaction row since it is not accurate
- unknownPaymentTransaction.setAttemptId(paymentStateContext.getAttemptId());
- // Set the current paymentTransaction in the context (needed for the state machine logic)
- paymentStateContext.setPaymentTransactionModelDao(unknownPaymentTransaction);
- // Set special flag to bypass the state machine altogether (plugin will not be called, state will not be updated, no event will be sent unless state is fixed)
- paymentStateContext.setSkipOperationForUnknownTransaction(true);
- return;
- }
-
// Handle PENDING cases, where we want to re-use the same transaction
final PaymentTransactionModelDao pendingPaymentTransaction = getPendingPaymentTransaction(existingPaymentTransactions);
if (pendingPaymentTransaction != null) {
@@ -94,7 +85,7 @@ public abstract class PaymentLeavingStateCallback implements LeavingStateCallbac
return;
}
- // At this point we are left with PAYMENT_FAILURE, PLUGIN_FAILURE or nothing, and we validated the uniquess of the paymentTransactionExternalKey so we will create a new row
+ // At this point we are left with PAYMENT_FAILURE, PLUGIN_FAILURE or nothing, and we validated the uniqueness of the paymentTransactionExternalKey so we will create a new row
daoHelper.createNewPaymentTransaction();
} catch (PaymentApiException e) {
@@ -141,4 +132,13 @@ public abstract class PaymentLeavingStateCallback implements LeavingStateCallbac
throw new PaymentApiException(ErrorCode.PAYMENT_ACTIVE_TRANSACTION_KEY_EXISTS, paymentStateContext.getPaymentTransactionExternalKey());
}
}
+
+ // At this point, the payment id should have been populated for follow-up transactions (see PaymentAutomationRunner#run)
+ protected void validatePaymentId(final List<PaymentTransactionModelDao> existingPaymentTransactions) throws PaymentApiException {
+ for (final PaymentTransactionModelDao paymentTransactionModelDao : existingPaymentTransactions) {
+ if (!paymentTransactionModelDao.getPaymentId().equals(paymentStateContext.getPaymentId())) {
+ throw new PaymentApiException(ErrorCode.PAYMENT_INVALID_PARAMETER, paymentTransactionModelDao.getId(), "does not belong to payment " + paymentStateContext.getPaymentId());
+ }
+ }
+ }
}
diff --git a/payment/src/test/java/org/killbill/billing/payment/api/TestPaymentApi.java b/payment/src/test/java/org/killbill/billing/payment/api/TestPaymentApi.java
index 19c04f6..a5b2a5a 100644
--- a/payment/src/test/java/org/killbill/billing/payment/api/TestPaymentApi.java
+++ b/payment/src/test/java/org/killbill/billing/payment/api/TestPaymentApi.java
@@ -582,34 +582,74 @@ public class TestPaymentApi extends PaymentTestSuiteWithEmbeddedDB {
}
}
- @Test(groups = "slow")
- public void testApiRetryWithUnknownPaymentTransaction() throws Exception {
+ @Test(groups = "slow", description = "https://github.com/killbill/killbill/issues/371")
+ public void testApiWithDuplicatePendingPaymentTransaction() throws Exception {
final BigDecimal requestedAmount = BigDecimal.TEN;
- final String paymentExternalKey = UUID.randomUUID().toString();
- final String paymentTransactionExternalKey = UUID.randomUUID().toString();
-
- final Payment badPayment = paymentApi.createAuthorization(account, account.getPaymentMethodId(), null, requestedAmount, account.getCurrency(),
- paymentExternalKey, paymentTransactionExternalKey, ImmutableList.<PluginProperty>of(), callContext);
-
- final String paymentStateName = paymentSMHelper.getErroredStateForTransaction(TransactionType.AUTHORIZE).toString();
- paymentDao.updatePaymentAndTransactionOnCompletion(account.getId(), badPayment.getId(), TransactionType.AUTHORIZE, paymentStateName, paymentStateName,
- badPayment.getTransactions().get(0).getId(), TransactionStatus.UNKNOWN, requestedAmount, account.getCurrency(),
- "eroor 64", "bad something happened", internalCallContext);
-
- final Payment payment = paymentApi.createAuthorization(account, account.getPaymentMethodId(), null, requestedAmount, account.getCurrency(),
- paymentExternalKey, paymentTransactionExternalKey, ImmutableList.<PluginProperty>of(), callContext);
-
- Assert.assertEquals(payment.getId(), badPayment.getId());
- Assert.assertEquals(payment.getExternalKey(), paymentExternalKey);
- Assert.assertEquals(payment.getExternalKey(), paymentExternalKey);
+ for (final TransactionType transactionType : ImmutableList.<TransactionType>of(TransactionType.AUTHORIZE, TransactionType.PURCHASE, TransactionType.CREDIT)) {
+ final String payment1ExternalKey = UUID.randomUUID().toString();
+ final String payment1TransactionExternalKey = UUID.randomUUID().toString();
+ final String payment2ExternalKey = UUID.randomUUID().toString();
+ final String payment2TransactionExternalKey = UUID.randomUUID().toString();
+ final String payment3TransactionExternalKey = UUID.randomUUID().toString();
+
+ final Payment pendingPayment1 = createPayment(transactionType, null, payment1ExternalKey, payment1TransactionExternalKey, requestedAmount, PaymentPluginStatus.PENDING);
+ Assert.assertNotNull(pendingPayment1);
+ Assert.assertEquals(pendingPayment1.getExternalKey(), payment1ExternalKey);
+ Assert.assertEquals(pendingPayment1.getTransactions().size(), 1);
+ Assert.assertEquals(pendingPayment1.getTransactions().get(0).getAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment1.getTransactions().get(0).getProcessedAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment1.getTransactions().get(0).getCurrency(), account.getCurrency());
+ Assert.assertEquals(pendingPayment1.getTransactions().get(0).getExternalKey(), payment1TransactionExternalKey);
+ Assert.assertEquals(pendingPayment1.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PENDING);
+
+ // Attempt to create a second transaction for the same payment, but with a different transaction external key
+ final Payment pendingPayment2 = createPayment(transactionType, null, payment1ExternalKey, payment2TransactionExternalKey, requestedAmount, PaymentPluginStatus.PENDING);
+ Assert.assertNotNull(pendingPayment2);
+ Assert.assertEquals(pendingPayment2.getId(), pendingPayment1.getId());
+ Assert.assertEquals(pendingPayment2.getExternalKey(), payment1ExternalKey);
+ Assert.assertEquals(pendingPayment2.getTransactions().size(), 2);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(0).getAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(0).getProcessedAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(0).getCurrency(), account.getCurrency());
+ Assert.assertEquals(pendingPayment2.getTransactions().get(0).getExternalKey(), payment1TransactionExternalKey);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PENDING);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(1).getAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(1).getProcessedAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(1).getCurrency(), account.getCurrency());
+ Assert.assertEquals(pendingPayment2.getTransactions().get(1).getExternalKey(), payment2TransactionExternalKey);
+ Assert.assertEquals(pendingPayment2.getTransactions().get(1).getTransactionStatus(), TransactionStatus.PENDING);
+
+ try {
+ // Verify we cannot use the same transaction external key on a different payment if the payment id isn't specified
+ createPayment(transactionType, null, payment2ExternalKey, payment1TransactionExternalKey, requestedAmount, PaymentPluginStatus.PENDING);
+ Assert.fail();
+ } catch (final PaymentApiException e) {
+ Assert.assertEquals(e.getCode(), ErrorCode.PAYMENT_INVALID_PARAMETER.getCode());
+ }
- Assert.assertEquals(payment.getTransactions().size(), 1);
- Assert.assertEquals(payment.getTransactions().get(0).getTransactionStatus(), TransactionStatus.SUCCESS);
- Assert.assertEquals(payment.getTransactions().get(0).getExternalKey(), paymentTransactionExternalKey);
+ try {
+ // Verify we cannot use the same transaction external key on a different payment if the payment id isn't specified
+ createPayment(transactionType, null, payment2ExternalKey, payment2TransactionExternalKey, requestedAmount, PaymentPluginStatus.PENDING);
+ Assert.fail();
+ } catch (final PaymentApiException e) {
+ Assert.assertEquals(e.getCode(), ErrorCode.PAYMENT_INVALID_PARAMETER.getCode());
+ }
+
+ // Attempt to create a second transaction for a different payment
+ final Payment pendingPayment3 = createPayment(transactionType, null, payment2ExternalKey, payment3TransactionExternalKey, requestedAmount, PaymentPluginStatus.PENDING);
+ Assert.assertNotNull(pendingPayment3);
+ Assert.assertNotEquals(pendingPayment3.getId(), pendingPayment1.getId());
+ Assert.assertEquals(pendingPayment3.getExternalKey(), payment2ExternalKey);
+ Assert.assertEquals(pendingPayment3.getTransactions().size(), 1);
+ Assert.assertEquals(pendingPayment3.getTransactions().get(0).getAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment3.getTransactions().get(0).getProcessedAmount().compareTo(requestedAmount), 0);
+ Assert.assertEquals(pendingPayment3.getTransactions().get(0).getCurrency(), account.getCurrency());
+ Assert.assertEquals(pendingPayment3.getTransactions().get(0).getExternalKey(), payment3TransactionExternalKey);
+ Assert.assertEquals(pendingPayment3.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PENDING);
+ }
}
- // Example of a 3D secure payment for instance
@Test(groups = "slow")
public void testApiWithPendingPaymentTransaction() throws Exception {
for (final TransactionType transactionType : ImmutableList.<TransactionType>of(TransactionType.AUTHORIZE, TransactionType.PURCHASE, TransactionType.CREDIT)) {
