Skip to content

Commit

Permalink
invoice: hardening for bad state
Browse files Browse the repository at this point in the history
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 #664.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
  • Loading branch information
pierre committed Dec 16, 2016
1 parent 7ac6797 commit f6ef852
Show file tree
Hide file tree
Showing 5 changed files with 607 additions and 102 deletions.
Expand Up @@ -52,6 +52,20 @@ public int getNumberOfMonthsInFuture(final InternalTenantContext tenantContext)
return getNumberOfMonthsInFuture(); 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 @Override
public int getMaxDailyNumberOfItemsSafetyBound() { public int getMaxDailyNumberOfItemsSafetyBound() {
return staticConfig.getMaxDailyNumberOfItemsSafetyBound(); return staticConfig.getMaxDailyNumberOfItemsSafetyBound();
Expand Down
Expand Up @@ -20,6 +20,7 @@
import java.math.BigDecimal; import java.math.BigDecimal;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collection; import java.util.Collection;
import java.util.HashMap;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
Expand All @@ -40,6 +41,7 @@
import org.killbill.billing.invoice.api.Invoice; import org.killbill.billing.invoice.api.Invoice;
import org.killbill.billing.invoice.api.InvoiceApiException; import org.killbill.billing.invoice.api.InvoiceApiException;
import org.killbill.billing.invoice.api.InvoiceItem; 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.generator.InvoiceWithMetadata.SubscriptionFutureNotificationDates;
import org.killbill.billing.invoice.model.FixedPriceInvoiceItem; import org.killbill.billing.invoice.model.FixedPriceInvoiceItem;
import org.killbill.billing.invoice.model.InvalidDateSequenceException; import org.killbill.billing.invoice.model.InvalidDateSequenceException;
Expand All @@ -59,6 +61,7 @@
import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects;
import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap; import com.google.common.collect.Multimap;
import com.google.common.collect.Range;
import com.google.inject.Inject; import com.google.inject.Inject;


import static org.killbill.billing.invoice.generator.InvoiceDateUtils.calculateNumberOfWholeBillingPeriods; import static org.killbill.billing.invoice.generator.InvoiceDateUtils.calculateNumberOfWholeBillingPeriods;
Expand Down Expand Up @@ -107,7 +110,7 @@ public List<InvoiceItem> generateItems(final ImmutableAccountData account, final
accountItemTree.mergeWithProposedItems(proposedItems); accountItemTree.mergeWithProposedItems(proposedItems);


final List<InvoiceItem> resultingItems = accountItemTree.getResultingItemList(); final List<InvoiceItem> resultingItems = accountItemTree.getResultingItemList();
safetyBound(resultingItems, createdItemsPerDayPerSubscription, internalCallContext); safetyBounds(resultingItems, createdItemsPerDayPerSubscription, internalCallContext);


return resultingItems; return resultingItems;
} }
Expand Down Expand Up @@ -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 @VisibleForTesting
private void safetyBound(final Iterable<InvoiceItem> resultingItems, final Multimap<UUID, LocalDate> createdItemsPerDayPerSubscription, final InternalTenantContext internalCallContext) throws InvoiceApiException { void safetyBounds(final Iterable<InvoiceItem> resultingItems, final Multimap<UUID, LocalDate> createdItemsPerDayPerSubscription, final InternalTenantContext internalCallContext) throws InvoiceApiException {

This comment has been minimized.

Copy link
@sbrossie

sbrossie Dec 20, 2016

Member

The diff is weird. It seems like the safetyBounds did not change, just the removal of private and the new @VisibleForTesting annotation, yet the whole section is green. Am i missing something?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Dec 20, 2016

Member

Ok forget my comment above, i understand, there is a new section (and the old one was kept unchanged)..

This comment has been minimized.

Copy link
@pierre

pierre Dec 20, 2016

Author Member

Yup - FWIW I usually keep two tabs open when reviewing diffs, the actual commit to add comments, and another one with ?w=1 to remove white spaces. It helps a bit...

// 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<UUID, Multimap<LocalDate, InvoiceItem>> fixedItemsPerDateAndSubscription = new HashMap<UUID, Multimap<LocalDate, InvoiceItem>>();
final Map<UUID, Multimap<Range<LocalDate>, InvoiceItem>> recurringItemsPerServicePeriodAndSubscription = new HashMap<UUID, Multimap<Range<LocalDate>, InvoiceItem>>();
for (final InvoiceItem resultingItem : resultingItems) {
if (resultingItem.getInvoiceItemType() == InvoiceItemType.FIXED) {
if (fixedItemsPerDateAndSubscription.get(resultingItem.getSubscriptionId()) == null) {
fixedItemsPerDateAndSubscription.put(resultingItem.getSubscriptionId(), LinkedListMultimap.<LocalDate, InvoiceItem>create());
}
fixedItemsPerDateAndSubscription.get(resultingItem.getSubscriptionId()).put(resultingItem.getStartDate(), resultingItem);

final Collection<InvoiceItem> 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.<Range<LocalDate>, InvoiceItem>create());
}
final Range<LocalDate> interval = Range.<LocalDate>closedOpen(resultingItem.getStartDate(), resultingItem.getEndDate());

This comment has been minimized.

Copy link
@sbrossie

sbrossie Dec 20, 2016

Member

Would the Range also capture things like:

  • [01-01-2016, 02-01-2016)
  • [01-02-2016, 02-01-2016)

OR do the duplicate intervals have to be exactly the same?

This comment has been minimized.

Copy link
@pierre

pierre Dec 20, 2016

Author Member

Good point, it's an exact match today. I'll try to see how hard it is to enhance this... Might need a tree 😼

This comment has been minimized.

Copy link
@pierre

pierre Dec 21, 2016

Author Member

See 794a681 (done in the tree, instead of outside/on the resulting items).

recurringItemsPerServicePeriodAndSubscription.get(resultingItem.getSubscriptionId()).put(interval, resultingItem);

final Collection<InvoiceItem> 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) { if (config.getMaxDailyNumberOfItemsSafetyBound(internalCallContext) == -1) {
// Safety bound disabled // Safety bound disabled
return; return;
Expand Down

1 comment on commit f6ef852

@sbrossie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Please sign in to comment.