catalog: Ensure that Kill Bill services populate full TenantContext (…

…incl accountId) before calling catalog apis.

Introduce a new CatalogInternalApi (and remove use of CatalogService) to ensure that Kill Bill services use different apis.
The catalog code adds special validation to ensure that such TenantContext (when coming from inetrnal apis) is fully populated.

api/src/main/java/org/killbill/billing/catalog/api/CatalogService.java → api/src/main/java/org/killbill/billing/catalog/api/CatalogInternalApi.java

@@ -1,7 +1,6 @@
 /*
- * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014 Groupon, Inc
- * Copyright 2014 The Billing Project, LLC
+ * Copyright 2014-2017 Groupon, Inc
+ * Copyright 2014-2017 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -19,14 +18,12 @@
 package org.killbill.billing.catalog.api;
 
 import org.killbill.billing.callcontext.InternalTenantContext;
-import org.killbill.billing.platform.api.KillbillService;
 
-/**
- * The interface {@code CatalogService} is a {@code KillbillService} required to handle catalog operations.
- */
-public interface CatalogService extends KillbillService {
+public interface CatalogInternalApi {
 
     public Catalog getFullCatalog(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
 
+
     public StaticCatalog getCurrentCatalog(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
+
 }

catalog/src/main/java/org/killbill/billing/catalog/DefaultCatalogService.java

@@ -92,15 +92,25 @@ public String getName() {
 
     @Override
     public Catalog getFullCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
-        return getCatalog(useDefaultCatalog, filterTemplateCatalog, context);
+        return getCatalog(useDefaultCatalog, filterTemplateCatalog, false, context);
+    }
+
+    @Override
+    public Catalog getFullCatalogForInternalUse(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
+        return getCatalog(useDefaultCatalog, filterTemplateCatalog, true, context);
     }
 
     @Override
     public StaticCatalog getCurrentCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
-        return getCatalog(useDefaultCatalog, filterTemplateCatalog, context);
+        return getCatalog(useDefaultCatalog, filterTemplateCatalog, false, context);
+    }
+
+    @Override
+    public StaticCatalog getCurrentCatalogForInternalUse(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
+        return getCatalog(useDefaultCatalog, filterTemplateCatalog, true, context);
     }
 
-    private VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
-        return catalogCache.getCatalog(useDefaultCatalog, filterTemplateCatalog, context);
+    private VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final boolean internalUse, final InternalTenantContext context) throws CatalogApiException {
+        return catalogCache.getCatalog(useDefaultCatalog, filterTemplateCatalog, internalUse, context);
     }
 }

catalog/src/main/java/org/killbill/billing/catalog/api/CatalogService.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2014-2017 Groupon, Inc
+ * Copyright 2014-2017 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
+ * License.  You may obtain a copy of the License at:
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.killbill.billing.catalog.api;
+
+import org.killbill.billing.callcontext.InternalTenantContext;
+import org.killbill.billing.platform.api.KillbillService;
+
+/**
+ * The interface {@code CatalogService} is a {@code KillbillService} required to handle catalog operations.
+ */
+public interface CatalogService extends KillbillService {
+
+    public Catalog getFullCatalog(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
+
+    public Catalog getFullCatalogForInternalUse(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
+
+    public StaticCatalog getCurrentCatalog(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
+
+    public StaticCatalog getCurrentCatalogForInternalUse(boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) throws CatalogApiException;
+}

catalog/src/main/java/org/killbill/billing/catalog/api/DefaultCatalogInternalApi.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2014-2017 Groupon, Inc
+ * Copyright 2014-2017 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
+ * License.  You may obtain a copy of the License at:
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.killbill.billing.catalog.api;
+
+import javax.inject.Inject;
+
+import org.killbill.billing.callcontext.InternalTenantContext;
+import org.killbill.billing.util.callcontext.InternalCallContextFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DefaultCatalogInternalApi implements CatalogInternalApi {
+
+    private final Logger logger = LoggerFactory.getLogger(DefaultCatalogInternalApi.class);
+
+    private final CatalogService catalogService;
+
+    @Inject
+    public DefaultCatalogInternalApi(final CatalogService catalogService,
+                                     final InternalCallContextFactory internalCallContextFactory) {
+        this.catalogService = catalogService;
+    }
+
+    @Override
+    public Catalog getFullCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
+        return catalogService.getFullCatalogForInternalUse(useDefaultCatalog, filterTemplateCatalog, context);
+    }
+
+    @Override
+    public StaticCatalog getCurrentCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext context) throws CatalogApiException {
+        return catalogService.getCurrentCatalogForInternalUse(useDefaultCatalog, filterTemplateCatalog, context);
+    }
+}

catalog/src/main/java/org/killbill/billing/catalog/caching/CatalogCache.java

@@ -25,7 +25,7 @@ public interface CatalogCache {
 
     public void loadDefaultCatalog(final String url) throws CatalogApiException;
 
-    public VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext tenantContext) throws CatalogApiException;
+    public VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final boolean internalUse, InternalTenantContext tenantContext) throws CatalogApiException;
 
     public void clearCatalog(InternalTenantContext tenantContext);
 

