killbill-memoizeit

Merge pull request #855 from killbill/fix-for-842 catalog:

1/31/2018 2:59:50 AM

Details

diff --git a/catalog/src/main/java/org/killbill/billing/catalog/StandaloneCatalogWithPriceOverride.java b/catalog/src/main/java/org/killbill/billing/catalog/StandaloneCatalogWithPriceOverride.java
index d7a2d9b..473debf 100644
--- a/catalog/src/main/java/org/killbill/billing/catalog/StandaloneCatalogWithPriceOverride.java
+++ b/catalog/src/main/java/org/killbill/billing/catalog/StandaloneCatalogWithPriceOverride.java
@@ -19,6 +19,7 @@ package org.killbill.billing.catalog;
 
 import java.util.regex.Matcher;
 
+import org.killbill.billing.ErrorCode;
 import org.killbill.billing.callcontext.InternalCallContext;
 import org.killbill.billing.callcontext.InternalTenantContext;
 import org.killbill.billing.catalog.api.CatalogApiException;
@@ -69,7 +70,6 @@ public class StandaloneCatalogWithPriceOverride extends StandaloneCatalog implem
         return internalCallContextFactory;
     }
 
-
     @Override
     public Plan createOrFindCurrentPlan(final PlanSpecifier spec, final PlanPhasePriceOverridesWithCallContext overrides) throws CatalogApiException {
         final Plan defaultPlan = super.createOrFindCurrentPlan(spec, null);
@@ -103,8 +103,10 @@ public class StandaloneCatalogWithPriceOverride extends StandaloneCatalog implem
     public DefaultPlan findCurrentPlan(final String planName) throws CatalogApiException {
         final Matcher m = DefaultPriceOverride.CUSTOM_PLAN_NAME_PATTERN.matcher(planName);
         if (m.matches()) {
-            final InternalTenantContext internalTenantContext = createInternalTenantContext();
-            return priceOverride.getOverriddenPlan(planName, this, internalTenantContext);
+            final DefaultPlan plan = maybeGetOverriddenPlan(planName);
+            if (plan != null) {
+                return plan;
+            }
         }
         return super.findCurrentPlan(planName);
     }
@@ -119,13 +121,32 @@ public class StandaloneCatalogWithPriceOverride extends StandaloneCatalog implem
         final String planName = DefaultPlanPhase.planName(phaseName);
         final Matcher m = DefaultPriceOverride.CUSTOM_PLAN_NAME_PATTERN.matcher(planName);
         if (m.matches()) {
-            final InternalTenantContext internalTenantContext = createInternalTenantContext();
-            final Plan plan = priceOverride.getOverriddenPlan(planName, this, internalTenantContext);
-            return plan.findPhase(phaseName);
+            final DefaultPlan plan = maybeGetOverriddenPlan(planName);
+            if (plan != null) {
+                return plan.findPhase(phaseName);
+            }
         }
         return super.findCurrentPhase(phaseName);
     }
 
+    private DefaultPlan maybeGetOverriddenPlan(final String planName) throws CatalogApiException {
+        final InternalTenantContext internalTenantContext = createInternalTenantContext();
+
+        try {
+            return priceOverride.getOverriddenPlan(planName, this, internalTenantContext);
+        } catch (final RuntimeException e) {
+            if (e.getCause() == null ||
+                e.getCause().getCause() == null ||
+                !(e.getCause().getCause() instanceof CatalogApiException) ||
+                ((CatalogApiException) e.getCause().getCause()).getCode() != ErrorCode.CAT_NO_SUCH_PLAN.getCode()) {
+                throw e;
+            } else {
+                // Otherwise, ambiguous name? See https://github.com/killbill/killbill/issues/842.
+                return null;
+            }
+        }
+    }
+
     private InternalTenantContext createInternalTenantContext() {
         return internalCallContextFactory.createInternalTenantContext(tenantRecordId, null);
     }
