killbill-memoizeit

diff --git a/catalog/pom.xml b/catalog/pom.xml
index c4bac68..0978f4a 100644
--- a/catalog/pom.xml
+++ b/catalog/pom.xml
@@ -76,16 +76,16 @@
             <artifactId>killbill-util</artifactId>
         </dependency>
         <dependency>
-            <groupId>org.kill-bill.billing.plugin</groupId>
-            <artifactId>killbill-plugin-api-catalog</artifactId>
-        </dependency>
-        <dependency>
             <groupId>org.kill-bill.billing</groupId>
             <artifactId>killbill-util</artifactId>
             <type>test-jar</type>
             <scope>test</scope>
         </dependency>
         <dependency>
+            <groupId>org.kill-bill.billing.plugin</groupId>
+            <artifactId>killbill-plugin-api-catalog</artifactId>
+        </dependency>
+        <dependency>
             <groupId>org.kill-bill.commons</groupId>
             <artifactId>killbill-clock</artifactId>
         </dependency>

diff --git a/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java b/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
index 8e10283..ede05e3 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
@@ -31,16 +31,18 @@ import org.killbill.billing.osgi.api.OSGIServiceRegistration;
 import org.killbill.billing.payment.api.PluginProperty;
 import org.killbill.billing.payment.api.TransactionStatus;
 import org.killbill.billing.payment.core.PaymentTransactionInfoPluginConverter;
+import org.killbill.billing.payment.core.sm.PaymentControlStateMachineHelper;
 import org.killbill.billing.payment.core.sm.PaymentStateMachineHelper;
 import org.killbill.billing.payment.core.sm.PluginRoutingPaymentAutomatonRunner;
-import org.killbill.billing.payment.core.sm.PaymentControlStateMachineHelper;
 import org.killbill.billing.payment.dao.PaymentDao;
 import org.killbill.billing.payment.dao.PaymentMethodModelDao;
 import org.killbill.billing.payment.dao.PaymentModelDao;
 import org.killbill.billing.payment.dao.PaymentTransactionModelDao;
 import org.killbill.billing.payment.plugin.api.PaymentPluginApi;
 import org.killbill.billing.payment.plugin.api.PaymentPluginApiException;
+import org.killbill.billing.payment.plugin.api.PaymentPluginStatus;
 import org.killbill.billing.payment.plugin.api.PaymentTransactionInfoPlugin;
+import org.killbill.billing.payment.provider.DefaultNoOpPaymentInfoPlugin;
 import org.killbill.billing.util.callcontext.CallContext;
 import org.killbill.billing.util.callcontext.InternalCallContextFactory;
 import org.killbill.billing.util.config.PaymentConfig;
@@ -48,6 +50,7 @@ import org.killbill.clock.Clock;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 
@@ -59,7 +62,6 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
                                                                                                           .build();
     private final static int MAX_ITEMS_PER_LOOP = 100;
 
-
     public IncompletePaymentTransactionTask(final Janitor janitor, final InternalCallContextFactory internalCallContextFactory, final PaymentConfig paymentConfig,
                                             final PaymentDao paymentDao, final Clock clock,
                                             final PaymentStateMachineHelper paymentStateMachineHelper, final PaymentControlStateMachineHelper retrySMHelper, final AccountInternalApi accountInternalApi,
@@ -83,10 +85,19 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
         final CallContext callContext = createCallContext("IncompletePaymentTransactionTask", internalTenantContext);
         final PaymentModelDao payment = paymentDao.getPayment(paymentTransaction.getPaymentId(), internalTenantContext);
 
-
         final PaymentMethodModelDao paymentMethod = paymentDao.getPaymentMethod(payment.getPaymentMethodId(), internalTenantContext);
         final PaymentPluginApi paymentPluginApi = getPaymentPluginApi(payment, paymentMethod.getPluginName());
 
+
+        final PaymentTransactionInfoPlugin undefinedPaymentTransaction = new DefaultNoOpPaymentInfoPlugin(payment.getId(),
+                                                                                                     paymentTransaction.getId(),
+                                                                                                     paymentTransaction.getTransactionType(),
+                                                                                                     paymentTransaction.getAmount(),
+                                                                                                     paymentTransaction.getCurrency(),
+                                                                                                     paymentTransaction.getCreatedDate(),
+                                                                                                     paymentTransaction.getCreatedDate(),
+                                                                                                     PaymentPluginStatus.UNDEFINED,
+                                                                                                     null);
         PaymentTransactionInfoPlugin paymentTransactionInfoPlugin;
         try {
             final List<PaymentTransactionInfoPlugin> result = paymentPluginApi.getPaymentInfo(payment.getAccountId(), payment.getId(), ImmutableList.<PluginProperty>of(), callContext);
@@ -95,10 +106,14 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
                 public boolean apply(final PaymentTransactionInfoPlugin input) {
                     return input.getKbTransactionPaymentId().equals(paymentTransaction.getId());
                 }
-            }).orNull();
-        } catch (final PaymentPluginApiException ignored) {
-            // The paymentTransactionInfoPlugin will be null and handled below as if the plugin did not know about that transaction
-            paymentTransactionInfoPlugin = null;
+            }).or(new Supplier<PaymentTransactionInfoPlugin>() {
+                @Override
+                public PaymentTransactionInfoPlugin get() {
+                    return undefinedPaymentTransaction;
+                }
+            });
+        } catch (final Exception e) {
+            paymentTransactionInfoPlugin = undefinedPaymentTransaction;
         }
 
         //
