killbill-aplcache
Changes
payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java 40(+27 -13)
Details
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..08688dc 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,18 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
public boolean apply(final PaymentTransactionInfoPlugin input) {
return input.getKbTransactionPaymentId().equals(paymentTransaction.getId());
}
- }).orNull();
+ }).or(new Supplier<PaymentTransactionInfoPlugin>() {
+ @Override
+ public PaymentTransactionInfoPlugin get() {
+ return undefinedPaymentTransaction;
+ }
+ });
} catch (final PaymentPluginApiException ignored) {
- // The paymentTransactionInfoPlugin will be null and handled below as if the plugin did not know about that transaction
+ // 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) {
+ paymentTransactionInfoPlugin = undefinedPaymentTransaction;
}
//
@@ -121,7 +140,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 +159,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,
@@ -151,11 +170,7 @@ 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) {
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 +190,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..91accab 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,12 +28,13 @@ 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
+ // 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 || paymentTransactionInfoPlugin.getStatus() == null) {
+ if (paymentTransactionInfoPlugin == null) {
return TransactionStatus.PLUGIN_FAILURE;
}
@@ -42,9 +43,14 @@ public class PaymentTransactionInfoPluginConverter {
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;
+ //
// 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;
@@ -63,7 +69,7 @@ public class PaymentTransactionInfoPluginConverter {
return OperationResult.PENDING;
case ERROR:
return OperationResult.FAILURE;
- // Might change later if Janitor fixes the state
+ // Might change later if janitor fixes the state
case UNDEFINED:
default:
return OperationResult.EXCEPTION;
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..d0fdaf4 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;
@@ -81,17 +81,22 @@ public abstract class PaymentOperation extends OperationCallbackBase<PaymentTran
@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);
+ //
+ // 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)
+ //
+ 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);
+ } 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);
}
}
@@ -181,8 +186,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 +228,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");
- }
- */
- }
}