Kill Bill throws 500 instead of 404 (or others)

[sbrossie]

When info is being retrieved through our caching layer, we throw a RuntimeException instead of the original Kill Bill ErrorCode. For example, retrieving a tenant by -- that does not exist:

2018-03-14T22:37:10,899+0000 lvl='WARN', log='LoggingResponse', th='qtp1071550332-28', xff='', rId='7250631c-be88-4b95-845d-708b18644e8b', aRId='', tRId='', Internal error
java.lang.RuntimeException: java.lang.IllegalStateException: TenantCacheLoader cannot find value for key swagger
	at org.killbill.billing.util.cache.EhCacheBasedCacheController.computeAndCacheValue(EhCacheBasedCacheController.java:133)
	at org.killbill.billing.util.cache.EhCacheBasedCacheController.get(EhCacheBasedCacheController.java:73)
	at org.killbill.billing.tenant.api.user.DefaultTenantUserApi.getTenantByApiKey(DefaultTenantUserApi.java:104)
	at org.killbill.billing.util.glue.KillbillApiAopModule$ProfilingMethodInterceptor$1.execute(KillbillApiAopModule.java:57)
	at org.killbill.commons.profiling.Profiling.executeWithProfiling(Profiling.java:33)
	at org.killbill.billing.util.glue.KillbillApiAopModule$ProfilingMethodInterceptor.invoke(KillbillApiAopModule.java:53)
	at org.killbill.billing.jaxrs.resources.TenantResource.getTenantByApiKey(TenantResource.java:118)
	at org.killbill.commons.skeleton.metrics.TimedResourceInterceptor.invoke(TimedResourceInterceptor.java:69)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.sun.jersey.spi.container.JavaMethodInvokerFactory$1.invoke(JavaMethodInvokerFactory.java:60)
	at com.sun.jersey.server.impl.model.method.dispatch.AbstractResourceMethodDispatchProvider$ResponseOutInvoker._dispatch(AbstractResourceMethodDispatchProvider.java:205)
	at com.sun.jersey.server.impl.model.method.dispatch.ResourceJavaMethodDispatcher.dispatch(ResourceJavaMethodDispatcher.java:75)
	at com.sun.jersey.server.impl.uri.rules.HttpMethodRule.accept(HttpMethodRule.java:302)
	at com.sun.jersey.server.impl.uri.rules.ResourceClassRule.accept(ResourceClassRule.java:108)
	at com.sun.jersey.server.impl.uri.rules.RightHandPathRule.accept(RightHandPathRule.java:147)
	at com.sun.jersey.server.impl.uri.rules.RootResourceClassesRule.accept(RootResourceClassesRule.java:84)
	at com.sun.jersey.server.impl.application.WebApplicationImpl._handleRequest(WebApplicationImpl.java:1542)
	at com.sun.jersey.server.impl.application.WebApplicationImpl._handleRequest(WebApplicationImpl.java:1473)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:1419)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:1409)
	at com.sun.jersey.spi.container.servlet.WebComponent.service(WebComponent.java:409)
	at com.sun.jersey.spi.container.servlet.ServletContainer.service(ServletContainer.java:558)
	at com.sun.jersey.spi.container.servlet.ServletContainer.service(ServletContainer.java:733)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
	at com.google.inject.servlet.ServletDefinition.doServiceImpl(ServletDefinition.java:286)
	at com.google.inject.servlet.ServletDefinition.doService(ServletDefinition.java:276)
	at com.google.inject.servlet.ServletDefinition.service(ServletDefinition.java:181)
	at com.google.inject.servlet.ManagedServletPipeline.service(ManagedServletPipeline.java:91)
	at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:85)
	at org.killbill.billing.server.security.TenantFilter.handleAuthenticationError(TenantFilter.java:120)
	at org.killbill.billing.server.security.TenantFilter.doFilter(TenantFilter.java:99)
	at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:82)
	at org.killbill.billing.server.filters.ResponseCorsFilter.doFilter(ResponseCorsFilter.java:75)
	at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:82)
	at ch.qos.logback.classic.helpers.MDCInsertingServletFilter.doFilter(MDCInsertingServletFilter.java:49)
	at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:82)
	at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:120)
	at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:135)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
	at org.apache.shiro.guice.web.SimpleFilterChain.doFilter(SimpleFilterChain.java:44)
	at org.apache.shiro.web.servlet.AdviceFilter.executeChain(AdviceFilter.java:108)
	at org.apache.shiro.web.servlet.AdviceFilter.doFilterInternal(AdviceFilter.java:137)
	at org.apache.shiro.web.servlet.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:125)
	at org.apache.shiro.guice.web.SimpleFilterChain.doFilter(SimpleFilterChain.java:41)
	at org.apache.shiro.web.servlet.AbstractShiroFilter.executeChain(AbstractShiroFilter.java:449)
	at org.apache.shiro.web.servlet.AbstractShiroFilter$1.call(AbstractShiroFilter.java:365)
	at org.apache.shiro.subject.support.SubjectCallable.doCall(SubjectCallable.java:90)
	at org.apache.shiro.subject.support.SubjectCallable.call(SubjectCallable.java:83)
	at org.apache.shiro.subject.support.DelegatingSubject.execute(DelegatingSubject.java:383)
	at org.apache.shiro.web.servlet.AbstractShiroFilter.doFilterInternal(AbstractShiroFilter.java:362)
	at org.apache.shiro.web.servlet.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:125)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
	at com.codahale.metrics.servlet.AbstractInstrumentedFilter.doFilter(AbstractInstrumentedFilter.java:111)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:585)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:577)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:223)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1127)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:515)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1061)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:215)
	at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:110)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97)
	at org.eclipse.jetty.server.Server.handle(Server.java:497)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:310)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257)
	at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:540)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:635)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:555)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.IllegalStateException: TenantCacheLoader cannot find value for key swagger
	at org.killbill.billing.util.cache.TenantCacheLoader.compute(TenantCacheLoader.java:49)
	at org.killbill.billing.util.cache.TenantCacheLoader.compute(TenantCacheLoader.java:28)
	at org.killbill.billing.util.cache.EhCacheBasedCacheController.computeAndCacheValue(EhCacheBasedCacheController.java:129)
	... 75 common frames omitted

[sbrossie]

The main issue with fixing this bug is that the logic that we have in our cache loaders with respect to what constitutes an exception that should be propagated versus a entry that is not found is not consistent across subsystems.

For example:

• In the case of the TenantCacheLoader, if we don't find an entry, the code currently throws an IllegalStateException. As a result, fetching a tenant using an unknown key will propagate this exception all the way up, leading to a 500, instead of the expected 404.
• In the case of the TenantStateMachineConfigCacheLoader, the same IllegalStateException is thrown to indicate a real exception that we would like to propagate all the way up. An example of such case is one where the call to tenantApi#getPluginPaymentStateMachineConfig throws a RuntimeException as indicated in this test
• In the case of the TenantCatalogCacheLoader, the same IllegalStateException is thrown to indicate an underlying catalog exception, which itself could originate from very different causes.

There is a common piece of logic on top of all these loaders that needs to decide whether to propagate the underlying IllegalStateException or not, and currently the code decides to propagate the exception as a RuntimeException.

So, we are left with pieces of logic like this or this, and then we still have missing pieces such as the one discussed in this thread with regards to correct answer when fetching tenants that don't exist.