diff --git a/catalog/src/test/java/org/killbill/billing/catalog/TestCatalogUpdater.java b/catalog/src/test/java/org/killbill/billing/catalog/TestCatalogUpdater.java
index f3b9caf..97ab5e8 100644
--- a/catalog/src/test/java/org/killbill/billing/catalog/TestCatalogUpdater.java
+++ b/catalog/src/test/java/org/killbill/billing/catalog/TestCatalogUpdater.java
@@ -1,6 +1,6 @@
 /*
- * Copyright 2014-2016 Groupon, Inc
- * Copyright 2014-2016 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 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
@@ -31,10 +31,12 @@ import org.killbill.billing.catalog.api.Currency;
 import org.killbill.billing.catalog.api.MutableStaticCatalog;
 import org.killbill.billing.catalog.api.PhaseType;
 import org.killbill.billing.catalog.api.Plan;
+import org.killbill.billing.catalog.api.PlanPhase;
 import org.killbill.billing.catalog.api.PriceList;
 import org.killbill.billing.catalog.api.Product;
 import org.killbill.billing.catalog.api.ProductCategory;
 import org.killbill.billing.catalog.api.SimplePlanDescriptor;
+import org.killbill.billing.catalog.api.StaticCatalog;
 import org.killbill.billing.catalog.api.TimeUnit;
 import org.killbill.billing.catalog.api.user.DefaultSimplePlanDescriptor;
 import org.killbill.xmlloader.XMLLoader;
@@ -62,6 +64,37 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         assertEquals(catalog.getCurrentPlans().size(), 0);
     }
 
+    @Test(groups = "fast", description = "https://github.com/killbill/killbill/issues/842")
+    public void testCreateAmbiguousPlan() throws CatalogApiException {
+        final DateTime now = clock.getUTCNow();
+        final SimplePlanDescriptor desc = new DefaultSimplePlanDescriptor("foo-monthly-12345", "Foo", ProductCategory.BASE, Currency.EUR, BigDecimal.TEN, BillingPeriod.MONTHLY, 0, TimeUnit.UNLIMITED, ImmutableList.<String>of());
+
+        final CatalogUpdater catalogUpdater = new CatalogUpdater(BillingMode.IN_ADVANCE, now, desc.getCurrency());
+        catalogUpdater.addSimplePlanDescriptor(desc);
+        final StandaloneCatalog catalog = catalogUpdater.getCatalog();
+
+        assertEquals(catalog.getCurrentPlans().size(), 1);
+
+        final StaticCatalog standaloneCatalogWithPriceOverride = new StandaloneCatalogWithPriceOverride(catalog,
+                                                                                                        priceOverride,
+                                                                                                        internalCallContext.getTenantRecordId(),
+                                                                                                        internalCallContextFactory);
+
+        final Plan plan = catalog.findCurrentPlan("foo-monthly-12345");
+        assertEquals(plan.getName(), "foo-monthly-12345");
+
+        // Verify PriceOverride logic
+        final Plan plan2 = standaloneCatalogWithPriceOverride.findCurrentPlan("foo-monthly-12345");
+        assertEquals(plan2.getName(), "foo-monthly-12345");
+
+        final PlanPhase planPhase = catalog.findCurrentPhase("foo-monthly-12345-evergreen");
+        assertEquals(planPhase.getName(), "foo-monthly-12345-evergreen");
+
+        // Verify PriceOverride logic
+        final PlanPhase phase2 = standaloneCatalogWithPriceOverride.findCurrentPhase("foo-monthly-12345-evergreen");
+        assertEquals(phase2.getName(), "foo-monthly-12345-evergreen");
+    }
+
     @Test(groups = "fast")
     public void testAddNoTrialPlanOnFirstCatalog() throws CatalogApiException {
 
@@ -102,7 +135,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         assertEquals(priceList.getPlans().iterator().next().getName(), "foo-monthly");
     }
 
-
     @Test(groups = "fast")
     public void testAddTrialPlanOnFirstCatalog() throws CatalogApiException {
 
@@ -149,8 +181,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         assertEquals(priceList.getPlans().iterator().next().getName(), "foo-monthly");
     }
 
-
-
     @Test(groups = "fast")
     public void testAddPlanOnExistingCatalog() throws Exception {
 
@@ -185,8 +215,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         assertEquals(priceList.getPlans().size(), 4);
     }
 
-
-
     @Test(groups = "fast")
     public void testAddExistingPlanWithNewCurrency() throws Exception {
         final StandaloneCatalog originalCatalog = XMLLoader.getObjectFromString(Resources.getResource("SpyCarBasic.xml").toExternalForm(), StandaloneCatalog.class);
@@ -231,7 +259,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         SimplePlanDescriptor desc = new DefaultSimplePlanDescriptor("standard-monthly", "Standard", ProductCategory.BASE, Currency.EUR, BigDecimal.TEN, BillingPeriod.MONTHLY, 0, TimeUnit.DAYS, ImmutableList.<String>of());
         addBadSimplePlanDescriptor(catalogUpdater, desc);
 
-
         // Existing Plan has a 30 days trial => try different trial length
         desc = new DefaultSimplePlanDescriptor("standard-monthly", "Standard", ProductCategory.BASE, Currency.EUR, BigDecimal.TEN, BillingPeriod.MONTHLY, 14, TimeUnit.DAYS, ImmutableList.<String>of());
         addBadSimplePlanDescriptor(catalogUpdater, desc);
@@ -257,11 +284,9 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         addBadSimplePlanDescriptor(catalogUpdater, desc);
     }
 
-
     @Test(groups = "fast")
     public void testPlanWithNonFinalFixedTermPhase() throws Exception {
 
-
         final StandaloneCatalog catalog = XMLLoader.getObjectFromString(Resources.getResource("SpyCarBasic.xml").toExternalForm(), StandaloneCatalog.class);
 
         final MutableStaticCatalog mutableCatalog = new DefaultMutableStaticCatalog(catalog);
@@ -272,7 +297,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         newProduct.initialize((StandaloneCatalog) mutableCatalog, null);
         mutableCatalog.addProduct(newProduct);
 
-
         final DefaultPlanPhase trialPhase = new DefaultPlanPhase();
         trialPhase.setPhaseType(PhaseType.TRIAL);
         trialPhase.setDuration(new DefaultDuration().setUnit(TimeUnit.DAYS).setNumber(14));
@@ -284,24 +308,22 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         fixedTermPhase.setDuration(new DefaultDuration().setUnit(TimeUnit.MONTHS).setNumber(3));
         fixedTermPhase.setRecurring(new DefaultRecurring().setBillingPeriod(BillingPeriod.MONTHLY).setRecurringPrice(new DefaultInternationalPrice().setPrices(new DefaultPrice[]{new DefaultPrice().setCurrency(Currency.USD).setValue(BigDecimal.TEN)})));
 
-
         final DefaultPlanPhase evergreenPhase = new DefaultPlanPhase();
         evergreenPhase.setPhaseType(PhaseType.EVERGREEN);
         evergreenPhase.setDuration(new DefaultDuration().setUnit(TimeUnit.MONTHS).setNumber(1));
         evergreenPhase.setRecurring(new DefaultRecurring().setBillingPeriod(BillingPeriod.MONTHLY).setRecurringPrice(new DefaultInternationalPrice().setPrices(new DefaultPrice[]{new DefaultPrice().setCurrency(Currency.USD).setValue(BigDecimal.TEN)})));
 
-
         final DefaultPlan newPlan = new DefaultPlan();
         newPlan.setName("something-with-fixed-term");
         newPlan.setPriceListName(DefaultPriceListSet.DEFAULT_PRICELIST_NAME);
         newPlan.setProduct(newProduct);
-        newPlan.setInitialPhases(new DefaultPlanPhase[] {trialPhase, fixedTermPhase});
+        newPlan.setInitialPhases(new DefaultPlanPhase[]{trialPhase, fixedTermPhase});
         newPlan.setFinalPhase(fixedTermPhase);
         mutableCatalog.addPlan(newPlan);
         newPlan.initialize((StandaloneCatalog) mutableCatalog, new URI("dummy"));
 
         final String newCatalogStr = XMLWriter.writeXML((StandaloneCatalog) mutableCatalog, StandaloneCatalog.class);
-        final StandaloneCatalog newCatalog =  XMLLoader.getObjectFromStream(new URI("dummy"), new ByteArrayInputStream(newCatalogStr.getBytes(Charset.forName("UTF-8"))), StandaloneCatalog.class);
+        final StandaloneCatalog newCatalog = XMLLoader.getObjectFromStream(new URI("dummy"), new ByteArrayInputStream(newCatalogStr.getBytes(Charset.forName("UTF-8"))), StandaloneCatalog.class);
 
         final DefaultPlan targetPlan = newCatalog.findCurrentPlan("something-with-fixed-term");
         Assert.assertEquals(targetPlan.getInitialPhases().length, 2);
@@ -309,8 +331,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
 
     }
 
-
-
     @Test(groups = "fast")
     public void testVerifyXML() throws Exception {
 
@@ -555,7 +575,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         System.err.println(catalogUpdater.getCatalogXML());
     }
 
-
     private StandaloneCatalog enhanceOriginalCatalogForInvalidTestCases(final String catalogName) throws Exception {
 
         final StandaloneCatalog catalog = XMLLoader.getObjectFromString(Resources.getResource(catalogName).toExternalForm(), StandaloneCatalog.class);
@@ -588,7 +607,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         mutableCatalog.addPlan(newPlan1);
         newPlan1.initialize((StandaloneCatalog) mutableCatalog, new URI("dummy"));
 
-
         final DefaultProduct newProduct2 = new DefaultProduct();
         newProduct2.setName("SuperDynamic");
         newProduct2.setCatagory(ProductCategory.BASE);
@@ -601,7 +619,6 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         fixedterm2.setDuration(new DefaultDuration().setUnit(TimeUnit.MONTHS).setNumber(3));
         fixedterm2.setRecurring(new DefaultRecurring().setBillingPeriod(BillingPeriod.MONTHLY).setRecurringPrice(new DefaultInternationalPrice().setPrices(new DefaultPrice[]{new DefaultPrice().setCurrency(Currency.USD).setValue(BigDecimal.TEN)})));
 
-
         final DefaultPlan newPlan2 = new DefaultPlan();
         newPlan2.setName("superdynamic-fixedterm");
         newPlan2.setPriceListName(DefaultPriceListSet.DEFAULT_PRICELIST_NAME);
@@ -610,12 +627,10 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
         mutableCatalog.addPlan(newPlan2);
         newPlan2.initialize((StandaloneCatalog) mutableCatalog, new URI("dummy"));
 
-
         final String newCatalogStr = XMLWriter.writeXML((StandaloneCatalog) mutableCatalog, StandaloneCatalog.class);
         return XMLLoader.getObjectFromStream(new URI("dummy"), new ByteArrayInputStream(newCatalogStr.getBytes(Charset.forName("UTF-8"))), StandaloneCatalog.class);
     }
 
-
     private void addBadSimplePlanDescriptor(final CatalogUpdater catalogUpdater, final SimplePlanDescriptor desc) {
         try {
             catalogUpdater.addSimplePlanDescriptor(desc);
@@ -624,4 +639,4 @@ public class TestCatalogUpdater extends CatalogTestSuiteNoDB {
             assertEquals(e.getCode(), ErrorCode.CAT_FAILED_SIMPLE_PLAN_VALIDATION.getCode());
         }
     }
-}
\ No newline at end of file
+}
diff --git a/catalog/src/test/java/org/killbill/billing/catalog/TestStandaloneCatalogWithPriceOverride.java b/catalog/src/test/java/org/killbill/billing/catalog/TestStandaloneCatalogWithPriceOverride.java
index 2a70ca6..218aeda 100644
--- a/catalog/src/test/java/org/killbill/billing/catalog/TestStandaloneCatalogWithPriceOverride.java
+++ b/catalog/src/test/java/org/killbill/billing/catalog/TestStandaloneCatalogWithPriceOverride.java
@@ -1,6 +1,6 @@
 /*
- * Copyright 2014-2016 Groupon, Inc
- * Copyright 2014-2016 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 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
@@ -17,19 +17,66 @@
 
 package org.killbill.billing.catalog;
 
+import java.math.BigDecimal;
+import java.util.regex.Matcher;
+
 import org.killbill.billing.ErrorCode;
 import org.killbill.billing.catalog.api.BillingPeriod;
 import org.killbill.billing.catalog.api.CatalogApiException;
+import org.killbill.billing.catalog.api.Currency;
+import org.killbill.billing.catalog.api.Plan;
+import org.killbill.billing.catalog.api.PlanPhasePriceOverride;
+import org.killbill.billing.catalog.api.PlanPhasePriceOverridesWithCallContext;
 import org.killbill.billing.catalog.api.PlanSpecifier;
 import org.killbill.billing.catalog.api.StaticCatalog;
+import org.killbill.billing.catalog.override.DefaultPriceOverride;
 import org.killbill.xmlloader.XMLLoader;
+import org.mockito.Mockito;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.io.Resources;
 
 public class TestStandaloneCatalogWithPriceOverride extends CatalogTestSuiteWithEmbeddedDB {
 
+    @Test(groups = "slow", description = "https://github.com/killbill/killbill/issues/842")
+    public void testCreateAmbiguousPlan() throws Exception {
+        final StandaloneCatalog catalog = XMLLoader.getObjectFromString(Resources.getResource("SpyCarAdvanced.xml").toExternalForm(), StandaloneCatalog.class);
+        final StaticCatalog standaloneCatalogWithPriceOverride = new StandaloneCatalogWithPriceOverride(catalog,
+                                                                                                        priceOverride,
+                                                                                                        internalCallContext.getTenantRecordId(),
+                                                                                                        internalCallContextFactory);
+
+        // Create ambiguous plan name
+        final PlanSpecifier spec = new PlanSpecifier("standard-monthly-67890");
+        final PlanPhasePriceOverridesWithCallContext overrides = Mockito.mock(PlanPhasePriceOverridesWithCallContext.class);
+        Mockito.when(overrides.getCallContext()).thenReturn(callContext);
+        final PlanPhasePriceOverride override = new DefaultPlanPhasePriceOverride("standard-monthly-evergreen", Currency.USD, null, BigDecimal.ONE);
+        Mockito.when(overrides.getOverrides()).thenReturn(ImmutableList.of(override));
+        final Plan plan = standaloneCatalogWithPriceOverride.createOrFindCurrentPlan(spec, overrides);
+        Assert.assertTrue(plan.getName().startsWith("standard-monthly-67890-"));
+        final Matcher m = DefaultPriceOverride.CUSTOM_PLAN_NAME_PATTERN.matcher(plan.getName());
+        Assert.assertTrue(m.matches());
+
+        // From the catalog
+        Assert.assertNotNull(catalog.findCurrentPlan("standard-monthly"));
+        Assert.assertNotNull(standaloneCatalogWithPriceOverride.findCurrentPlan("standard-monthly"));
+
+        // Created on the fly
+        try {
+            catalog.findCurrentPlan(plan.getName());
+            Assert.fail();
+        } catch (final CatalogApiException e) {
+            Assert.assertEquals(e.getCode(), ErrorCode.CAT_NO_SUCH_PLAN.getCode());
+        }
+        Assert.assertNotNull(standaloneCatalogWithPriceOverride.findCurrentPlan("standard-monthly-1"));
+
+        // From the catalog
+        Assert.assertNotNull(catalog.findCurrentPlan("standard-monthly-12345"));
+        Assert.assertNotNull(standaloneCatalogWithPriceOverride.findCurrentPlan("standard-monthly-12345"));
+    }
+
     @Test(groups = "slow")
     public void testCreatePlanNoProduct() throws Exception {
         final StandaloneCatalog catalog = XMLLoader.getObjectFromString(Resources.getResource("SpyCarAdvanced.xml").toExternalForm(), StandaloneCatalog.class);
diff --git a/catalog/src/test/resources/SpyCarAdvanced.xml b/catalog/src/test/resources/SpyCarAdvanced.xml
index a08c425..bfc21c4 100644
--- a/catalog/src/test/resources/SpyCarAdvanced.xml
+++ b/catalog/src/test/resources/SpyCarAdvanced.xml
@@ -247,6 +247,39 @@
                 </recurring>
             </finalPhase>
         </plan>
+        <plan name="standard-monthly-12345">
+            <product>Standard</product>
+            <finalPhase type="EVERGREEN">
+                <duration>
+                    <unit>UNLIMITED</unit>
+                </duration>
+                <recurring>
+                    <billingPeriod>MONTHLY</billingPeriod>
+                    <recurringPrice>
+                        <price>
+                            <currency>GBP</currency>
+                            <value>75.00</value>
+                        </price>
+                        <price>
+                            <currency>EUR</currency>
+                            <value>85.00</value>
+                        </price>
+                        <price>
+                            <currency>USD</currency>
+                            <value>100.00</value>
+                        </price>
+                        <price>
+                            <currency>JPY</currency>
+                            <value>10.00</value>
+                        </price>
+                        <price>
+                            <currency>BTC</currency>
+                            <value>0.1</value>
+                        </price>
+                    </recurringPrice>
+                </recurring>
+            </finalPhase>
+        </plan>
         <plan name="sports-monthly">
             <product>Sports</product>
             <initialPhases>
@@ -967,5 +1000,10 @@
                 <plan>cia-sports-monthly</plan>
             </plans>
         </childPriceList>
+        <childPriceList name="ambiguous">
+            <plans>
+                <plan>standard-monthly-12345</plan>
+            </plans>
+        </childPriceList>
     </priceLists>
 </catalog>
diff --git a/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java b/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
index c53b879..b977301 100644
--- a/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
+++ b/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2012 Ning, Inc.
- * Copyright 2014-2017 Groupon, Inc
- * Copyright 2014-2017 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 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
@@ -128,7 +128,8 @@ public class EhCacheBasedCacheController<K, V> implements CacheController<K, V> 
         try {
             value = baseCacheLoader.compute(key, cacheLoaderArgument);
         } catch (final Exception e) {
-            logger.warn("Unable to compute cached value for key='{}' and cacheLoaderArgument='{}'", key, cacheLoaderArgument, e);
+            // Remove noisy log (might be expected, see https://github.com/killbill/killbill/issues/842)
+            //logger.warn("Unable to compute cached value for key='{}' and cacheLoaderArgument='{}'", key, cacheLoaderArgument, e);
             throw new RuntimeException(e);
         }