Repository: killbill/killbill
Issue: Possible double payments with invoice payments in UNKNOWN state

Scenario:

1. Invoice payment in `UNKNOWN` state
2. Trigger a new successful payment by calling `invoicePaymentApi.createPurchaseForInvoicePayment`
3. Fix the original payment (`UNKNOWN -> SUCCESS`)

Result: the invoice ends up double payed and the invoice balance is negative (absolute value is the amount of the payment).


Changed files:
beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java
invoice/src/test/java/org/killbill/billing/invoice/dao/MockInvoiceDao.java
invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java
api/src/main/java/org/killbill/billing/invoice/api/InvoiceInternalApi.java
invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDao.java
invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java
payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java

diff --git a/api/src/main/java/org/killbill/billing/invoice/api/InvoiceInternalApi.java b/api/src/main/java/org/killbill/billing/invoice/api/InvoiceInternalApi.java
index ffe6ad5e9a..e6534c81d5 100644
--- a/api/src/main/java/org/killbill/billing/invoice/api/InvoiceInternalApi.java
+++ b/api/src/main/java/org/killbill/billing/invoice/api/InvoiceInternalApi.java
@@ -84,5 +84,7 @@ public interface InvoiceInternalApi {
 
     public List<InvoicePayment> getInvoicePaymentsByAccount(UUID accountId, TenantContext context);
 
+    public List<InvoicePayment> getInvoicePaymentsByInvoice(UUID invoiceId, InternalTenantContext context);
+
     public InvoicePayment getInvoicePaymentByCookieId(String cookieId, TenantContext context);
 }
diff --git a/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java b/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java
index b00c5e308a..4eca162eba 100644
--- a/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java
+++ b/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java
@@ -1095,4 +1095,139 @@ public class TestInvoicePayment extends TestIntegrationBase {
         Assert.assertEquals(payments.get(0).getTransactions().size(), 1);
 
     }
