Repository: killbill/killbill
Issue: NPE in /1.0/kb/invoices/:invoiceId/payments

Aborted `payment_attempts` can have a corresponding row in `invoice_payments` with a `NULL` `payment_id` (note that the `payment_cookie_id` i.e. the `payment_attempts. transaction_external_key` won't be `NULL` though):

```
MySQL [killbill]> select * from invoice_payments order by record_id desc limit 1\\G
*************************** 1. row ***************************
                record_id: 3
                       id: e7f6b1d0-3823-4c5b-9226-12327af64492
                     type: ATTEMPT
               invoice_id: 742a1a60-58f8-4ed2-b9db-b1816a741f44
               payment_id: NULL
             payment_date: 2012-05-02 00:37:59
                   amount: 0.000000000
                 currency: USD
       processed_currency: USD
        payment_cookie_id: bac67833-ac7c-458d-8e2a-659c0cdfb0b6
linked_invoice_payment_id: NULL
                  success: 0
               created_by: PaymentRequestProcessor
             created_date: 2012-05-02 00:37:59
        account_record_id: 1
         tenant_record_id: 0
1 row in set (0.00 sec)
```

This can trigger a NPE here: https://github.com/killbill/killbill/blob/1be24a4a144e80abcf45a5982deb7c1f2e22aa3b/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java#L623

```
java.lang.NullPointerException: null
    at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:877)
    at com.google.common.collect.ImmutableSet$Builder.add(ImmutableSet.java:483)
    at com.google.common.collect.ImmutableSet.copyOf(ImmutableSet.java:248)
    at com.google.common.collect.ImmutableSet.copyOf(ImmutableSet.java:230)
    at org.killbill.billing.jaxrs.resources.InvoiceResource.getPaymentsForInvoice(InvoiceResource.java:620)
```

Changed files:
profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java, jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/JaxRsResourceBase.java, jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java

diff --git a/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java b/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java
index 6d1def04a1..b917d4a740 100644
--- a/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java
+++ b/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/InvoiceResource.java
@@ -25,6 +25,7 @@ import java.net.URI;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -83,6 +84,7 @@ import org.killbill.billing.payment.api.InvoicePaymentApi;
 import org.killbill.billing.payment.api.Payment;
 import org.killbill.billing.payment.api.PaymentApi;
 import org.killbill.billing.payment.api.PaymentApiException;
+import org.killbill.billing.payment.api.PaymentOptions;
 import org.killbill.billing.payment.api.PluginProperty;
 import org.killbill.billing.tenant.api.TenantApiException;
 import org.killbill.billing.tenant.api.TenantKV.TenantKey;
@@ -110,7 +112,6 @@ import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Ordering;
@@ -617,12 +618,12 @@ public class InvoiceResource extends JaxRsResourceBase {
         final Invoice invoice = invoiceApi.getInvoice(invoiceId, tenantContext);
 
         // Extract unique set of paymentId for this invoice
-        final Set<UUID> invoicePaymentIds = ImmutableSet.copyOf(Iterables.transform(invoice.getPayments(), new Function<InvoicePayment, UUID>() {
-            @Override
-            public UUID apply(final InvoicePayment input) {
-                return input.getPaymentId();
+        final Set<UUID> invoicePaymentIds = new HashSet<UUID>();
+        for (final InvoicePayment invoicePayment : invoice.getPayments()) {
+            if (invoicePayment.getPaymentId() != null) {
+                invoicePaymentIds.add(invoicePayment.getPaymentId());
             }
-        }));
+        }
         if (invoicePaymentIds.isEmpty()) {
             return Response.status(Status.OK).entity(ImmutableList.<InvoicePaymentJson>of()).build();
         }
@@ -655,6 +656,7 @@ public class InvoiceResource extends JaxRsResourceBase {
     public Response createInstantPayment(@PathParam("invoiceId") final UUID invoiceId,
                                          final InvoicePaymentJson payment,
                                          @QueryParam(QUERY_PAYMENT_EXTERNAL) @DefaultValue("false") final Boolean externalPayment,
+                                         @QueryParam(QUERY_PAYMENT_CONTROL_PLUGIN_NAME) final List<String> paymentControlPluginNames,
                                          @QueryParam(QUERY_PLUGIN_PROPERTY) final List<String> pluginPropertiesString,
                                          @HeaderParam(HDR_CREATED_BY) final String createdBy,
                                          @HeaderParam(HDR_REASON) final String reason,
@@ -672,8 +674,9 @@ public class InvoiceResource extends JaxRsResourceBase {
         final UUID paymentMethodId = externalPayment ? null :
                                      (payment.getPaymentMethodId() != null ? payment.getPaymentMethodId() : account.getPaymentMethodId());
 
-        final InvoicePayment result = createPurchaseForInvoice(account, invoiceId, payment.getPurchasedAmount(), paymentMethodId, externalPayment,
-                                                               payment.getPaymentExternalKey(), null, pluginProperties, callContext);
+        final PaymentOptions paymentOptions = createControlPluginApiPaymentOptions(externalPayment, paymentControlPluginNames);
+        final InvoicePayment result = createPurchaseForInvoice(account, invoiceId, payment.getPurchasedAmount(), paymentMethodId,
+                                                               payment.getPaymentExternalKey(), null, pluginProperties, paymentOptions, callContext);
         return result != null ?
                uriBuilder.buildResponse(uriInfo, InvoicePaymentResource.class, "getInvoicePayment", result.getPaymentId(), request) :
                Response.status(Status.NO_CONTENT).build();
diff --git a/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/JaxRsResourceBase.java b/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/JaxRsResourceBase.java
index 2f61bd1b57..4726ddc8ad 100644
--- a/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/JaxRsResourceBase.java
+++ b/jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/JaxRsResourceBase.java
@@ -22,7 +22,6 @@ import java.io.IOException;
 import java.io.OutputStream;
 import java.math.BigDecimal;
 import java.net.URI;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -553,7 +552,35 @@ public abstract class JaxRsResourceBase implements JaxrsResource {
         return properties;
     }
 
-    protected InvoicePayment createPurchaseForInvoice(final Account account, final UUID invoiceId, final BigDecimal amountToPay, final UUID paymentMethodId, final Boolean externalPayment, final String paymentExternalKey, final String transactionExternalKey, final Iterable<PluginProperty> pluginProperties, final CallContext callContext) throws PaymentApiException {
+    protected InvoicePayment createPurchaseForInvoice(final Account account,
+                                                      final UUID invoiceId,
+                                                      final BigDecimal amountToPay,
+                                                      final UUID paymentMethodId,
+                                                      final Boolean externalPayment,
+                                                      final String paymentExternalKey,
+                                                      final String transactionExternalKey,
+                                                      final Iterable<PluginProperty> pluginProperties,
+                                                      final CallContext callContext) throws PaymentApiException {
+        return createPurchaseForInvoice(account,
+                                        invoiceId,
+                                        amountToPay,
+                                        paymentMethodId,
+                                        paymentExternalKey,
+                                        transactionExternalKey,
+                                        pluginProperties,
+                                        createInvoicePaymentControlPluginApiPaymentOptions(externalPayment),
+                                        callContext);
+    }
+
+    protected InvoicePayment createPurchaseForInvoice(final Account account,
+                                                      final UUID invoiceId,
+                                                      final BigDecimal amountToPay,
+                                                      final UUID paymentMethodId,
+                                                      final String paymentExternalKey,
+                                                      final String transactionExternalKey,
+                                                      final Iterable<PluginProperty> pluginProperties,
+                                                      final PaymentOptions paymentOptions,
+                                                      final CallContext callContext) throws PaymentApiException {
         try {
             return invoicePaymentApi.createPurchaseForInvoicePayment(account,
                                                                      invoiceId,
@@ -564,7 +591,7 @@ public abstract class JaxRsResourceBase implements JaxrsResource {
                                                                      paymentExternalKey,
                                                                      transactionExternalKey,
                                                                      pluginProperties,
-                                                                     createInvoicePaymentControlPluginApiPaymentOptions(externalPayment),
+                                                                     paymentOptions,
                                                                      callContext);
         } catch (final PaymentApiException e) {
             if (e.getCode() == ErrorCode.PAYMENT_PLUGIN_EXCEPTION.getCode() /* &&
diff --git a/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java b/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java
index 4e8856125a..a0facd3ff9 100644
--- a/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java
+++ b/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java
@@ -1,6 +1,6 @@
 /*
- * Copyright 2014-2015 Groupon, Inc
- * Copyright 2014-2015 The Billing Project, LLC
+ * Copyright 2014-2020 Groupon, Inc
+ * Copyright 2014-2020 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
@@ -19,14 +19,12 @@ package org.killbill.billing.jaxrs;
 
 import java.math.BigDecimal;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
 
 import javax.annotation.Nullable;
-import javax.ws.rs.HEAD;
 
 import org.joda.time.DateTime;
 import org.killbill.billing.ObjectType;
@@ -38,6 +36,8 @@ import org.killbill.billing.client.model.Tags;
 import org.killbill.billing.client.model.gen.Account;
 import org.killbill.billing.client.model.gen.AuditLog;
 import org.killbill.billing.client.model.gen.ComboPaymentTransaction;
+import org.killbill.billing.client.model.gen.Invoice;
+import org.killbill.billing.client.model.gen.InvoicePayment;
 import org.killbill.billing.client.model.gen.Payment;
 import org.killbill.billing.client.model.gen.PaymentMethod;
 import org.killbill.billing.client.model.gen.PaymentMethodPluginDetail;
@@ -219,6 +219,50 @@ public class TestPayment extends TestJaxrsBase {
         }
     }
 
+    @Test(groups = "slow", description = "https://github.com/killbill/killbill/issues/1288")
+    public void testWithAbortedInvoicePayment() throws Exception {
+        mockPaymentProviderPlugin.makeNextPaymentFailWithError();
+        final Account account = createAccountWithPMBundleAndSubscriptionAndWaitForFirstInvoice(false);
+        // Getting Invoice #2 (first after Trial period)
+        final Invoice failedInvoice = accountApi.getInvoicesForAccount(account.getAccountId(), null, null, RequestOptions.empty()).get(1);
+
+        // Verify initial state
+        final Payments initialPayments = accountApi.getPaymentsForAccount(account.getAccountId(), true, false, ImmutableMap.<String, String>of(), AuditLevel.NONE, requestOptions);
+        Assert.assertEquals(initialPayments.size(), 1);
+        Assert.assertEquals(initialPayments.get(0).getTransactions().size(), 1);
+        Assert.assertEquals(initialPayments.get(0).getTransactions().get(0).getStatus(), TransactionStatus.PAYMENT_FAILURE);
+        Assert.assertEquals(initialPayments.get(0).getPaymentAttempts().size(), 2);
+        Assert.assertEquals(initialPayments.get(0).getPaymentAttempts().get(0).getStateName(), "RETRIED");
+        Assert.assertEquals(initialPayments.get(0).getPaymentAttempts().get(1).getStateName(), "SCHEDULED");
+        final InvoicePayments initialInvoicePayments = invoiceApi.getPaymentsForInvoice(failedInvoice.getInvoiceId(), requestOptions);
+        Assert.assertEquals(initialInvoicePayments.size(), 1);
+        Assert.assertEquals(initialInvoicePayments.get(0).getPaymentId(), initialPayments.get(0).getPaymentId());
+
+        // Trigger manually an invoice payment but make the control plugin abort it
+        mockPaymentControlProviderPlugin.setAborted(true);
+        final HashMultimap<String, String> queryParams = HashMultimap.create();
+        queryParams.putAll("controlPluginName", Arrays.<String>asList(MockPaymentControlProviderPlugin.PLUGIN_NAME));
+        final RequestOptions inputOptions = RequestOptions.builder()
+                                                          .withCreatedBy(createdBy)
+                                                          .withReason(reason)
+                                                          .withComment(comment)
+                                                          .withQueryParams(queryParams).build();
+        final InvoicePayment invoicePayment = new InvoicePayment();
+        invoicePayment.setPurchasedAmount(failedInvoice.getBalance());
+        invoicePayment.setAccountId(failedInvoice.getAccountId());
+        invoicePayment.setTargetInvoiceId(failedInvoice.getInvoiceId());
+        final InvoicePayment invoicePaymentNull = invoiceApi.createInstantPayment(failedInvoice.getInvoiceId(), invoicePayment, true, null, inputOptions);
+        Assert.assertNull(invoicePaymentNull);
+
+        // Verify new state
+        final Payments updatedPayments = accountApi.getPaymentsForAccount(account.getAccountId(), true, false, ImmutableMap.<String, String>of(), AuditLevel.NONE, requestOptions);
+        Assert.assertEquals(updatedPayments.size(), 1);
+        Assert.assertEquals(updatedPayments.get(0).getPaymentId(), initialPayments.get(0).getPaymentId());
+        final InvoicePayments updatedInvoicePayments = invoiceApi.getPaymentsForInvoice(failedInvoice.getInvoiceId(), requestOptions);
+        Assert.assertEquals(updatedInvoicePayments.size(), 1);
+        Assert.assertEquals(updatedInvoicePayments.get(0).getPaymentId(), updatedPayments.get(0).getPaymentId());
+    }
+
     @Test(groups = "slow")
     public void testWithFailedPaymentAndScheduledAttemptsGetInvoicePayment() throws Exception {
         mockPaymentProviderPlugin.makeNextPaymentFailWithError();