catalog/src/main/java/org/killbill/billing/catalog/caching/EhCacheCatalogCache.java

@@ -48,6 +48,7 @@
 import org.slf4j.LoggerFactory;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 
 public class EhCacheCatalogCache implements CatalogCache {
@@ -91,7 +92,17 @@ public void loadDefaultCatalog(final String url) throws CatalogApiException {
     }
 
     @Override
-    public VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final InternalTenantContext tenantContext) throws CatalogApiException {
+    public VersionedCatalog getCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, final boolean internalUse, final InternalTenantContext tenantContext) throws CatalogApiException {
+
+        //
+        // This is used by Kill Bill services (subscription/invoice/... creation/change)
+        //
+        // The goal is to ensure that on such operations any catalog plugin would also receive a TenantContext that contains both tenantId AND accountID (and could therefore run some optimization to return
+        // specific pieces of catalog for certain account). In such a scenario plugin would have to make sure that Kill Bill does not cache the catalog (since this is only cached at the tenant level)
+        //
+        if (internalUse) {
+            Preconditions.checkState(tenantContext.getAccountRecordId() !=null, "Unexpected null accountRecordId in context issued from internal Kill Bill service");
+        }
 
         final VersionedCatalog pluginVersionedCatalog = getCatalogFromPlugins(tenantContext);
         if (pluginVersionedCatalog != null) {

catalog/src/main/java/org/killbill/billing/catalog/glue/CatalogModule.java

@@ -19,8 +19,10 @@
 package org.killbill.billing.catalog.glue;
 
 import org.killbill.billing.catalog.DefaultCatalogService;
+import org.killbill.billing.catalog.api.CatalogInternalApi;
 import org.killbill.billing.catalog.api.CatalogService;
 import org.killbill.billing.catalog.api.CatalogUserApi;
+import org.killbill.billing.catalog.api.DefaultCatalogInternalApi;
 import org.killbill.billing.catalog.api.user.DefaultCatalogUserApi;
 import org.killbill.billing.catalog.caching.CatalogCache;
 import org.killbill.billing.catalog.caching.CatalogCacheInvalidationCallback;
@@ -72,6 +74,10 @@ protected void installCatalogUserApi() {
         bind(CatalogUserApi.class).to(DefaultCatalogUserApi.class).asEagerSingleton();
     }
 
+    protected void installCatalogInternalApi() {
+        bind(CatalogInternalApi.class).to(DefaultCatalogInternalApi.class).asEagerSingleton();
+    }
+
     public void installCatalogConfigCache() {
         bind(CatalogCache.class).to(EhCacheCatalogCache.class).asEagerSingleton();
         bind(CacheInvalidationCallback.class).annotatedWith(Names.named(CATALOG_INVALIDATION_CALLBACK)).to(CatalogCacheInvalidationCallback.class).asEagerSingleton();
@@ -91,6 +97,7 @@ protected void configure() {
         installCatalogDao();
         installCatalog();
         installCatalogUserApi();
+        installCatalogInternalApi();
         installCatalogConfigCache();
         installCatalogPluginApi();
     }

catalog/src/test/java/org/killbill/billing/catalog/MockCatalogModule.java

@@ -21,6 +21,7 @@
 import org.killbill.billing.callcontext.InternalCallContext;
 import org.killbill.billing.catalog.api.Catalog;
 import org.killbill.billing.catalog.api.CatalogApiException;
+import org.killbill.billing.catalog.api.CatalogInternalApi;
 import org.killbill.billing.catalog.api.CatalogService;
 import org.killbill.billing.platform.api.KillbillConfigSource;
 import org.killbill.billing.util.glue.KillBillModule;
@@ -37,10 +38,14 @@ protected void configure() {
         final Catalog catalog = Mockito.mock(Catalog.class);
 
         final CatalogService catalogService = Mockito.mock(CatalogService.class);
+        final CatalogInternalApi catalogInternalApi = Mockito.mock(CatalogInternalApi.class);
         try {
+            Mockito.when(catalogService.getCurrentCatalogForInternalUse(Mockito.any(Boolean.class), Mockito.any(Boolean.class), Mockito.any(InternalCallContext.class))).thenReturn(new MockCatalog());
+            Mockito.when(catalogService.getFullCatalogForInternalUse(Mockito.any(Boolean.class), Mockito.any(Boolean.class), Mockito.any(InternalCallContext.class))).thenReturn(catalog);
             Mockito.when(catalogService.getCurrentCatalog(Mockito.any(Boolean.class), Mockito.any(Boolean.class), Mockito.any(InternalCallContext.class))).thenReturn(new MockCatalog());
             Mockito.when(catalogService.getFullCatalog(Mockito.any(Boolean.class), Mockito.any(Boolean.class), Mockito.any(InternalCallContext.class))).thenReturn(catalog);
             bind(CatalogService.class).toInstance(catalogService);
+            bind(CatalogInternalApi.class).toInstance(catalogInternalApi);
         } catch (CatalogApiException e) {
             throw new RuntimeException(e);
         }

catalog/src/test/java/org/killbill/billing/catalog/MockCatalogService.java

@@ -39,6 +39,16 @@ public String getName() {
         return "Mock Catalog";
     }
 
+    @Override
+    public Catalog getFullCatalogForInternalUse(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) {
+        return catalog;
+    }
+
+    @Override
+    public StaticCatalog getCurrentCatalogForInternalUse(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) {
+        return catalog;
+    }
+
     @Override
     public Catalog getFullCatalog(final boolean useDefaultCatalog, final boolean filterTemplateCatalog, InternalTenantContext context) {
         return catalog;

catalog/src/test/java/org/killbill/billing/catalog/caching/TestEhCacheCatalogCache.java

@@ -73,7 +73,7 @@ protected void beforeMethod() throws Exception {
     @Test(groups = "fast")
     public void testMissingDefaultCatalog() throws CatalogApiException {
         catalogCache.loadDefaultCatalog(null);
-        Assert.assertEquals(catalogCache.getCatalog(true, true, internalCallContext).getCatalogName(), "EmptyCatalog");
+        Assert.assertEquals(catalogCache.getCatalog(true, true, false, internalCallContext).getCatalogName(), "EmptyCatalog");
     }
 
     //
@@ -83,7 +83,7 @@ public void testMissingDefaultCatalog() throws CatalogApiException {
     public void testDefaultCatalog() throws CatalogApiException {
         catalogCache.loadDefaultCatalog(Resources.getResource("SpyCarBasic.xml").toExternalForm());
 
-        final VersionedCatalog result = catalogCache.getCatalog(true, true, internalCallContext);
+        final VersionedCatalog result = catalogCache.getCatalog(true, true, false, internalCallContext);
         Assert.assertNotNull(result);
         final Collection<Product> products = result.getProducts(clock.getUTCNow());
         Assert.assertEquals(products.size(), 3);
@@ -94,10 +94,10 @@ public void testDefaultCatalog() throws CatalogApiException {
             resultForMultiTenantContext.add(new StandaloneCatalogWithPriceOverride(cur, priceOverride, multiTenantContext.getTenantRecordId(), internalCallContextFactory));
         }
 
-        Assert.assertEquals(catalogCache.getCatalog(true, true, multiTenantContext).getCatalogName(), resultForMultiTenantContext.getCatalogName());
-        Assert.assertEquals(catalogCache.getCatalog(true, true, multiTenantContext).getVersions().size(), resultForMultiTenantContext.getVersions().size());
-        for (int i = 0; i < catalogCache.getCatalog(true, true, multiTenantContext).getVersions().size(); i++) {
-           Assert.assertEquals(((StandaloneCatalogWithPriceOverride) catalogCache.getCatalog(true, true, multiTenantContext).getVersions().get(i)).getTenantRecordId(), ((StandaloneCatalogWithPriceOverride) resultForMultiTenantContext.getVersions().get(i)).getTenantRecordId());
+        Assert.assertEquals(catalogCache.getCatalog(true, true, false, multiTenantContext).getCatalogName(), resultForMultiTenantContext.getCatalogName());
+        Assert.assertEquals(catalogCache.getCatalog(true, true, false, multiTenantContext).getVersions().size(), resultForMultiTenantContext.getVersions().size());
+        for (int i = 0; i < catalogCache.getCatalog(true, true, false, multiTenantContext).getVersions().size(); i++) {
+           Assert.assertEquals(((StandaloneCatalogWithPriceOverride) catalogCache.getCatalog(true, true, false, multiTenantContext).getVersions().get(i)).getTenantRecordId(), ((StandaloneCatalogWithPriceOverride) resultForMultiTenantContext.getVersions().get(i)).getTenantRecordId());
         }
     }
 
@@ -138,14 +138,14 @@ public List<String> answer(final InvocationOnMock invocation) throws Throwable {
         });
 
         // Verify the lookup for a non-cached tenant. No system config is set yet but EhCacheCatalogCache returns a default empty one
-        VersionedCatalog differentResult = catalogCache.getCatalog(true, true, differentMultiTenantContext);
+        VersionedCatalog differentResult = catalogCache.getCatalog(true, true, false, differentMultiTenantContext);
         Assert.assertNotNull(differentResult);
         Assert.assertEquals(differentResult.getCatalogName(), "EmptyCatalog");
 
         // Make sure the cache loader isn't invoked, see https://github.com/killbill/killbill/issues/300
         shouldThrow.set(true);
 
-        differentResult = catalogCache.getCatalog(true, true, differentMultiTenantContext);
+        differentResult = catalogCache.getCatalog(true, true, false, differentMultiTenantContext);
         Assert.assertNotNull(differentResult);
         Assert.assertEquals(differentResult.getCatalogName(), "EmptyCatalog");
 
@@ -155,30 +155,30 @@ public List<String> answer(final InvocationOnMock invocation) throws Throwable {
         catalogCache.loadDefaultCatalog(Resources.getResource("SpyCarBasic.xml").toExternalForm());
 
         // Verify the lookup for this tenant
-        final VersionedCatalog result = catalogCache.getCatalog(true, true, multiTenantContext);
+        final VersionedCatalog result = catalogCache.getCatalog(true, true, false, multiTenantContext);
         Assert.assertNotNull(result);
         final Collection<Product> products = result.getProducts(clock.getUTCNow());
         Assert.assertEquals(products.size(), 6);
 
         // Verify the lookup for another tenant
-        final VersionedCatalog otherResult = catalogCache.getCatalog(true, true, otherMultiTenantContext);
+        final VersionedCatalog otherResult = catalogCache.getCatalog(true, true, false, otherMultiTenantContext);
         Assert.assertNotNull(otherResult);
         final Collection<Product> otherProducts = otherResult.getProducts(clock.getUTCNow());
         Assert.assertEquals(otherProducts.size(), 3);
 
         shouldThrow.set(true);
 
         // Verify the lookup for this tenant
-        final VersionedCatalog result2 = catalogCache.getCatalog(true, true, multiTenantContext);
+        final VersionedCatalog result2 = catalogCache.getCatalog(true, true, false, multiTenantContext);
         Assert.assertEquals(result2, result);
 
         // Verify the lookup with another context for the same tenant
         final InternalCallContext sameMultiTenantContext = Mockito.mock(InternalCallContext.class);
         Mockito.when(sameMultiTenantContext.getAccountRecordId()).thenReturn(9102L);
         Mockito.when(sameMultiTenantContext.getTenantRecordId()).thenReturn(multiTenantRecordId);
-        Assert.assertEquals(catalogCache.getCatalog(true, true, sameMultiTenantContext), result);
+        Assert.assertEquals(catalogCache.getCatalog(true, true, false, sameMultiTenantContext), result);
 
         // Verify the lookup with the other tenant
-        Assert.assertEquals(catalogCache.getCatalog(true, true, otherMultiTenantContext), otherResult);
+        Assert.assertEquals(catalogCache.getCatalog(true, true, false, otherMultiTenantContext), otherResult);
     }
 }