@@ -121,7 +136,7 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
             case UNKNOWN:
             default:
                 log.info("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, bail early...",
-                         new Object[] {payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus});
+                         new Object[]{payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus});
                 // We can't get anything interesting from the plugin...
                 return;
         }
@@ -140,7 +155,7 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
         final String gatewayError = paymentTransactionInfoPlugin != null ? paymentTransactionInfoPlugin.getGatewayError() : paymentTransaction.getGatewayErrorMsg();
 
         log.info("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, transitioning transactionStatus from {} -> {}",
-                 new Object[] {payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus});
+                 new Object[]{payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus});
 
         final InternalCallContext internalCallContext = internalCallContextFactory.createInternalCallContext(payment.getAccountId(), callContext);
         paymentDao.updatePaymentAndTransactionOnCompletion(payment.getAccountId(), payment.getId(), paymentTransaction.getTransactionType(), newPaymentState, lastSuccessPaymentState,
@@ -149,13 +164,9 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
     }
 
     // Keep the existing currentTransactionStatus if we can't obtain a better answer from the plugin; if not, return the newTransactionStatus
-    private TransactionStatus computeNewTransactionStatusFromPaymentTransactionInfoPlugin(@Nullable PaymentTransactionInfoPlugin input, final TransactionStatus currentTransactionStatus) {
+    private TransactionStatus computeNewTransactionStatusFromPaymentTransactionInfoPlugin(PaymentTransactionInfoPlugin input, final TransactionStatus currentTransactionStatus) {
         final TransactionStatus newTransactionStatus = PaymentTransactionInfoPluginConverter.toTransactionStatus(input);
-        if (newTransactionStatus == TransactionStatus.PLUGIN_FAILURE || newTransactionStatus ==  TransactionStatus.UNKNOWN) {
-            return currentTransactionStatus;
-        } else {
-            return newTransactionStatus;
-        }
+        return (newTransactionStatus != TransactionStatus.UNKNOWN) ? newTransactionStatus : currentTransactionStatus;
     }
 
     private boolean isPendingOrFinalTransactionStatus(final TransactionStatus transactionStatus) {
@@ -175,7 +186,6 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
         return clock.getUTCNow().minusMillis((int) delayBeforeNowMs);
     }
 
-
     private DateTime getCreatedDateAfter() {
         final long delayBeforeNowMs = paymentConfig.getIncompleteTransactionsTimeSpanGiveup().getMillis();
         return clock.getUTCNow().minusMillis((int) delayBeforeNowMs);

diff --git a/payment/src/main/java/org/killbill/billing/payment/core/PaymentTransactionInfoPluginConverter.java b/payment/src/main/java/org/killbill/billing/payment/core/PaymentTransactionInfoPluginConverter.java
index d60d540..bcdc703 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/PaymentTransactionInfoPluginConverter.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/PaymentTransactionInfoPluginConverter.java
@@ -28,34 +28,32 @@ import org.killbill.billing.payment.plugin.api.PaymentTransactionInfoPlugin;
 //
 public class PaymentTransactionInfoPluginConverter {
 
-    public static TransactionStatus toTransactionStatus(@Nullable final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
-        //
-        // paymentTransactionInfoPlugin when we got an exception from the plugin, or if the plugin behaves badly
-        // and decides to return null; in all cases this is seen as a PLUGIN_FAILURE
-        //
-        if (paymentTransactionInfoPlugin == null || paymentTransactionInfoPlugin.getStatus() == null) {
-            return TransactionStatus.PLUGIN_FAILURE;
-        }
 
+    public static TransactionStatus toTransactionStatus(final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
         switch (paymentTransactionInfoPlugin.getStatus()) {
             case PROCESSED:
                 return TransactionStatus.SUCCESS;
             case PENDING:
                 return TransactionStatus.PENDING;
+            // The naming is a bit inconsistent, but ERROR on the plugin side means PAYMENT_FAILURE (that is a case where transaction went through but did not
+            // return successfully (e.g: CC denied, ...)
             case ERROR:
                 return TransactionStatus.PAYMENT_FAILURE;
+            //
+            // The plugin is trying to tell us that it knows for sure that payment transaction did not happen (connection failure,..)
+            case CANCELED:
+                return TransactionStatus.PLUGIN_FAILURE;
+            //
             // This will be picked up by Janitor to figure out what really happened and correct the state if needed
+            // Note that the default case includes the null status
+            //
             case UNDEFINED:
             default:
                 return TransactionStatus.UNKNOWN;
         }
     }
 
-    public static OperationResult toOperationResult(@Nullable final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
-        if (paymentTransactionInfoPlugin == null || paymentTransactionInfoPlugin.getStatus() == null) {
-            return OperationResult.EXCEPTION;
-        }
-
+    public static OperationResult toOperationResult(final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
         switch (paymentTransactionInfoPlugin.getStatus()) {
             case PROCESSED:
                 return OperationResult.SUCCESS;
@@ -63,8 +61,8 @@ public class PaymentTransactionInfoPluginConverter {
                 return OperationResult.PENDING;
             case ERROR:
                 return OperationResult.FAILURE;
-            // Might change later if Janitor fixes the state
             case UNDEFINED:
+            case CANCELED:
             default:
                 return OperationResult.EXCEPTION;
         }

diff --git a/payment/src/main/java/org/killbill/billing/payment/core/ProcessorBase.java b/payment/src/main/java/org/killbill/billing/payment/core/ProcessorBase.java
index a411e2c..77a1265 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/ProcessorBase.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/ProcessorBase.java
@@ -174,8 +174,7 @@ public abstract class ProcessorBase {
         final PaymentTransactionModelDao transactionAlreadyExists = Iterables.tryFind(transactions, new Predicate<PaymentTransactionModelDao>() {
             @Override
             public boolean apply(final PaymentTransactionModelDao input) {
-                return input.getTransactionStatus() == TransactionStatus.SUCCESS ||
-                       input.getTransactionStatus() == TransactionStatus.UNKNOWN;
+                return input.getTransactionStatus() == TransactionStatus.SUCCESS;
             }
         }).orNull();
         if (transactionAlreadyExists != null) {

diff --git a/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentOperation.java b/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentOperation.java
index 9f989f3..6781ecd 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentOperation.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentOperation.java
@@ -45,7 +45,7 @@ import org.killbill.billing.payment.provider.DefaultNoOpPaymentInfoPlugin;
 import org.killbill.commons.locker.GlobalLocker;
 import org.killbill.commons.locker.LockFailedException;
 
-import com.google.common.base.Objects;
+import com.google.common.base.MoreObjects;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
@@ -74,25 +74,24 @@ public abstract class PaymentOperation extends OperationCallbackBase<PaymentTran
             } else {
                 return doSimpleOperationCallback();
             }
-        } catch (final PaymentApiException e) {
-            throw new OperationException(e, OperationResult.EXCEPTION);
+        } catch (final Exception e) {
+            throw convertToUnknownTransactionStatusAndErroredPaymentState(e);
         }
     }
 
     @Override
     protected OperationException rewrapExecutionException(final PaymentStateContext paymentStateContext, final ExecutionException e) {
-        final Throwable realException = Objects.firstNonNull(e.getCause(), e);
-        if (e.getCause() instanceof PaymentApiException) {
-            logger.warn("Unsuccessful plugin call for account {}", paymentStateContext.getAccount().getExternalKey(), realException);
-            return convertToUnknownTransactionStatusAndErroredPaymentState(realException);
-        } else if (e.getCause() instanceof LockFailedException) {
-            final String format = String.format("Failed to lock account %s", paymentStateContext.getAccount().getExternalKey());
-            logger.error(String.format(format));
-            return new OperationException(realException, OperationResult.EXCEPTION);
-        } else /* if (e instanceof RuntimeException) */ {
-            logger.warn("Plugin call threw an exception for account {}", paymentStateContext.getAccount().getExternalKey(), e);
-            return new OperationException(realException, OperationResult.EXCEPTION);
+        //
+        // Any case of exception (checked or runtime) should lead to a TransactionStatus.UNKNOWN (and a XXX_ERRORED payment state).
+        // In order to reach that state we create PaymentTransactionInfoPlugin with an PaymentPluginStatus.UNDEFINED status (and an OperationResult.EXCEPTION).
+        //
+        final Throwable originalExceptionOrCause = MoreObjects.firstNonNull(e.getCause(), e);
+        if (originalExceptionOrCause instanceof LockFailedException) {
+            logger.warn("Failed to lock account {}", paymentStateContext.getAccount().getExternalKey());
+        } else {
+            logger.warn("Payment plugin call threw an exception for account {}", paymentStateContext.getAccount().getExternalKey(), originalExceptionOrCause);
         }
+        return convertToUnknownTransactionStatusAndErroredPaymentState(originalExceptionOrCause);
     }
 
     @Override
@@ -102,7 +101,7 @@ public abstract class PaymentOperation extends OperationCallbackBase<PaymentTran
     }
 
     //
-    // In the case we don't know exactly what happen (Timeout or PluginApiException):
+    // In case of exceptions, timeouts, we don't really know what happened:
     // - Return an OperationResult.EXCEPTION to transition Payment State to Errored (see PaymentTransactionInfoPluginConverter#toOperationResult)
     // - Construct a PaymentTransactionInfoPlugin whose PaymentPluginStatus = UNDEFINED to end up with a paymentTransactionStatus = UNKNOWN and have a chance to
     //   be fixed by Janitor.
@@ -181,8 +180,14 @@ public abstract class PaymentOperation extends OperationCallbackBase<PaymentTran
             //
             if (paymentStateContext.getOverridePluginOperationResult() == null) {
                 final PaymentTransactionInfoPlugin paymentInfoPlugin = doCallSpecificOperationCallback();
-                // Throws if plugin is  ot correctly implemented (e.g returns null result, values,..)
-                sanityOnPaymentInfoPlugin(paymentInfoPlugin);
+                //
+                // We catch null paymentInfoPlugin and throw a RuntimeException to end up in an UNKNOWN transactionStatus
+                // That way we can use the null paymentInfoPlugin when a PaymentPluginApiException is thrown and correctly
+                // make the transition to PLUGIN_FAILURE
+                //
+                if (paymentInfoPlugin == null) {
+                    throw new IllegalStateException("Payment plugin returned a null result");
+                }
 
                 paymentStateContext.setPaymentTransactionInfoPlugin(paymentInfoPlugin);
                 return PaymentTransactionInfoPluginConverter.toOperationResult(paymentStateContext.getPaymentTransactionInfoPlugin());
@@ -217,22 +222,4 @@ public abstract class PaymentOperation extends OperationCallbackBase<PaymentTran
                 return PaymentPluginStatus.UNDEFINED;
         }
     }
-
-    private void sanityOnPaymentInfoPlugin(final PaymentTransactionInfoPlugin paymentInfoPlugin) throws PaymentApiException {
-        if (paymentInfoPlugin == null) {
-            throw new PaymentApiException(ErrorCode.PAYMENT_PLUGIN_EXCEPTION, "Payment plugin returned a null result");
-        }
-        /*
-        TODO this breaks our tests so test would have to be fixed
-        if (paymentTransactionInfoPlugin.getKbTransactionPaymentId() == null || !paymentTransactionInfoPlugin.getKbTransactionPaymentId().equals(paymentStateContext.getTransactionId())) {
-            throw new PaymentApiException(ErrorCode.PAYMENT_PLUGIN_EXCEPTION, "Payment plugin returned invalid kbTransactionId");
-        }
-        if (paymentTransactionInfoPlugin.getKbPaymentId() == null || !paymentTransactionInfoPlugin.getKbPaymentId().equals(paymentStateContext.getPaymentId())) {
-            throw new PaymentApiException(ErrorCode.PAYMENT_PLUGIN_EXCEPTION, "Payment plugin returned invalid kbPaymentId");
-        }
-        if (paymentTransactionInfoPlugin.getTransactionType() == null || !paymentTransactionInfoPlugin.getKbPaymentId().equals(paymentStateContext.getTransactionType())) {
-            throw new PaymentApiException(ErrorCode.PAYMENT_PLUGIN_EXCEPTION, "Payment plugin returned invalid transaction type");
-        }
-        */
-    }
 }

diff --git a/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentRoutingPluginApi.java b/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentRoutingPluginApi.java
index e1b5ddf..30d5115 100644
--- a/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentRoutingPluginApi.java
+++ b/payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentRoutingPluginApi.java
@@ -385,7 +385,8 @@ public final class InvoicePaymentRoutingPluginApi implements PaymentRoutingPlugi
     private DateTime getNextRetryDateForPluginFailure(final List<PaymentTransactionModelDao> purchasedTransactions) {
 
         DateTime result = null;
-        final int attemptsInState = getNumberAttemptsInState(purchasedTransactions, TransactionStatus.PLUGIN_FAILURE);
+        // Not clear whether or not we should really include TransactionStatus.UNKNOWN
+        final int attemptsInState = getNumberAttemptsInState(purchasedTransactions, TransactionStatus.PLUGIN_FAILURE, TransactionStatus.UNKNOWN);
         final int retryAttempt = (attemptsInState - 1) >= 0 ? (attemptsInState - 1) : 0;
 
         if (retryAttempt < paymentConfig.getPluginFailureRetryMaxAttempts()) {

diff --git a/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentEnteringStateCallback.java b/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentEnteringStateCallback.java
index c4f53e6..e146a6e 100644
--- a/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentEnteringStateCallback.java
+++ b/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentEnteringStateCallback.java
@@ -105,18 +105,6 @@ public class TestPaymentEnteringStateCallback extends PaymentTestSuiteWithEmbedd
         Assert.assertEquals(paymentTransaction.getGatewayErrorMsg(), paymentInfoPlugin.getGatewayError());
     }
 
-    @Test(groups = "slow")
-    public void testEnterStateWithOperationException() throws Exception {
-        daoHelper.createNewPaymentTransaction();
-        // Simulate a bug in the plugin - i.e. nothing was returned
-        paymentStateContext.setPaymentTransactionInfoPlugin(null);
-        operationResult = OperationResult.EXCEPTION;
-
-        callback.enteringState(state, operationCallback, operationResult, leavingStateCallback);
-
-        Assert.assertEquals(paymentDao.getPaymentTransaction(paymentStateContext.getPaymentTransactionModelDao().getId(), internalCallContext).getTransactionStatus(), TransactionStatus.PLUGIN_FAILURE);
-    }
-
     private static final class PaymentEnteringStateTestCallback extends PaymentEnteringStateCallback {
 
         private PaymentEnteringStateTestCallback(final PaymentAutomatonDAOHelper daoHelper, final PaymentStateContext paymentStateContext) throws PaymentApiException {

diff --git a/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentOperation.java b/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentOperation.java
index 9f6eb58..586e759 100644
--- a/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentOperation.java
+++ b/payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentOperation.java
@@ -76,7 +76,8 @@ public class TestPaymentOperation extends PaymentTestSuiteNoDB {
             Assert.assertEquals(e.getOperationResult(), OperationResult.EXCEPTION);
         }
 
-        Assert.assertNull(paymentStateContext.getPaymentTransactionInfoPlugin());
+        Assert.assertNotNull(paymentStateContext.getPaymentTransactionInfoPlugin());
+        Assert.assertEquals(paymentStateContext.getPaymentTransactionInfoPlugin().getStatus(), PaymentPluginStatus.UNDEFINED);
     }
 
     @Test(groups = "fast")