Payment handle new PaymentPluginStatus.CANCELED and simplify the exce…

…ption logic

payment/src/main/java/org/killbill/billing/payment/core/PaymentTransactionInfoPluginConverter.java

@@ -29,15 +29,7 @@
 public class PaymentTransactionInfoPluginConverter {
 
 
-    public static TransactionStatus toTransactionStatus(@Nullable final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
-        //
-        // A null paymentTransactionInfoPlugin means that plugin threw a PluginApiException to indicate it knew for sure
-        // the transaction did not occur, so we can safely transition into PLUGIN_FAILURE
-        //
-        if (paymentTransactionInfoPlugin == null) {
-            return TransactionStatus.PLUGIN_FAILURE;
-        }
-
+    public static TransactionStatus toTransactionStatus(final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin) {
         switch (paymentTransactionInfoPlugin.getStatus()) {
             case PROCESSED:
                 return TransactionStatus.SUCCESS;
@@ -48,6 +40,10 @@ public static TransactionStatus toTransactionStatus(@Nullable final PaymentTrans
             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
             //
@@ -57,20 +53,16 @@ public static TransactionStatus toTransactionStatus(@Nullable final PaymentTrans
         }
     }
 
-    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;
             case PENDING:
                 return OperationResult.PENDING;
             case ERROR:
                 return OperationResult.FAILURE;
-            // Might change later if janitor fixes the state
             case UNDEFINED:
+            case CANCELED:
             default:
                 return OperationResult.EXCEPTION;
         }

payment/src/main/java/org/killbill/billing/payment/core/ProcessorBase.java

@@ -174,8 +174,7 @@ protected void validateUniqueTransactionExternalKey(@Nullable final String trans
         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) {

payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java

@@ -112,11 +112,7 @@ public PaymentTransactionInfoPlugin get() {
                     return undefinedPaymentTransaction;
                 }
             });
-        } catch (final PaymentPluginApiException ignored) {
-            // TODO STEPH: In the case of a payment call, PaymentPluginApiException means that plugin knows payment transaction WAS NOT EVEN attempted; what does it mean for the getPaymentInfo?
-            // The code below assumes the same, but that needs to be discussed
-            paymentTransactionInfoPlugin = null;
-        } catch (final RuntimeException e) {
+        } catch (final Exception e) {
             paymentTransactionInfoPlugin = undefinedPaymentTransaction;
         }
 
@@ -168,7 +164,7 @@ public PaymentTransactionInfoPlugin get() {
     }
 
     // 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);
         return (newTransactionStatus != TransactionStatus.UNKNOWN) ? newTransactionStatus : currentTransactionStatus;
     }

payment/src/main/java/org/killbill/billing/payment/core/sm/payments/PaymentOperation.java

@@ -74,30 +74,24 @@ public OperationResult doOperationCallback() throws OperationException {
             } 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) {
         //
-        // There are 2 main cases to distinguish:
-        // - PaymentPluginApiException (hidden in the PaymentApiException) -> plugin is telling us transaction was not even attempted => TransactionStatus.PLUGIN_FAILURE
-        // - All other exceptions => TransactionStatus.UNKNOWN (Candidates to be fixed by Janitor)
+        // 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).
         //
-        if (e.getCause() instanceof PaymentApiException && ((PaymentApiException)e.getCause()).getCode() == ErrorCode.PAYMENT_PLUGIN_EXCEPTION.getCode()) {
-            // We keep the PaymentTransactionInfoPlugin unset in the context to mark that specific case
-            return new OperationException(MoreObjects.firstNonNull(e.getCause(), e), OperationResult.EXCEPTION);
+        final Throwable originalExceptionOrCause = MoreObjects.firstNonNull(e.getCause(), e);
+        if (originalExceptionOrCause instanceof LockFailedException) {
+            logger.warn("Failed to lock account {}", paymentStateContext.getAccount().getExternalKey());
         } else {
-            if (e.getCause() 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(), e);
-            }
-            convertToUnknownTransactionStatusAndErroredPaymentState(e);
-            return new OperationException(MoreObjects.firstNonNull(e.getCause(), e), OperationResult.EXCEPTION);
+            logger.warn("Payment plugin call threw an exception for account {}", paymentStateContext.getAccount().getExternalKey(), originalExceptionOrCause);
         }
+        return convertToUnknownTransactionStatusAndErroredPaymentState(originalExceptionOrCause);
     }
 
     @Override
@@ -107,7 +101,7 @@ protected OperationException wrapTimeoutException(final PaymentStateContext paym
     }
 
     //
-    // 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.

payment/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentRoutingPluginApi.java

@@ -385,7 +385,8 @@ private DateTime getNextRetryDateForPaymentFailure(final List<PaymentTransaction
     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()) {

payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentEnteringStateCallback.java

@@ -105,18 +105,6 @@ public void testEnterStateAndProcessPaymentTransactionInfoPlugin() throws Except
         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 {

payment/src/test/java/org/killbill/billing/payment/core/sm/TestPaymentOperation.java

@@ -76,7 +76,8 @@ public void testPluginFailure() throws Exception {
             Assert.assertEquals(e.getOperationResult(), OperationResult.EXCEPTION);
         }
 
-        Assert.assertNull(paymentStateContext.getPaymentTransactionInfoPlugin());
+        Assert.assertNotNull(paymentStateContext.getPaymentTransactionInfoPlugin());
+        Assert.assertEquals(paymentStateContext.getPaymentTransactionInfoPlugin().getStatus(), PaymentPluginStatus.UNDEFINED);
     }
 
     @Test(groups = "fast")

pom.xml

@@ -21,7 +21,7 @@
     <parent>
         <artifactId>killbill-oss-parent</artifactId>
         <groupId>org.kill-bill.billing</groupId>
-        <version>0.14</version>
+        <version>0.15</version>
     </parent>
     <artifactId>killbill</artifactId>
     <version>0.15.0-SNAPSHOT</version>