From 7e9161e977a7a01dba8c04151542ed1e4c9aa9cd Mon Sep 17 00:00:00 2001 From: stephane brossier Date: Thu, 18 Jun 2015 15:24:40 -0700 Subject: [PATCH] Payment handle new PaymentPluginStatus.CANCELED and simplify the exception logic --- ...PaymentTransactionInfoPluginConverter.java | 22 +++++----------- .../billing/payment/core/ProcessorBase.java | 3 +-- .../IncompletePaymentTransactionTask.java | 8 ++---- .../core/sm/payments/PaymentOperation.java | 26 +++++++------------ .../InvoicePaymentRoutingPluginApi.java | 3 ++- .../sm/TestPaymentEnteringStateCallback.java | 12 --------- .../payment/core/sm/TestPaymentOperation.java | 3 ++- pom.xml | 2 +- 8 files changed, 25 insertions(+), 54 deletions(-) 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 91accab1b8..bcdc703205 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 @@ -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,11 +53,7 @@ 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; @@ -69,8 +61,8 @@ public static OperationResult toOperationResult(@Nullable final PaymentTransacti 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 a411e2c522..77a1265012 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 @@ protected void validateUniqueTransactionExternalKey(@Nullable final String trans final PaymentTransactionModelDao transactionAlreadyExists = Iterables.tryFind(transactions, new Predicate() { @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/janitor/IncompletePaymentTransactionTask.java b/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java index 08688dc989..ede05e356b 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 @@ -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; } 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 d0fdaf4dcb..6781ecd9b6 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 @@ -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. 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 e1b5ddfb52..30d5115192 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 @@ private DateTime getNextRetryDateForPaymentFailure(final List 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 c4f53e6fde..e146a6e9f4 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 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 { 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 9f6eb589ea..586e7597f2 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 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") diff --git a/pom.xml b/pom.xml index ffe30aec7a..837f44f7ed 100644 --- a/pom.xml +++ b/pom.xml @@ -21,7 +21,7 @@ killbill-oss-parent org.kill-bill.billing - 0.14 + 0.15 killbill 0.15.0-SNAPSHOT