+
+    @Test(groups = "slow")
+    public void testWithUNKNOWNPaymentFixedToSuccess() throws Exception {
+        // Verify integration with Overdue in that particular test
+        final String configXml = "<overdueConfig>" +
+                                 "   <accountOverdueStates>" +
+                                 "       <initialReevaluationInterval>" +
+                                 "           <unit>DAYS</unit><number>1</number>" +
+                                 "       </initialReevaluationInterval>" +
+                                 "       <state name=\\"OD1\\">" +
+                                 "           <condition>" +
+                                 "               <timeSinceEarliestUnpaidInvoiceEqualsOrExceeds>" +
+                                 "                   <unit>DAYS</unit><number>1</number>" +
+                                 "               </timeSinceEarliestUnpaidInvoiceEqualsOrExceeds>" +
+                                 "           </condition>" +
+                                 "           <externalMessage>Reached OD1</externalMessage>" +
+                                 "           <blockChanges>true</blockChanges>" +
+                                 "           <disableEntitlementAndChangesBlocked>false</disableEntitlementAndChangesBlocked>" +
+                                 "       </state>" +
+                                 "   </accountOverdueStates>" +
+                                 "</overdueConfig>";
+        final InputStream is = new ByteArrayInputStream(configXml.getBytes());
+        final DefaultOverdueConfig config = XMLLoader.getObjectFromStreamNoValidation(is, DefaultOverdueConfig.class);
+        overdueConfigCache.loadDefaultOverdueConfig(config);
+
+        clock.setDay(new LocalDate(2012, 4, 1));
+
+        final AccountData accountData = getAccountData(1);
+        final Account account = createAccountWithNonOsgiPaymentMethod(accountData);
+        accountChecker.checkAccount(account.getId(), accountData, callContext);
+
+        checkODState(OverdueWrapper.CLEAR_STATE_NAME, account.getId());
+
+        paymentPlugin.makeNextPaymentUnknown();
+
+        final DefaultEntitlement baseEntitlement = createBaseEntitlementAndCheckForCompletion(account.getId(), "bundleKey", "Shotgun", ProductCategory.BASE, BillingPeriod.MONTHLY, NextEvent.CREATE, NextEvent.BLOCK, NextEvent.INVOICE);
+
+        addDaysAndCheckForCompletion(30, NextEvent.PHASE, NextEvent.INVOICE, NextEvent.PAYMENT_PLUGIN_ERROR, NextEvent.INVOICE_PAYMENT_ERROR);
+
+        invoiceChecker.checkChargedThroughDate(baseEntitlement.getId(), new LocalDate(2012, 6, 1), callContext);
+
+        final List<Invoice> invoices = invoiceUserApi.getInvoicesByAccount(account.getId(), false, false, callContext);
+        assertEquals(invoices.size(), 2);
+
+        final Invoice invoice1 = invoices.get(0).getInvoiceItems().get(0).getInvoiceItemType() == InvoiceItemType.RECURRING ?
+                                 invoices.get(0) : invoices.get(1);
+        assertTrue(invoice1.getBalance().compareTo(new BigDecimal("249.95")) == 0);
+        assertTrue(invoice1.getPaidAmount().compareTo(BigDecimal.ZERO) == 0);
+        assertTrue(invoice1.getChargedAmount().compareTo(new BigDecimal("249.95")) == 0);
+        assertEquals(invoice1.getPayments().size(), 1);
+        assertEquals(invoice1.getPayments().get(0).getAmount().compareTo(BigDecimal.ZERO), 0);
+        assertEquals(invoice1.getPayments().get(0).getCurrency(), Currency.USD);
+        assertFalse(invoice1.getPayments().get(0).isSuccess());
+        assertNotNull(invoice1.getPayments().get(0).getPaymentId());
+
+        final BigDecimal accountBalance1 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
+        assertTrue(accountBalance1.compareTo(new BigDecimal("249.95")) == 0);
+
+        final List<Payment> payments = paymentApi.getAccountPayments(account.getId(), false, true, ImmutableList.<PluginProperty>of(), callContext);
+        assertEquals(payments.size(), 1);
+        assertEquals(payments.get(0).getPurchasedAmount().compareTo(BigDecimal.ZERO), 0);
+        assertEquals(payments.get(0).getTransactions().size(), 1);
+        assertEquals(payments.get(0).getTransactions().get(0).getAmount().compareTo(new BigDecimal("249.95")), 0);
+        assertEquals(payments.get(0).getTransactions().get(0).getCurrency(), Currency.USD);
+        assertEquals(payments.get(0).getTransactions().get(0).getProcessedAmount().compareTo(BigDecimal.ZERO), 0);
+        assertEquals(payments.get(0).getTransactions().get(0).getProcessedCurrency(), Currency.USD);
+        assertEquals(payments.get(0).getTransactions().get(0).getTransactionStatus(), TransactionStatus.UNKNOWN);
+        assertEquals(payments.get(0).getPaymentAttempts().size(), 1);
+        assertEquals(payments.get(0).getPaymentAttempts().get(0).getPluginName(), InvoicePaymentControlPluginApi.PLUGIN_NAME);
+        assertEquals(payments.get(0).getPaymentAttempts().get(0).getStateName(), "ABORTED");
+
+        // Verify account transitions to OD1
+        addDaysAndCheckForCompletion(2, NextEvent.BLOCK);
+        checkODState("OD1", account.getId());
+
+        // Verify we cannot trigger double payments
+        try {
+            invoicePaymentApi.createPurchaseForInvoicePayment(account,
+                                                              invoice1.getId(),
+                                                              payments.get(0).getPaymentMethodId(),
+                                                              null,
+                                                              invoice1.getBalance(),
+                                                              invoice1.getCurrency(),
+                                                              clock.getUTCNow(),
+                                                              null,
+                                                              null,
+                                                              ImmutableList.<PluginProperty>of(),
+                                                              PAYMENT_OPTIONS,
+                                                              callContext);
+            Assert.fail();
+        } catch (final PaymentApiException e) {
+            Assert.assertEquals(e.getCode(), ErrorCode.PAYMENT_PLUGIN_API_ABORTED.getCode());
+            assertListenerStatus();
+        }
+
+        // Transition the payment to success
+        final PaymentTransaction existingPaymentTransaction = payments.get(0).getTransactions().get(0);
+        final PaymentTransaction updatedPaymentTransaction = Mockito.mock(PaymentTransaction.class);
+        Mockito.when(updatedPaymentTransaction.getId()).thenReturn(existingPaymentTransaction.getId());
+        Mockito.when(updatedPaymentTransaction.getExternalKey()).thenReturn(existingPaymentTransaction.getExternalKey());
+        Mockito.when(updatedPaymentTransaction.getTransactionType()).thenReturn(existingPaymentTransaction.getTransactionType());
+        Mockito.when(updatedPaymentTransaction.getProcessedAmount()).thenReturn(new BigDecimal("249.95"));
+        Mockito.when(updatedPaymentTransaction.getProcessedCurrency()).thenReturn(existingPaymentTransaction.getCurrency());
+        busHandler.pushExpectedEvents(NextEvent.PAYMENT, NextEvent.INVOICE_PAYMENT, NextEvent.BLOCK);
+        adminPaymentApi.fixPaymentTransactionState(payments.get(0), updatedPaymentTransaction, TransactionStatus.SUCCESS, null, null, ImmutableList.<PluginProperty>of(), callContext);
+        assertListenerStatus();
+
+        checkODState(OverdueWrapper.CLEAR_STATE_NAME, account.getId());
+
+        final Invoice invoice2 = invoiceUserApi.getInvoice(invoice1.getId(), callContext);
+        assertTrue(invoice2.getBalance().compareTo(BigDecimal.ZERO) == 0);
+        assertTrue(invoice2.getPaidAmount().compareTo(new BigDecimal("249.95")) == 0);
+        assertTrue(invoice2.getChargedAmount().compareTo(new BigDecimal("249.95")) == 0);
+        assertEquals(invoice2.getPayments().size(), 1);
+        assertEquals(invoice2.getPayments().get(0).getAmount().compareTo(new BigDecimal("249.95")), 0);
+        assertEquals(invoice2.getPayments().get(0).getCurrency(), Currency.USD);
+        assertTrue(invoice2.getPayments().get(0).isSuccess());
+        assertNotNull(invoice2.getPayments().get(0).getPaymentId());
+
+        final BigDecimal accountBalance2 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
+        assertTrue(accountBalance2.compareTo(BigDecimal.ZERO) == 0);
+
+        final List<Payment> payments2 = paymentApi.getAccountPayments(account.getId(), false, true, ImmutableList.<PluginProperty>of(), callContext);
+        assertEquals(payments2.size(), 1);
+        assertEquals(payments2.get(0).getPurchasedAmount().compareTo(new BigDecimal("249.95")), 0);
+        assertEquals(payments2.get(0).getTransactions().size(), 1);
+        assertEquals(payments2.get(0).getTransactions().get(0).getAmount().compareTo(new BigDecimal("249.95")), 0);
+        assertEquals(payments2.get(0).getTransactions().get(0).getCurrency(), Currency.USD);
+        assertEquals(payments2.get(0).getTransactions().get(0).getProcessedAmount().compareTo(new BigDecimal("249.95")), 0);
+        assertEquals(payments2.get(0).getTransactions().get(0).getProcessedCurrency(), Currency.USD);
+        assertEquals(payments2.get(0).getTransactions().get(0).getTransactionStatus(), TransactionStatus.SUCCESS);
+        assertEquals(payments2.get(0).getPaymentAttempts().size(), 1);
+        assertEquals(payments2.get(0).getPaymentAttempts().get(0).getPluginName(), InvoicePaymentControlPluginApi.PLUGIN_NAME);
+        assertEquals(payments2.get(0).getPaymentAttempts().get(0).getStateName(), "SUCCESS");
+    }
 }
