Merge pull request #29155 from vespa-engine/reset-exportid-when-deleted-in-stripe

Reset exportid when deleted in stripe

7 files changed, 218 insertions, 40 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/BillingReporterMock.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/BillingReporterMock.java
index 689ecc356dc..04c618880e0 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/BillingReporterMock.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/BillingReporterMock.java
@@ -7,6 +7,8 @@ import com.yahoo.vespa.hosted.controller.tenant.CloudTenant;
 import java.math.BigDecimal;
 import java.time.Clock;
 import java.time.ZonedDateTime;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Optional;
 import java.util.UUID;
 
@@ -14,6 +16,8 @@ public class BillingReporterMock implements BillingReporter {
     private final Clock clock;
     private final BillingDatabaseClient dbClient;
 
+    private final Map<Bill.Id, String> exportedBills = new HashMap<>();
+
     public BillingReporterMock(Clock clock, BillingDatabaseClient dbClient) {
         this.clock = clock;
         this.dbClient = dbClient;
@@ -26,18 +30,28 @@ public class BillingReporterMock implements BillingReporter {
 
     @Override
     public InvoiceUpdate maintainInvoice(Bill bill) {
-        dbClient.addLineItem(bill.tenant(), maintainedMarkerItem(), Optional.of(bill.id()));
-        return new InvoiceUpdate(1,0,0);
+        if (exportedBills.containsKey(bill.id())) {
+            dbClient.addLineItem(bill.tenant(), maintainedMarkerItem(), Optional.of(bill.id()));
+            return ModifiableInvoiceUpdate.of(bill.id(), 1, 0, 0);
+        } else {
+            return FailedInvoiceUpdate.removed(bill.id());
+        }
     }
 
     @Override
     public String exportBill(Bill bill, String exportMethod, CloudTenant tenant) {
         // Replace bill with a copy with exportedId set
-        var exportedId = "EXT-ID-123";
+        var exportedId = "EXPORTED-" + bill.id().value();
+        exportedBills.put(bill.id(), exportedId);
         dbClient.setExportedInvoiceId(bill.id(), exportedId);
         return exportedId;
     }
 
+    // Emulates deleting a bill in the external system.
+    public void deleteExportedBill(Bill.Id billId) {
+        exportedBills.remove(billId);
+    }
+
     private static Bill.LineItem maintainedMarkerItem() {
         return new Bill.LineItem("maintained", "", BigDecimal.valueOf(0.0), "", "", ZonedDateTime.now());
     }
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/FailedInvoiceUpdate.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/FailedInvoiceUpdate.java
new file mode 100644
index 00000000000..9a93c7b05ec
--- /dev/null
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/FailedInvoiceUpdate.java
@@ -0,0 +1,28 @@
+package com.yahoo.vespa.hosted.controller.api.integration.billing;
+
+/**
+ * @author gjoranv
+ */
+public class FailedInvoiceUpdate extends InvoiceUpdate {
+
+    public enum Reason {
+        UNMODIFIABLE,
+        REMOVED
+    }
+
+    public final Reason reason;
+
+    public FailedInvoiceUpdate(Bill.Id billId, Reason reason) {
+        super(billId, ItemsUpdate.empty());
+        this.reason = reason;
+    }
+
+    public static FailedInvoiceUpdate unmodifiable(Bill.Id billId) {
+        return new FailedInvoiceUpdate(billId, Reason.UNMODIFIABLE);
+    }
+
+    public static FailedInvoiceUpdate removed(Bill.Id billId) {
+        return new FailedInvoiceUpdate(billId, Reason.REMOVED);
+    }
+
+}
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/InvoiceUpdate.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/InvoiceUpdate.java
index 6ca3cf6ebb1..bb76834a483 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/InvoiceUpdate.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/InvoiceUpdate.java
@@ -1,44 +1,69 @@
 package com.yahoo.vespa.hosted.controller.api.integration.billing;
 
+import java.util.Objects;
+
 /**
  * Helper to track changes to an invoice.
  *
  * @author gjoranv
  */
-public record InvoiceUpdate(int itemsAdded, int itemsRemoved, int itemsModified) {
-    public boolean isEmpty() {
-        return itemsAdded == 0 && itemsRemoved == 0 && itemsModified == 0;
+public abstract class InvoiceUpdate {
+
+    final Bill.Id billId;
+    final ItemsUpdate itemsUpdate;
+
+    InvoiceUpdate(Bill.Id billId, ItemsUpdate itemsUpdate) {
+        this.billId = billId;
+        this.itemsUpdate = itemsUpdate;
     }
 
-    public static InvoiceUpdate empty() {
-        return new InvoiceUpdate(0, 0, 0);
+    public Bill.Id billId() {
+        return billId;
     }
 
-    public static class Counter {
-        private int itemsAdded = 0;
-        private int itemsRemoved = 0;
-        private int itemsModified = 0;
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        InvoiceUpdate that = (InvoiceUpdate) o;
+        return Objects.equals(billId, that.billId) && Objects.equals(itemsUpdate, that.itemsUpdate);
+    }
 
-        public void addedItem() {
-            itemsAdded++;
-        }
+    @Override
+    public int hashCode() {
+        return Objects.hash(billId, itemsUpdate);
+    }
 
-        public void removedItem() {
-            itemsRemoved++;
-        }
+    public record ItemsUpdate(int itemsAdded, int itemsRemoved, int itemsModified) {
 
-        public void modifiedItem() {
-            itemsModified++;
+        public boolean isEmpty() {
+            return itemsAdded == 0 && itemsRemoved == 0 && itemsModified == 0;
         }
 
-        public void add(InvoiceUpdate other) {
-            itemsAdded += other.itemsAdded;
-            itemsRemoved += other.itemsRemoved;
-            itemsModified += other.itemsModified;
+        public static ItemsUpdate empty() {
+            return new ItemsUpdate(0, 0, 0);
         }
 
-        public InvoiceUpdate finish() {
-            return new InvoiceUpdate(itemsAdded, itemsRemoved, itemsModified);
+        public static class Counter {
+            private int itemsAdded = 0;
+            private int itemsRemoved = 0;
+            private int itemsModified = 0;
+
+            public void addedItem() {
+                itemsAdded++;
+            }
+
+            public void removedItem() {
+                itemsRemoved++;
+            }
+
+            public void modifiedItem() {
+                itemsModified++;
+            }
+
+            public ItemsUpdate finish() {
+                return new ItemsUpdate(itemsAdded, itemsRemoved, itemsModified);
+            }
         }
     }
 
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/ModifiableInvoiceUpdate.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/ModifiableInvoiceUpdate.java
new file mode 100644
index 00000000000..75cce564fc7
--- /dev/null
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/billing/ModifiableInvoiceUpdate.java
@@ -0,0 +1,28 @@
+package com.yahoo.vespa.hosted.controller.api.integration.billing;
+
+/**
+ * @author gjoranv
+ */
+public class ModifiableInvoiceUpdate extends InvoiceUpdate {
+
+    public ModifiableInvoiceUpdate(Bill.Id billId, ItemsUpdate itemsUpdate) {
+        super(billId, itemsUpdate);
+    }
+
+    public ItemsUpdate itemsUpdate() {
+        return itemsUpdate;
+    }
+
+    public boolean isEmpty() {
+        return itemsUpdate.isEmpty();
+    }
+
+    public static ModifiableInvoiceUpdate of(Bill.Id billId, int itemsAdded, int itemsRemoved, int itemsModified) {
+        return new ModifiableInvoiceUpdate(billId, new ItemsUpdate(itemsAdded, itemsRemoved, itemsModified));
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        return super.equals(o);
+    }
+}
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainer.java
index 7868c3fe611..a755e2145b0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainer.java
@@ -5,17 +5,21 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.LockedTenant;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.Bill;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillStatus;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillingController;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillingDatabaseClient;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillingReporter;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.FailedInvoiceUpdate;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.InvoiceUpdate;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.ModifiableInvoiceUpdate;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.Plan;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.PlanRegistry;
 import com.yahoo.vespa.hosted.controller.tenant.CloudTenant;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
 
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -63,17 +67,31 @@ public class BillingReportMaintainer extends ControllerMaintainer {
         });
     }
 
-    InvoiceUpdate maintainInvoices() {
+    List<InvoiceUpdate> maintainInvoices() {
+        var updates = new ArrayList<InvoiceUpdate>();
+
         var billsNeedingMaintenance = databaseClient.readBills().stream()
                 .filter(bill -> bill.getExportedId().isPresent())
                 .filter(exported -> exported.status() == BillStatus.OPEN)
                 .toList();
 
-        var updates = new InvoiceUpdate.Counter();
         for (var bill : billsNeedingMaintenance) {
-            updates.add(reporter.maintainInvoice(bill));
+            var exportedId = bill.getExportedId().orElseThrow();
+            var update = reporter.maintainInvoice(bill);
+            if (update instanceof ModifiableInvoiceUpdate modifiable && ! modifiable.isEmpty()) {
+                log.fine(invoiceMessage(bill.id(), exportedId) + " was updated with " + modifiable.itemsUpdate());
+            } else if (update instanceof FailedInvoiceUpdate failed && failed.reason == FailedInvoiceUpdate.Reason.REMOVED) {
+                log.fine(invoiceMessage(bill.id(), exportedId) +  " has been deleted in the external system");
+                // Reset the exportedId to null, so that we don't maintain it again
+                databaseClient.setExportedInvoiceId(bill.id(), null);
+            }
+            updates.add(update);
         }
-        return updates.finish();
+        return updates;
+    }
+
+    private String invoiceMessage(Bill.Id billId, String invoiceId) {
+        return "Invoice '" + invoiceId + "' for bill '" + billId.value() + "'";
     }
 
     private Map<TenantName, CloudTenant> cloudTenants() {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
index 39d867d813d..2d1f06f3602 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
@@ -100,6 +100,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     private final PlanRegistry planRegistry = new PlanRegistryMock();
     private final ResourceDatabaseClient resourceDb = new ResourceDatabaseClientMock(planRegistry);
     private final BillingDatabaseClient billingDb = new BillingDatabaseClientMock(clock, planRegistry);
+    private final BillingReporterMock billingReporter = new BillingReporterMock(clock, billingDb);
     private final MockBillingController billingController = new MockBillingController(clock, billingDb);
     private final RoleMaintainerMock roleMaintainer = new RoleMaintainerMock();
 
@@ -325,7 +326,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
 
     @Override
     public BillingReporter billingReporter() {
-        return new BillingReporterMock(clock(), billingDb);
+        return billingReporter;
     }
 
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainerTest.java
index 5cb46664a75..8d1848539f0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/BillingReportMaintainerTest.java
@@ -5,7 +5,9 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillStatus;
-import com.yahoo.vespa.hosted.controller.api.integration.billing.InvoiceUpdate;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.BillingReporterMock;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.FailedInvoiceUpdate;
+import com.yahoo.vespa.hosted.controller.api.integration.billing.ModifiableInvoiceUpdate;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.PlanRegistryMock;
 import com.yahoo.vespa.hosted.controller.tenant.BillingReference;
 import com.yahoo.vespa.hosted.controller.tenant.CloudTenant;
@@ -46,7 +48,6 @@ public class BillingReportMaintainerTest {
     @Test
     void only_open_bills_with_exported_id_are_maintained() {
         var t1 = tester.createTenant("t1");
-        var billingController = tester.controller().serviceRegistry().billingController();
         var billingDb = tester.controller().serviceRegistry().billingDatabase();
 
         var start = LocalDate.of(2020, 5, 23).atStartOfDay(ZoneOffset.UTC);
@@ -57,25 +58,88 @@ public class BillingReportMaintainerTest {
         var bill3 = billingDb.createBill(t1, start, end, "exported-and-frozen");
         billingDb.setStatus(bill3, "foo", BillStatus.FROZEN);
 
-        billingController.setPlan(t1, PlanRegistryMock.paidPlan.id(), false, true);
-
-        tester.controller().serviceRegistry().billingReporter().exportBill(billingDb.readBill(bill2).get(), "FOO", cloudTenant(t1));
-        tester.controller().serviceRegistry().billingReporter().exportBill(billingDb.readBill(bill3).get(), "FOO", cloudTenant(t1));
+        var reporter = tester.controller().serviceRegistry().billingReporter();
+        reporter.exportBill(billingDb.readBill(bill2).get(), "FOO", cloudTenant(t1));
+        reporter.exportBill(billingDb.readBill(bill3).get(), "FOO", cloudTenant(t1));
         var updates = maintainer.maintainInvoices();
-        assertEquals(new InvoiceUpdate(1, 0, 0), updates);
 
         assertTrue(billingDb.readBill(bill1).get().getExportedId().isEmpty());
 
+        // Only the exported open bill is maintained
+        assertEquals(1, updates.size());
+        assertEquals(bill2, updates.get(0).billId());
+        assertEquals(ModifiableInvoiceUpdate.class, updates.get(0).getClass());
         var exportedBill = billingDb.readBill(bill2).get();
-        assertEquals("EXT-ID-123", exportedBill.getExportedId().get());
+        assertEquals("EXPORTED-" + exportedBill.id().value(), exportedBill.getExportedId().get());
+        // Verify that the bill has been updated with a marker line item by the mock
         var lineItems = exportedBill.lineItems();
         assertEquals(1, lineItems.size());
         assertEquals("maintained", lineItems.get(0).id());
 
+        // The frozen bill is untouched by the maintainer
         var frozenBill = billingDb.readBill(bill3).get();
-        assertEquals("EXT-ID-123", frozenBill.getExportedId().get());
+        assertEquals("EXPORTED-" + frozenBill.id().value(), frozenBill.getExportedId().get());
         assertEquals(0, frozenBill.lineItems().size());
+    }
+
+    @Test
+    void bills_whose_invoice_has_been_deleted_in_the_external_system_are_no_longer_maintained() {
+        var t1 = tester.createTenant("t1");
+        var billingDb = tester.controller().serviceRegistry().billingDatabase();
+
+        var start = LocalDate.of(2020, 5, 23).atStartOfDay(ZoneOffset.UTC);
+        var end = start.toLocalDate().plusDays(6).atStartOfDay(ZoneOffset.UTC);
+
+        var bill1 = billingDb.createBill(t1, start, end, "exported-then-deleted");
+
+        var reporter = (BillingReporterMock)tester.controller().serviceRegistry().billingReporter();
+        reporter.exportBill(billingDb.readBill(bill1).get(), "FOO", cloudTenant(t1));
+
+        var updates = maintainer.maintainInvoices();
+        assertEquals(1, updates.size());
+        assertEquals(ModifiableInvoiceUpdate.class, updates.get(0).getClass());
+
+        // Delete invoice from the external system
+        reporter.deleteExportedBill(bill1);
+
+        // Maintainer should report that the invoice has been removed
+        updates = maintainer.maintainInvoices();
+        assertEquals(1, updates.size());
+        assertEquals(FailedInvoiceUpdate.class, updates.get(0).getClass());
+        assertEquals(FailedInvoiceUpdate.Reason.REMOVED, ((FailedInvoiceUpdate)updates.get(0)).reason);
+
+        // The bill should no longer be maintained
+        updates = maintainer.maintainInvoices();
+        assertEquals(0, updates.size());
+    }
+
+    @Test
+    void it_is_allowed_to_re_export_bills_whose_invoice_has_been_deleted_in_the_external_system() {
+        var t1 = tester.createTenant("t1");
+        var billingDb = tester.controller().serviceRegistry().billingDatabase();
+
+        var start = LocalDate.of(2020, 5, 23).atStartOfDay(ZoneOffset.UTC);
+        var end = start.toLocalDate().plusDays(6).atStartOfDay(ZoneOffset.UTC);
+
+        var bill1 = billingDb.createBill(t1, start, end, "exported-then-deleted");
+
+        var reporter = (BillingReporterMock)tester.controller().serviceRegistry().billingReporter();
+
+        // Export the bill, then delete it in the external system
+        reporter.exportBill(billingDb.readBill(bill1).get(), "FOO", cloudTenant(t1));
+        maintainer.maintainInvoices();
+        reporter.deleteExportedBill(bill1);
+        maintainer.maintainInvoices();
+
+        // Ensure it is currently ignored by the maintainer
+        var updates = maintainer.maintainInvoices();
+        assertEquals(0, updates.size());
 
+        // Re-export the bill and verify that it is maintained again
+        reporter.exportBill(billingDb.readBill(bill1).get(), "FOO", cloudTenant(t1));
+        updates = maintainer.maintainInvoices();
+        assertEquals(1, updates.size());
+        assertEquals(ModifiableInvoiceUpdate.class, updates.get(0).getClass());
     }
 
     private CloudTenant cloudTenant(TenantName tenantName) {