From f6ef85232ec73bdf75ee4bd116058c8d2614d4fd Mon Sep 17 00:00:00 2001 From: Pierre-Alexandre Meyer Date: Fri, 16 Dec 2016 02:53:02 -0800 Subject: [PATCH] invoice: hardening for bad state Add various tests to verify the current behavior under atypical scenarios, namely: * Complex timelines with various same-day changes do not trigger any bounds * Duplicate (buggy) billing events do not lead to invoicing errors * If multiple fixed items for a given subscription and start date already exist on disk, the next run will not re-generate one * If multiple recurring items for a given subscription and service period already exist on disk, the next run will not complete (the merge logic will detect the existing bad state, i.e. double billing) While the system can already handle these bad cases, it's still possible that either our items generation code (proposed items) or the merge algorithm have issues in even more complex setups (repairs, adjustments, etc.). To pro-actively detect these, this patch introduces new internal safety checks on the resulting list: * At most one FIXED item should be present for a given subscription id and start date * At most one RECURRING item should be present for a given subscription id and service period The behavior can be disabled by setting org.killbill.invoice.sanitySafetyBoundEnabled=false. See https://github.com/killbill/killbill/issues/664. Signed-off-by: Pierre-Alexandre Meyer --- .../config/MultiTenantInvoiceConfig.java | 14 + ...FixedAndRecurringInvoiceItemGenerator.java | 43 +- .../TestDefaultInvoiceGenerator.java | 407 +++++++++++++----- ...FixedAndRecurringInvoiceItemGenerator.java | 235 +++++++++- .../util/config/definition/InvoiceConfig.java | 10 + 5 files changed, 607 insertions(+), 102 deletions(-) diff --git a/invoice/src/main/java/org/killbill/billing/invoice/config/MultiTenantInvoiceConfig.java b/invoice/src/main/java/org/killbill/billing/invoice/config/MultiTenantInvoiceConfig.java index acb4dd07ba..d519cc0189 100644 --- a/invoice/src/main/java/org/killbill/billing/invoice/config/MultiTenantInvoiceConfig.java +++ b/invoice/src/main/java/org/killbill/billing/invoice/config/MultiTenantInvoiceConfig.java @@ -52,6 +52,20 @@ public int getNumberOfMonthsInFuture(final InternalTenantContext tenantContext) return getNumberOfMonthsInFuture(); } + @Override + public boolean isSanitySafetyBoundEnabled() { + return staticConfig.isSanitySafetyBoundEnabled(); + } + + @Override + public boolean isSanitySafetyBoundEnabled(final InternalTenantContext tenantContext) { + final String result = getStringTenantConfig("isSanitySafetyBoundEnabled", tenantContext); + if (result != null) { + return Boolean.parseBoolean(result); + } + return isSanitySafetyBoundEnabled(); + } + @Override public int getMaxDailyNumberOfItemsSafetyBound() { return staticConfig.getMaxDailyNumberOfItemsSafetyBound(); diff --git a/invoice/src/main/java/org/killbill/billing/invoice/generator/FixedAndRecurringInvoiceItemGenerator.java b/invoice/src/main/java/org/killbill/billing/invoice/generator/FixedAndRecurringInvoiceItemGenerator.java index 6dc31d65dc..eaa69f8333 100644 --- a/invoice/src/main/java/org/killbill/billing/invoice/generator/FixedAndRecurringInvoiceItemGenerator.java +++ b/invoice/src/main/java/org/killbill/billing/invoice/generator/FixedAndRecurringInvoiceItemGenerator.java @@ -20,6 +20,7 @@ import java.math.BigDecimal; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -40,6 +41,7 @@ import org.killbill.billing.invoice.api.Invoice; import org.killbill.billing.invoice.api.InvoiceApiException; import org.killbill.billing.invoice.api.InvoiceItem; +import org.killbill.billing.invoice.api.InvoiceItemType; import org.killbill.billing.invoice.generator.InvoiceWithMetadata.SubscriptionFutureNotificationDates; import org.killbill.billing.invoice.model.FixedPriceInvoiceItem; import org.killbill.billing.invoice.model.InvalidDateSequenceException; @@ -59,6 +61,7 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.Multimap; +import com.google.common.collect.Range; import com.google.inject.Inject; import static org.killbill.billing.invoice.generator.InvoiceDateUtils.calculateNumberOfWholeBillingPeriods; @@ -107,7 +110,7 @@ public List generateItems(final ImmutableAccountData account, final accountItemTree.mergeWithProposedItems(proposedItems); final List resultingItems = accountItemTree.getResultingItemList(); - safetyBound(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); + safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); return resultingItems; } @@ -403,8 +406,42 @@ private InvoiceItem generateFixedPriceItem(final UUID invoiceId, final UUID acco } } - // Trigger an exception if we create too many subscriptions for a subscription on a given day - private void safetyBound(final Iterable resultingItems, final Multimap createdItemsPerDayPerSubscription, final InternalTenantContext internalCallContext) throws InvoiceApiException { + @VisibleForTesting + void safetyBounds(final Iterable resultingItems, final Multimap createdItemsPerDayPerSubscription, final InternalTenantContext internalCallContext) throws InvoiceApiException { + // Trigger an exception if we detect the creation of similar items for a given subscription + // See https://github.com/killbill/killbill/issues/664 + if (config.isSanitySafetyBoundEnabled(internalCallContext)) { + final Map> fixedItemsPerDateAndSubscription = new HashMap>(); + final Map, InvoiceItem>> recurringItemsPerServicePeriodAndSubscription = new HashMap, InvoiceItem>>(); + for (final InvoiceItem resultingItem : resultingItems) { + if (resultingItem.getInvoiceItemType() == InvoiceItemType.FIXED) { + if (fixedItemsPerDateAndSubscription.get(resultingItem.getSubscriptionId()) == null) { + fixedItemsPerDateAndSubscription.put(resultingItem.getSubscriptionId(), LinkedListMultimap.create()); + } + fixedItemsPerDateAndSubscription.get(resultingItem.getSubscriptionId()).put(resultingItem.getStartDate(), resultingItem); + + final Collection resultingInvoiceItems = fixedItemsPerDateAndSubscription.get(resultingItem.getSubscriptionId()).get(resultingItem.getStartDate()); + if (resultingInvoiceItems.size() > 1) { + throw new InvoiceApiException(ErrorCode.UNEXPECTED_ERROR, String.format("SAFETY BOUND TRIGGERED Multiple FIXED items for subscriptionId='%s', startDate='%s', resultingItems=%s", + resultingItem.getSubscriptionId(), resultingItem.getStartDate(), resultingInvoiceItems)); + } + } else if (resultingItem.getInvoiceItemType() == InvoiceItemType.RECURRING) { + if (recurringItemsPerServicePeriodAndSubscription.get(resultingItem.getSubscriptionId()) == null) { + recurringItemsPerServicePeriodAndSubscription.put(resultingItem.getSubscriptionId(), LinkedListMultimap., InvoiceItem>create()); + } + final Range interval = Range.closedOpen(resultingItem.getStartDate(), resultingItem.getEndDate()); + recurringItemsPerServicePeriodAndSubscription.get(resultingItem.getSubscriptionId()).put(interval, resultingItem); + + final Collection resultingInvoiceItems = recurringItemsPerServicePeriodAndSubscription.get(resultingItem.getSubscriptionId()).get(interval); + if (resultingInvoiceItems.size() > 1) { + throw new InvoiceApiException(ErrorCode.UNEXPECTED_ERROR, String.format("SAFETY BOUND TRIGGERED Multiple RECURRING items for subscriptionId='%s', startDate='%s', endDate='%s', resultingItems=%s", + resultingItem.getSubscriptionId(), resultingItem.getStartDate(), resultingItem.getEndDate(), resultingInvoiceItems)); + } + } + } + } + + // Trigger an exception if we create too many invoice items for a subscription on a given day if (config.getMaxDailyNumberOfItemsSafetyBound(internalCallContext) == -1) { // Safety bound disabled return; diff --git a/invoice/src/test/java/org/killbill/billing/invoice/generator/TestDefaultInvoiceGenerator.java b/invoice/src/test/java/org/killbill/billing/invoice/generator/TestDefaultInvoiceGenerator.java index e104024972..e6eb41f16d 100644 --- a/invoice/src/test/java/org/killbill/billing/invoice/generator/TestDefaultInvoiceGenerator.java +++ b/invoice/src/test/java/org/killbill/billing/invoice/generator/TestDefaultInvoiceGenerator.java @@ -122,6 +122,16 @@ public int getNumberOfMonthsInFuture(final InternalTenantContext context) { return getNumberOfMonthsInFuture(); } + @Override + public boolean isSanitySafetyBoundEnabled() { + return true; + } + + @Override + public boolean isSanitySafetyBoundEnabled(final InternalTenantContext tenantContext) { + return true; + } + @Override public int getMaxDailyNumberOfItemsSafetyBound() { return 10; @@ -252,18 +262,6 @@ public void testWithSingleThirtyDaysEvent() throws InvoiceApiException, CatalogA assertEquals(invoice.getInvoiceItems().get(1).getEndDate(), new LocalDate(2011, 10, 31)); } - private SubscriptionBase createSubscription() { - return createSubscription(UUID.randomUUID(), UUID.randomUUID()); - } - - private SubscriptionBase createSubscription(final UUID subscriptionId, final UUID bundleId) { - final SubscriptionBase sub = Mockito.mock(SubscriptionBase.class); - Mockito.when(sub.getId()).thenReturn(subscriptionId); - Mockito.when(sub.getBundleId()).thenReturn(bundleId); - - return sub; - } - @Test(groups = "fast") public void testSimpleWithTimeZone() throws InvoiceApiException, CatalogApiException { final SubscriptionBase sub = createSubscription(); @@ -842,65 +840,6 @@ public void testTargetDateRestrictionFailure() throws InvoiceApiException, Catal generator.generateInvoice(account, events, null, targetDate, Currency.USD, internalCallContext); } - private MockPlanPhase createMockThirtyDaysPlanPhase(@Nullable final BigDecimal recurringRate) { - return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), - null, BillingPeriod.THIRTY_DAYS); - } - - private MockPlanPhase createMockMonthlyPlanPhase() { - return new MockPlanPhase(null, null, BillingPeriod.MONTHLY); - } - - private MockPlanPhase createMockMonthlyPlanPhase(@Nullable final BigDecimal recurringRate) { - return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), - null, BillingPeriod.MONTHLY); - } - - private MockPlanPhase createMockMonthlyPlanPhase(final BigDecimal recurringRate, final PhaseType phaseType) { - return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), - null, BillingPeriod.MONTHLY, phaseType); - } - - private MockPlanPhase createMockMonthlyPlanPhase(@Nullable final BigDecimal recurringRate, - @Nullable final BigDecimal fixedCost, - final PhaseType phaseType) { - final MockInternationalPrice recurringPrice = (recurringRate == null) ? null : new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)); - final MockInternationalPrice fixedPrice = (fixedCost == null) ? null : new MockInternationalPrice(new DefaultPrice(fixedCost, Currency.USD)); - - return new MockPlanPhase(recurringPrice, fixedPrice, BillingPeriod.MONTHLY, phaseType); - } - - private MockPlanPhase createMockAnnualPlanPhase(final BigDecimal recurringRate, final PhaseType phaseType) { - return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), - null, BillingPeriod.ANNUAL, phaseType); - } - - private BillingEvent createBillingEvent(final UUID subscriptionId, final UUID bundleId, final LocalDate startDate, - final Plan plan, final PlanPhase planPhase, final int billCycleDayLocal) throws CatalogApiException { - final SubscriptionBase sub = createSubscription(subscriptionId, bundleId); - final Currency currency = Currency.USD; - - return invoiceUtil.createMockBillingEvent(null, sub, startDate.toDateTimeAtStartOfDay(), plan, planPhase, - planPhase.getFixed().getPrice() == null ? null : planPhase.getFixed().getPrice().getPrice(currency), - planPhase.getRecurring().getRecurringPrice() == null ? null : planPhase.getRecurring().getRecurringPrice().getPrice(currency), - currency, planPhase.getRecurring().getBillingPeriod(), - billCycleDayLocal, BillingMode.IN_ADVANCE, "Test", 1L, SubscriptionBaseTransitionType.CREATE); - } - - private void testInvoiceGeneration(final UUID accountId, final BillingEventSet events, final List existingInvoices, - final LocalDate targetDate, final int expectedNumberOfItems, - final BigDecimal expectedAmount) throws InvoiceApiException { - final Currency currency = Currency.USD; - final InvoiceWithMetadata invoiceWithMetadata = generator.generateInvoice(account, events, existingInvoices, targetDate, currency, internalCallContext); - final Invoice invoice = invoiceWithMetadata.getInvoice(); - assertNotNull(invoice); - assertEquals(invoice.getNumberOfItems(), expectedNumberOfItems); - existingInvoices.add(invoice); - - distributeItems(existingInvoices); - assertEquals(invoice.getBalance(), KillBillMoney.of(expectedAmount, invoice.getCurrency())); - } - @Test(groups = "fast") public void testWithFullRepairInvoiceGeneration() throws CatalogApiException, InvoiceApiException { final LocalDate april25 = new LocalDate(2012, 4, 25); @@ -1110,32 +1049,6 @@ public void testRegressionFor170() throws EntityPersistenceException, InvoiceApi assertNull(newInvoice); } - private void distributeItems(final List invoices) { - final Map invoiceMap = new HashMap(); - - for (final Invoice invoice : invoices) { - invoiceMap.put(invoice.getId(), invoice); - } - - for (final Invoice invoice : invoices) { - final Iterator itemIterator = invoice.getInvoiceItems().iterator(); - final UUID invoiceId = invoice.getId(); - - while (itemIterator.hasNext()) { - final InvoiceItem item = itemIterator.next(); - - if (!item.getInvoiceId().equals(invoiceId)) { - final Invoice thisInvoice = invoiceMap.get(item.getInvoiceId()); - if (thisInvoice == null) { - throw new NullPointerException(); - } - thisInvoice.addInvoiceItem(item); - itemIterator.remove(); - } - } - } - } - @Test(groups = "fast") public void testAutoInvoiceOffAccount() throws Exception { final MockBillingEventSet events = new MockBillingEventSet(); @@ -1158,6 +1071,7 @@ public void testAutoInvoiceOffAccount() throws Exception { assertNull(invoiceWithMetadata.getInvoice()); } + @Test(groups = "fast") public void testAutoInvoiceOffWithCredits() throws CatalogApiException, InvoiceApiException { final Currency currency = Currency.USD; final List invoices = new ArrayList(); @@ -1257,6 +1171,305 @@ public void testCancelEOTWithFullItemAdjustment() throws CatalogApiException, In assertNull(invoice2); } + // Complex but plausible scenario, with multiple same-day changes, to verify bounds are not triggered + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testMultipleDailyChangesDoNotTriggerBounds() throws InvoiceApiException, CatalogApiException { + final UUID accountId = UUID.randomUUID(); + final UUID bundleId = UUID.randomUUID(); + final UUID subscriptionId1 = UUID.randomUUID(); + + final BillingEventSet events = new MockBillingEventSet(); + final List invoices = new ArrayList(); + Invoice invoice; + + final Plan plan1 = new MockPlan("plan1"); + final PlanPhase plan1Phase1 = createMockMonthlyPlanPhase(null, EIGHT, PhaseType.TRIAL); + final PlanPhase plan1Phase2 = createMockMonthlyPlanPhase(TWELVE, PhaseType.DISCOUNT); + final LocalDate plan1StartDate = invoiceUtil.buildDate(2011, 1, 5); + final LocalDate plan1PhaseChangeDate = invoiceUtil.buildDate(2011, 4, 5); + + final Plan plan2 = new MockPlan("plan2"); + final PlanPhase plan2Phase1 = createMockMonthlyPlanPhase(null, TWENTY, PhaseType.TRIAL); + final PlanPhase plan2Phase2 = createMockMonthlyPlanPhase(THIRTY, PhaseType.DISCOUNT); + final PlanPhase plan2Phase3 = createMockMonthlyPlanPhase(FORTY, PhaseType.EVERGREEN); + final PlanPhase plan2Phase4 = createMockMonthlyPlanPhase(); + final LocalDate plan2PhaseChangeToEvergreenDate = invoiceUtil.buildDate(2011, 6, 5); + final LocalDate plan2CancelDate = invoiceUtil.buildDate(2011, 6, 5); + + // On 1/5/2011, start TRIAL on plan1 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1StartDate, plan1, plan1Phase1, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1StartDate, 1, EIGHT); + invoice = invoices.get(0); + assertEquals(invoice.getInvoiceItems().size(), 1); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.FIXED); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 1, 5)); + assertNull(invoice.getInvoiceItems().get(0).getEndDate()); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(EIGHT), 0); + + // On 1/5/2011, change to TRIAL on plan2 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1StartDate, plan2, plan2Phase1, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1StartDate, 1, TWENTY); + assertEquals(invoices.get(0), invoice); + invoice = invoices.get(1); + assertEquals(invoice.getInvoiceItems().size(), 1); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.FIXED); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 1, 5)); + assertNull(invoice.getInvoiceItems().get(0).getEndDate()); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(TWENTY), 0); + + // On 1/5/2011, change back to TRIAL on plan1 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1StartDate, plan1, plan1Phase1, 5)); + + // We don't repair FIXED items and one already exists for that date - nothing to generate + testNullInvoiceGeneration(events, invoices, plan1StartDate); + + // On 4/5/2011, phase change to DISCOUNT on plan1 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1PhaseChangeDate, plan1, plan1Phase2, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1PhaseChangeDate, 1, TWELVE); + assertEquals(invoices.get(1), invoice); + invoice = invoices.get(2); + assertEquals(invoice.getInvoiceItems().size(), 1); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(TWELVE), 0); + + // On 4/5/2011, change to DISCOUNT on plan2 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1PhaseChangeDate, plan2, plan2Phase2, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1PhaseChangeDate, 2, new BigDecimal("18")); + assertEquals(invoices.get(2), invoice); + invoice = invoices.get(3); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(THIRTY), 0); + assertEquals(invoice.getInvoiceItems().get(1).getLinkedItemId(), invoices.get(2).getInvoiceItems().get(0).getId()); + assertEquals(invoice.getInvoiceItems().get(1).getInvoiceItemType(), InvoiceItemType.REPAIR_ADJ); + assertEquals(invoice.getInvoiceItems().get(1).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getAmount().compareTo(TWELVE.negate()), 0); + + // On 4/5/2011, change back to DISCOUNT on plan1 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1PhaseChangeDate, plan1, plan1Phase2, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1PhaseChangeDate, 2, new BigDecimal("-18")); + assertEquals(invoices.get(3), invoice); + invoice = invoices.get(4); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(TWELVE), 0); + assertEquals(invoice.getInvoiceItems().get(1).getLinkedItemId(), invoices.get(3).getInvoiceItems().get(0).getId()); + assertEquals(invoice.getInvoiceItems().get(1).getInvoiceItemType(), InvoiceItemType.REPAIR_ADJ); + assertEquals(invoice.getInvoiceItems().get(1).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getAmount().compareTo(THIRTY.negate()), 0); + + // On 4/5/2011, change back to DISCOUNT on plan2 + events.add(createBillingEvent(subscriptionId1, bundleId, plan1PhaseChangeDate, plan2, plan2Phase2, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan1PhaseChangeDate, 2, new BigDecimal("18")); + assertEquals(invoices.get(4), invoice); + invoice = invoices.get(5); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(THIRTY), 0); + assertEquals(invoice.getInvoiceItems().get(1).getLinkedItemId(), invoices.get(4).getInvoiceItems().get(0).getId()); + assertEquals(invoice.getInvoiceItems().get(1).getInvoiceItemType(), InvoiceItemType.REPAIR_ADJ); + assertEquals(invoice.getInvoiceItems().get(1).getStartDate(), new LocalDate(2011, 4, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getEndDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getAmount().compareTo(TWELVE.negate()), 0); + + // On 6/5/2011, phase change to EVERGREEN on plan2 + events.add(createBillingEvent(subscriptionId1, bundleId, plan2PhaseChangeToEvergreenDate, plan2, plan2Phase3, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan2PhaseChangeToEvergreenDate, 2, new BigDecimal("70")); + assertEquals(invoices.get(5), invoice); + invoice = invoices.get(6); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 5, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 6, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(THIRTY), 0); + assertEquals(invoice.getInvoiceItems().get(1).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(1).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(1).getStartDate(), new LocalDate(2011, 6, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getEndDate(), new LocalDate(2011, 7, 5)); + assertEquals(invoice.getInvoiceItems().get(1).getAmount().compareTo(FORTY), 0); + + // On 6/5/2011, cancel subscription + events.add(createBillingEvent(subscriptionId1, bundleId, plan2CancelDate, plan2, plan2Phase4, 5)); + + testInvoiceGeneration(accountId, events, invoices, plan2PhaseChangeToEvergreenDate, 1, FORTY.negate()); + assertEquals(invoices.get(6), invoice); + invoice = invoices.get(7); + assertEquals(invoice.getInvoiceItems().get(0).getLinkedItemId(), invoices.get(6).getInvoiceItems().get(1).getId()); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.REPAIR_ADJ); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 6, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 7, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(FORTY.negate()), 0); + } + + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testBuggyBillingEventsDoNotImpactInvoicing() throws InvoiceApiException, CatalogApiException { + final UUID accountId = UUID.randomUUID(); + final UUID bundleId = UUID.randomUUID(); + final UUID subscriptionId1 = UUID.randomUUID(); + + final BillingEventSet events = new MockBillingEventSet(); + final List invoices = new ArrayList(); + Invoice invoice; + + final Plan plan1 = new MockPlan("plan1"); + final PlanPhase plan1Phase1 = createMockMonthlyPlanPhase(null, EIGHT, PhaseType.TRIAL); + final PlanPhase plan1Phase2 = createMockMonthlyPlanPhase(TWELVE, PhaseType.EVERGREEN); + final LocalDate plan1StartDate = invoiceUtil.buildDate(2011, 1, 5); + final LocalDate plan1PhaseChangeDate = invoiceUtil.buildDate(2011, 2, 5); + + // To simulate a bug, duplicate the billing events + for (int i = 0; i < 10; i++) { + events.add(createBillingEvent(subscriptionId1, bundleId, plan1StartDate, plan1, plan1Phase1, 5)); + events.add(createBillingEvent(subscriptionId1, bundleId, plan1PhaseChangeDate, plan1, plan1Phase2, 5)); + } + assertEquals(events.size(), 20); + + // Fix for https://github.com/killbill/killbill/issues/467 will prevent duplicate fixed items + testInvoiceGeneration(accountId, events, invoices, plan1StartDate, 1, EIGHT); + invoice = invoices.get(0); + assertEquals(invoice.getInvoiceItems().size(), 1); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.FIXED); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 1, 5)); + assertNull(invoice.getInvoiceItems().get(0).getEndDate()); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(EIGHT), 0); + + // Intermediate billing intervals associated with recurring items will be less than a day, so only one recurring item will be generated + testInvoiceGeneration(accountId, events, invoices, plan1PhaseChangeDate, 1, TWELVE); + invoice = invoices.get(1); + assertEquals(invoice.getInvoiceItems().size(), 1); + assertEquals(invoice.getInvoiceItems().get(0).getSubscriptionId(), subscriptionId1); + assertEquals(invoice.getInvoiceItems().get(0).getInvoiceItemType(), InvoiceItemType.RECURRING); + assertEquals(invoice.getInvoiceItems().get(0).getStartDate(), new LocalDate(2011, 2, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getEndDate(), new LocalDate(2011, 3, 5)); + assertEquals(invoice.getInvoiceItems().get(0).getAmount().compareTo(TWELVE), 0); + } + + private Long totalOrdering = 1L; + + private MockPlanPhase createMockThirtyDaysPlanPhase(@Nullable final BigDecimal recurringRate) { + return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), + null, BillingPeriod.THIRTY_DAYS); + } + + private MockPlanPhase createMockMonthlyPlanPhase() { + return new MockPlanPhase(null, null, BillingPeriod.MONTHLY); + } + + private MockPlanPhase createMockMonthlyPlanPhase(@Nullable final BigDecimal recurringRate) { + return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), + null, BillingPeriod.MONTHLY); + } + + private MockPlanPhase createMockMonthlyPlanPhase(final BigDecimal recurringRate, final PhaseType phaseType) { + return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), + null, BillingPeriod.MONTHLY, phaseType); + } + + private MockPlanPhase createMockMonthlyPlanPhase(@Nullable final BigDecimal recurringRate, + @Nullable final BigDecimal fixedCost, + final PhaseType phaseType) { + final MockInternationalPrice recurringPrice = (recurringRate == null) ? null : new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)); + final MockInternationalPrice fixedPrice = (fixedCost == null) ? null : new MockInternationalPrice(new DefaultPrice(fixedCost, Currency.USD)); + + return new MockPlanPhase(recurringPrice, fixedPrice, BillingPeriod.MONTHLY, phaseType); + } + + private MockPlanPhase createMockAnnualPlanPhase(final BigDecimal recurringRate, final PhaseType phaseType) { + return new MockPlanPhase(new MockInternationalPrice(new DefaultPrice(recurringRate, Currency.USD)), + null, BillingPeriod.ANNUAL, phaseType); + } + + private SubscriptionBase createSubscription() { + return createSubscription(UUID.randomUUID(), UUID.randomUUID()); + } + + private SubscriptionBase createSubscription(final UUID subscriptionId, final UUID bundleId) { + final SubscriptionBase sub = Mockito.mock(SubscriptionBase.class); + Mockito.when(sub.getId()).thenReturn(subscriptionId); + Mockito.when(sub.getBundleId()).thenReturn(bundleId); + + return sub; + } + + private BillingEvent createBillingEvent(final UUID subscriptionId, final UUID bundleId, final LocalDate startDate, + final Plan plan, final PlanPhase planPhase, final int billCycleDayLocal) throws CatalogApiException { + final SubscriptionBase sub = createSubscription(subscriptionId, bundleId); + final Currency currency = Currency.USD; + + return invoiceUtil.createMockBillingEvent(null, sub, startDate.toDateTimeAtStartOfDay(), plan, planPhase, + planPhase.getFixed().getPrice() == null ? null : planPhase.getFixed().getPrice().getPrice(currency), + planPhase.getRecurring().getRecurringPrice() == null ? null : planPhase.getRecurring().getRecurringPrice().getPrice(currency), + currency, planPhase.getRecurring().getBillingPeriod(), + billCycleDayLocal, BillingMode.IN_ADVANCE, "Test", totalOrdering++, SubscriptionBaseTransitionType.CREATE); + } + + private void testInvoiceGeneration(final UUID accountId, final BillingEventSet events, final List existingInvoices, + final LocalDate targetDate, final int expectedNumberOfItems, + final BigDecimal expectedAmount) throws InvoiceApiException { + final Currency currency = Currency.USD; + final InvoiceWithMetadata invoiceWithMetadata = generator.generateInvoice(account, events, existingInvoices, targetDate, currency, internalCallContext); + final Invoice invoice = invoiceWithMetadata.getInvoice(); + assertNotNull(invoice); + assertEquals(invoice.getNumberOfItems(), expectedNumberOfItems); + existingInvoices.add(invoice); + + distributeItems(existingInvoices); + assertEquals(invoice.getBalance(), KillBillMoney.of(expectedAmount, invoice.getCurrency())); + } + + private void testNullInvoiceGeneration(final BillingEventSet events, final List existingInvoices, final LocalDate targetDate) throws InvoiceApiException { + final Currency currency = Currency.USD; + final InvoiceWithMetadata invoiceWithMetadata = generator.generateInvoice(account, events, existingInvoices, targetDate, currency, internalCallContext); + final Invoice invoice = invoiceWithMetadata.getInvoice(); + assertNull(invoice); + } + + private void distributeItems(final List invoices) { + final Map invoiceMap = new HashMap(); + + for (final Invoice invoice : invoices) { + invoiceMap.put(invoice.getId(), invoice); + } + + for (final Invoice invoice : invoices) { + final Iterator itemIterator = invoice.getInvoiceItems().iterator(); + final UUID invoiceId = invoice.getId(); + + while (itemIterator.hasNext()) { + final InvoiceItem item = itemIterator.next(); + + if (!item.getInvoiceId().equals(invoiceId)) { + final Invoice thisInvoice = invoiceMap.get(item.getInvoiceId()); + if (thisInvoice == null) { + throw new NullPointerException(); + } + thisInvoice.addInvoiceItem(item); + itemIterator.remove(); + } + } + } + } + private void printDetailInvoice(final Invoice invoice) { log.info("-------------------- START DETAIL ----------------------"); log.info("Invoice " + invoice.getId() + ": BALANCE = " + invoice.getBalance() diff --git a/invoice/src/test/java/org/killbill/billing/invoice/generator/TestFixedAndRecurringInvoiceItemGenerator.java b/invoice/src/test/java/org/killbill/billing/invoice/generator/TestFixedAndRecurringInvoiceItemGenerator.java index 5d81097361..7f19ff7083 100644 --- a/invoice/src/test/java/org/killbill/billing/invoice/generator/TestFixedAndRecurringInvoiceItemGenerator.java +++ b/invoice/src/test/java/org/killbill/billing/invoice/generator/TestFixedAndRecurringInvoiceItemGenerator.java @@ -19,6 +19,7 @@ import java.math.BigDecimal; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -55,6 +56,9 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.Multimap; + import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -78,7 +82,6 @@ public void beforeMethod() { } } - @Test(groups = "fast") public void testIsSameDayAndSameSubscriptionWithNullPrevEvent() { @@ -286,7 +289,7 @@ public void testProcessFixedBillingEventsWithMultipleChangeOnSameDay() throws In } @Test(groups = "fast") - public void testSafetyBounds() throws InvoiceApiException { + public void testSafetyBoundsTooManyInvoiceItemsForGivenSubscriptionAndInvoiceDate() throws InvoiceApiException { final int threshold = 15; final LocalDate startDate = new LocalDate("2016-01-01"); @@ -373,4 +376,232 @@ public void testSafetyBounds() throws InvoiceApiException { assertEquals(e.getCode(), ErrorCode.UNEXPECTED_ERROR.getCode()); } } + + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testTooManyFixedInvoiceItemsForGivenSubscriptionAndStartDate() throws InvoiceApiException { + final LocalDate startDate = new LocalDate("2016-01-01"); + + final BillingEventSet events = new MockBillingEventSet(); + final BigDecimal amount = BigDecimal.TEN; + final MockInternationalPrice price = new MockInternationalPrice(new DefaultPrice(amount, account.getCurrency())); + final Plan plan = new MockPlan("my-plan"); + final PlanPhase planPhase = new MockPlanPhase(null, price, BillingPeriod.NO_BILLING_PERIOD, PhaseType.TRIAL); + final BillingEvent event = invoiceUtil.createMockBillingEvent(account, + subscription, + startDate.toDateTimeAtStartOfDay(), + plan, + planPhase, + amount, + null, + account.getCurrency(), + BillingPeriod.NO_BILLING_PERIOD, + 1, + BillingMode.IN_ADVANCE, + "Billing Event Desc", + 1L, + SubscriptionBaseTransitionType.CREATE); + events.add(event); + + // Simulate a bunch of fixed items for that subscription and start date (simulate bad data on disk) + final List existingInvoices = new LinkedList(); + for (int i = 0; i < 20; i++) { + final Invoice invoice = new DefaultInvoice(account.getId(), clock.getUTCToday(), startDate, account.getCurrency()); + invoice.addInvoiceItem(new FixedPriceInvoiceItem(UUID.randomUUID(), + clock.getUTCNow(), + invoice.getId(), + account.getId(), + subscription.getBundleId(), + subscription.getId(), + event.getPlan().getName(), + event.getPlanPhase().getName(), + "Buggy fixed item", + startDate, + amount, + account.getCurrency())); + existingInvoices.add(invoice); + } + + final List generatedItems = fixedAndRecurringInvoiceItemGenerator.generateItems(account, + UUID.randomUUID(), + events, + existingInvoices, + startDate, + account.getCurrency(), + new HashMap(), + internalCallContext); + // There will be one proposed, but because it will match one of ones in the existing list and we don't repair, it won't be returned + assertEquals(generatedItems.size(), 0); + } + + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testSubscriptionAlreadyDoubleBilledForServicePeriod() throws InvoiceApiException { + final LocalDate startDate = new LocalDate("2016-01-01"); + + final BillingEventSet events = new MockBillingEventSet(); + final BigDecimal amount = BigDecimal.TEN; + final MockInternationalPrice price = new MockInternationalPrice(new DefaultPrice(amount, account.getCurrency())); + final Plan plan = new MockPlan("my-plan"); + final PlanPhase planPhase = new MockPlanPhase(price, null, BillingPeriod.MONTHLY, PhaseType.EVERGREEN); + final BillingEvent event = invoiceUtil.createMockBillingEvent(account, + subscription, + startDate.toDateTimeAtStartOfDay(), + plan, + planPhase, + null, + amount, + account.getCurrency(), + BillingPeriod.MONTHLY, + 1, + BillingMode.IN_ADVANCE, + "Billing Event Desc", + 1L, + SubscriptionBaseTransitionType.CREATE); + events.add(event); + + // Simulate a bunch of recurring items for that subscription and service period (bad data on disk leading to double billing) + final List existingInvoices = new LinkedList(); + for (int i = 0; i < 20; i++) { + final Invoice invoice = new DefaultInvoice(account.getId(), clock.getUTCToday(), startDate.plusMonths(i), account.getCurrency()); + invoice.addInvoiceItem(new RecurringInvoiceItem(UUID.randomUUID(), + // Set random dates to verify it doesn't impact double billing detection + startDate.plusMonths(i).toDateTimeAtStartOfDay(), + invoice.getId(), + account.getId(), + subscription.getBundleId(), + subscription.getId(), + event.getPlan().getName(), + event.getPlanPhase().getName(), + startDate, + startDate.plusMonths(1), + amount, + amount, + account.getCurrency())); + existingInvoices.add(invoice); + } + + try { + // There will be one proposed item but the tree will refuse the merge because of the bad state on disk + final List generatedItems = fixedAndRecurringInvoiceItemGenerator.generateItems(account, + UUID.randomUUID(), + events, + existingInvoices, + startDate, + account.getCurrency(), + new HashMap(), + internalCallContext); + fail(); + } catch (final IllegalStateException e) { + assertTrue(e.getMessage().startsWith("Double billing detected")); + } + } + + // Simulate a bug in the generator where two fixed items for the same day and subscription end up in the resulting items + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testTooManyFixedInvoiceItemsForGivenSubscriptionAndStartDatePostMerge() throws InvoiceApiException { + final Multimap createdItemsPerDayPerSubscription = LinkedListMultimap.create(); + final LocalDate startDate = new LocalDate("2016-01-01"); + + final Collection resultingItems = new LinkedList(); + final InvoiceItem fixedPriceInvoiceItem = new FixedPriceInvoiceItem(UUID.randomUUID(), + clock.getUTCNow(), + null, + account.getId(), + subscription.getBundleId(), + subscription.getId(), + "planName", + "phaseName", + "description", + startDate, + BigDecimal.ONE, + account.getCurrency()); + resultingItems.add(fixedPriceInvoiceItem); + resultingItems.add(fixedPriceInvoiceItem); + + try { + fixedAndRecurringInvoiceItemGenerator.safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); + fail(); + } catch (final InvoiceApiException e) { + assertEquals(e.getCode(), ErrorCode.UNEXPECTED_ERROR.getCode()); + } + + resultingItems.clear(); + for (int i = 0; i < 2; i++) { + resultingItems.add(new FixedPriceInvoiceItem(UUID.randomUUID(), + clock.getUTCNow(), + null, + account.getId(), + subscription.getBundleId(), + subscription.getId(), + "planName", + "phaseName", + "description", + startDate, + // Amount shouldn't have any effect + BigDecimal.ONE.add(new BigDecimal(i)), + account.getCurrency())); + } + + try { + fixedAndRecurringInvoiceItemGenerator.safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); + fail(); + } catch (final InvoiceApiException e) { + assertEquals(e.getCode(), ErrorCode.UNEXPECTED_ERROR.getCode()); + } + } + + // Simulate a bug in the generator where two recurring items for the same service period and subscription end up in the resulting items + @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/664") + public void testTooManyRecurringInvoiceItemsForGivenSubscriptionAndServicePeriodPostMerge() throws InvoiceApiException { + final Multimap createdItemsPerDayPerSubscription = LinkedListMultimap.create(); + final LocalDate startDate = new LocalDate("2016-01-01"); + + final Collection resultingItems = new LinkedList(); + final InvoiceItem recurringInvoiceItem = new RecurringInvoiceItem(UUID.randomUUID(), + clock.getUTCNow(), + null, + account.getId(), + subscription.getBundleId(), + subscription.getId(), + "planName", + "phaseName", + startDate, + startDate.plusMonths(1), + BigDecimal.ONE, + BigDecimal.ONE, + account.getCurrency()); + resultingItems.add(recurringInvoiceItem); + resultingItems.add(recurringInvoiceItem); + + try { + fixedAndRecurringInvoiceItemGenerator.safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); + fail(); + } catch (final InvoiceApiException e) { + assertEquals(e.getCode(), ErrorCode.UNEXPECTED_ERROR.getCode()); + } + + resultingItems.clear(); + for (int i = 0; i < 2; i++) { + resultingItems.add(new RecurringInvoiceItem(UUID.randomUUID(), + clock.getUTCNow(), + null, + account.getId(), + subscription.getBundleId(), + subscription.getId(), + "planName", + "phaseName", + startDate, + startDate.plusMonths(1), + // Amount shouldn't have any effect + BigDecimal.TEN, + BigDecimal.ONE, + account.getCurrency())); + } + + try { + fixedAndRecurringInvoiceItemGenerator.safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); + fail(); + } catch (final InvoiceApiException e) { + assertEquals(e.getCode(), ErrorCode.UNEXPECTED_ERROR.getCode()); + } + } } diff --git a/util/src/main/java/org/killbill/billing/util/config/definition/InvoiceConfig.java b/util/src/main/java/org/killbill/billing/util/config/definition/InvoiceConfig.java index 776621bf3e..b2ac2b0b03 100644 --- a/util/src/main/java/org/killbill/billing/util/config/definition/InvoiceConfig.java +++ b/util/src/main/java/org/killbill/billing/util/config/definition/InvoiceConfig.java @@ -36,6 +36,16 @@ public interface InvoiceConfig extends KillbillConfig { @Description("Maximum target date to consider when generating an invoice") int getNumberOfMonthsInFuture(@Param("dummy") final InternalTenantContext tenantContext); + @Config("org.killbill.invoice.sanitySafetyBoundEnabled") + @Default("true") + @Description("Whether internal sanity checks to prevent mis- and double-billing are enabled") + boolean isSanitySafetyBoundEnabled(); + + @Config("org.killbill.invoice.sanitySafetyBoundEnabled") + @Default("true") + @Description("Whether internal sanity checks to prevent mis- and double-billing are enabled") + boolean isSanitySafetyBoundEnabled(@Param("dummy") final InternalTenantContext tenantContext); + @Config("org.killbill.invoice.maxDailyNumberOfItemsSafetyBound") @Default("15") @Description("Maximum daily number of invoice items to generate for a subscription id")