diff --git a/invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java b/invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java
index 0f2cba226c..1b9e082849 100644
--- a/invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java
+++ b/invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java
@@ -217,6 +217,18 @@ public class DefaultInvoiceInternalApi implements InvoiceInternalApi {
                                                                           ));
     }
 
+    @Override
+    public List<InvoicePayment> getInvoicePaymentsByInvoice(final UUID invoiceId, final InternalTenantContext context) {
+        return ImmutableList.<InvoicePayment>copyOf(Collections2.transform(dao.getInvoicePaymentsByInvoice(invoiceId, context),
+                                                                           new Function<InvoicePaymentModelDao, InvoicePayment>() {
+                                                                               @Override
+                                                                               public InvoicePayment apply(final InvoicePaymentModelDao input) {
+                                                                                   return new DefaultInvoicePayment(input);
+                                                                               }
+                                                                           }
+                                                                          ));
+    }
+
     @Override
     public InvoicePayment getInvoicePaymentByCookieId(final String cookieId, final TenantContext context) {
         final InvoicePaymentModelDao invoicePaymentModelDao = dao.getInvoicePaymentByCookieId(cookieId, internalCallContextFactory.createInternalTenantContext(context.getAccountId(), ObjectType.ACCOUNT, context));
diff --git a/invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java b/invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java
index 963e6c9cf4..cc48631f72 100644
--- a/invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java
+++ b/invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java
@@ -559,6 +559,16 @@ public class DefaultInvoiceDao extends EntityDaoBase<InvoiceModelDao, Invoice, I
         });
     }
 
+    @Override
+    public List<InvoicePaymentModelDao> getInvoicePaymentsByInvoice(final UUID invoiceId, final InternalTenantContext context) {
+        return transactionalSqlDao.execute(true, new EntitySqlDaoTransactionWrapper<List<InvoicePaymentModelDao>>() {
+            @Override
+            public List<InvoicePaymentModelDao> inTransaction(final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFactory) throws Exception {
+                return entitySqlDaoWrapperFactory.become(InvoicePaymentSqlDao.class).getAllPaymentsForInvoiceIncludedInit(invoiceId.toString(), context);
+            }
+        });
+    }
+
     @Override
     public InvoicePaymentModelDao getInvoicePaymentByCookieId(final String cookieId, final InternalTenantContext context) {
         return transactionalSqlDao.execute(true, new EntitySqlDaoTransactionWrapper<InvoicePaymentModelDao>() {
diff --git a/invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDao.java b/invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDao.java
index 8d4f871c8b..e7c77901b5 100644
--- a/invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDao.java
+++ b/invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDao.java
@@ -66,6 +66,8 @@ public interface InvoiceDao extends EntityDao<InvoiceModelDao, Invoice, InvoiceA
 
     List<InvoicePaymentModelDao> getInvoicePaymentsByAccount(InternalTenantContext context);
 
+    List<InvoicePaymentModelDao> getInvoicePaymentsByInvoice(final UUID invoiceId, InternalTenantContext context);
+
     InvoicePaymentModelDao getInvoicePaymentByCookieId(String cookieId, InternalTenantContext internalTenantContext);
 
     BigDecimal getAccountBalance(UUID accountId, InternalTenantContext context);
diff --git a/invoice/src/test/java/org/killbill/billing/invoice/dao/MockInvoiceDao.java b/invoice/src/test/java/org/killbill/billing/invoice/dao/MockInvoiceDao.java
index b51ee616f0..32499dff55 100644
--- a/invoice/src/test/java/org/killbill/billing/invoice/dao/MockInvoiceDao.java
+++ b/invoice/src/test/java/org/killbill/billing/invoice/dao/MockInvoiceDao.java
@@ -233,6 +233,19 @@ public class MockInvoiceDao extends MockEntityDaoBase<InvoiceModelDao, Invoice,
         return result;
     }
 
+    @Override
+    public List<InvoicePaymentModelDao> getInvoicePaymentsByInvoice(final UUID invoiceId, final InternalTenantContext context) {
+        final List<InvoicePaymentModelDao> result = new LinkedList<InvoicePaymentModelDao>();
+        synchronized (monitor) {
+            for (final InvoicePaymentModelDao payment : payments.values()) {
+                if (invoiceId.equals(payment.getInvoiceId())) {
+                    result.add(payment);
+                }
+            }
+        }
+        return result;
+    }
+
     @Override
     public List<InvoicePaymentModelDao> getInvoicePaymentsByAccount(final InternalTenantContext context) {
 
diff --git a/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java b/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java
index ecb7dd218c..01277a9b92 100644
--- a/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java
+++ b/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java
@@ -55,6 +55,7 @@ import org.killbill.billing.payment.api.PaymentApiException;
 import org.killbill.billing.payment.api.PluginProperty;
 import org.killbill.billing.payment.api.TransactionStatus;
 import org.killbill.billing.payment.api.TransactionType;
+import org.killbill.billing.payment.core.janitor.IncompletePaymentTransactionTask;
 import org.killbill.billing.payment.dao.PaymentDao;
 import org.killbill.billing.payment.dao.PaymentModelDao;
 import org.killbill.billing.payment.dao.PaymentTransactionModelDao;
@@ -321,7 +322,6 @@ public final class InvoicePaymentControlPluginApi implements PaymentControlPlugi
                 return new DefaultPriorPaymentControlResult(true);
             }
 
-
             // Get account and check if it is child and payment is delegated to parent => abort
 
             final AccountData accountData = accountApi.getAccountById(invoice.getAccountId(), internalContext);
@@ -340,7 +340,7 @@ public final class InvoicePaymentControlPluginApi implements PaymentControlPlugi
 
             // Do we have a  paymentMethod ?
             if (paymentControlPluginContext.getPaymentMethodId() == null) {
-                log.warn("Payment for invoiceId='{}' was not triggered, accountId='{}' doesn't have a default payment method", getInvoiceId(pluginProperties), paymentControlPluginContext.getAccountId());
+                log.warn("Payment for invoiceId='{}' was not triggered, accountId='{}' doesn't have a default payment method", invoiceId, paymentControlPluginContext.getAccountId());
                 invoiceApi.recordPaymentAttemptCompletion(invoiceId,
                                                           paymentControlPluginContext.getAmount(),
                                                           paymentControlPluginContext.getCurrency(),
@@ -359,6 +359,16 @@ public final class InvoicePaymentControlPluginApi implements PaymentControlPlugi
                 return new DefaultPriorPaymentControlResult(true);
             }
 
+            final List<InvoicePayment> existingInvoicePayments = invoiceApi.getInvoicePaymentsByInvoice(invoiceId, internalContext);
+            for (final InvoicePayment existingInvoicePayment : existingInvoicePayments) {
+                final List<PaymentTransactionModelDao> existingTransactions = paymentDao.getPaymentTransactionsByExternalKey(existingInvoicePayment.getPaymentCookieId(), internalContext);
+                for (final PaymentTransactionModelDao existingTransaction : existingTransactions) {
+                    if (existingTransaction.getTransactionStatus() == TransactionStatus.UNKNOWN) {
+                        log.warn("Existing paymentTransactionId='{}' for invoiceId='{}' in UNKNOWN state", existingTransaction.getId(), invoiceId);
+                        return new DefaultPriorPaymentControlResult(true);
+                    }
+                }
+            }
 
             //
             // Insert attempt row with a success = false status to implement a two-phase commit strategy and guard against scenario where payment would go through
diff --git a/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java b/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
index 32bba1c8da..78004a59c5 100644
--- a/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
+++ b/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014-2016 Groupon, Inc
- * Copyright 2014-2016 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -76,6 +76,7 @@ public class MockPaymentProviderPlugin implements PaymentPluginApi {
     private final AtomicBoolean makeNextPaymentFailWithException = new AtomicBoolean(false);
     private final AtomicBoolean makeAllPaymentsFailWithError = new AtomicBoolean(false);
     private final AtomicBoolean makeNextPaymentPending = new AtomicBoolean(false);
+    private final AtomicBoolean makeNextPaymentUnknown = new AtomicBoolean(false);
     private final AtomicInteger makePluginWaitSomeMilliseconds = new AtomicInteger(0);
     private final AtomicReference<BigDecimal> overrideNextProcessedAmount = new AtomicReference<BigDecimal>();
     private final AtomicReference<Currency> overrideNextProcessedCurrency = new AtomicReference<Currency>();
@@ -206,6 +207,7 @@ public class MockPaymentProviderPlugin implements PaymentPluginApi {
         makeNextPaymentFailWithError.set(false);
         makeNextPaymentFailWithCancellation.set(false);
         makeNextPaymentPending.set(false);
+        makeNextPaymentUnknown.set(false);
         makePluginWaitSomeMilliseconds.set(0);
         overrideNextProcessedAmount.set(null);
         paymentMethods.clear();
@@ -222,6 +224,10 @@ public class MockPaymentProviderPlugin implements PaymentPluginApi {
         makeNextPaymentPending.set(true);
     }
 
+    public void makeNextPaymentUnknown() {
+        makeNextPaymentUnknown.set(true);
+    }
+
     public void makeNextPaymentFailWithCancellation() {
         makeNextPaymentFailWithCancellation.set(true);
     }
@@ -465,6 +471,8 @@ public class MockPaymentProviderPlugin implements PaymentPluginApi {
             status = PaymentPluginStatus.CANCELED;
         } else if (makeNextPaymentPending.getAndSet(false)) {
             status = PaymentPluginStatus.PENDING;
+        } else if (makeNextPaymentUnknown.getAndSet(false)) {
+            status = PaymentPluginStatus.UNDEFINED;
         } else {
             status = PaymentPluginStatus.PROCESSED;
         }