New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for NPE when `paymentMethodProps.getProperties()` returns null. #1094

Merged
merged 2 commits into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@beccagaspard
Copy link
Contributor

beccagaspard commented Feb 8, 2019

Fix for NPE when paymentMethodProps.getProperties() returns null.

When payment control plugins are in use and an __EXTERNAL_PAYMENT__ payment method is added via AccountApi::createPaymentMethod, a NPE is thrown. Added check to skip merge when payment method properties are null.

Full stacktrace:

java.lang.NullPointerException: null
	at org.killbill.billing.util.PluginProperties.toMap(PluginProperties.java:41)
	at org.killbill.billing.util.PluginProperties.merge(PluginProperties.java:34)
	at org.killbill.billing.payment.core.PaymentMethodProcessor.addPaymentMethodWithControl(PaymentMethodProcessor.java:181)
	at org.killbill.billing.payment.api.DefaultPaymentApi.addPaymentMethodWithPaymentControl(DefaultPaymentApi.java:995)
	at org.killbill.billing.util.glue.KillbillApiAopModule$ProfilingMethodInterceptor$1.execute(KillbillApiAopModule.java:76)
	at org.killbill.commons.profiling.Profiling.executeWithProfiling(Profiling.java:33)
	at org.killbill.billing.util.glue.KillbillApiAopModule$ProfilingMethodInterceptor.invoke(KillbillApiAopModule.java:92)
	at org.killbill.billing.util.security.AopAllianceMethodInvocationAdapter.proceed(AopAllianceMethodInvocationAdapter.java:45)
	at org.apache.shiro.authz.aop.AuthorizingAnnotationMethodInterceptor.invoke(AuthorizingAnnotationMethodInterceptor.java:68)
	at org.killbill.billing.util.security.AopAllianceMethodInterceptorAdapter.invoke(AopAllianceMethodInterceptorAdapter.java:32)
	at org.killbill.billing.jaxrs.resources.AccountResource.createPaymentMethod(AccountResource.java:832)
	at org.killbill.commons.skeleton.metrics.TimedResourceInterceptor.invoke(TimedResourceInterceptor.java:69)
	at org.killbill.billing.server.modules.JaxRSAopModule$JaxRsMethodInterceptor$1.execute(JaxRSAopModule.java:87)
	at org.killbill.billing.util.entity.dao.DBRouterUntyped.withRODBIAllowed(DBRouterUntyped.java:57)
	at org.killbill.billing.server.modules.JaxRSAopModule$JaxRsMethodInterceptor.invoke(JaxRSAopModule.java:82)
	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.RightHandPathRule.accept(RightHandPathRule.java:147)
	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:731)
	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.doFilter(TenantFilter.java:111)
	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.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	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.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at com.codahale.metrics.servlet.AbstractInstrumentedFilter.doFilter(AbstractInstrumentedFilter.java:111)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:221)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:122)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:505)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:169)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:103)
	at org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:956)
	at org.apache.catalina.valves.RemoteIpValve.invoke(RemoteIpValve.java:683)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:116)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:436)
	at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1078)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:625)
	at org.apache.tomcat.util.net.AprEndpoint$SocketProcessor.doRun(AprEndpoint.java:2517)
	at org.apache.tomcat.util.net.AprEndpoint$SocketProcessor.run(AprEndpoint.java:2506)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.lang.Thread.run(Thread.java:748)
@sbrossie

This comment has been minimized.

Copy link
Member

sbrossie commented Feb 8, 2019

I am fine with the PR as it is, but i am wondering if the fix should not be at the PluginProperties#merge level to handle the null case.

Also, in our JAXRS layer, probably retuning null in the first place is not so great - an ImmutableList.of() should be better i think.

👍

Guard against NPE in PluginProperties::merge. Return empty list inste…
…ad of null when there are no PluginProperties in PaymentMethodJson.
@beccagaspard

This comment has been minimized.

Copy link
Contributor Author

beccagaspard commented Feb 8, 2019

Hey @sbrossie - made the changes as you suggested. Should I revert the original change in PaymentMethodProcessor as it should no longer be necessary?

@sbrossie

This comment has been minimized.

Copy link
Member

sbrossie commented Feb 8, 2019

Hey @sbrossie - made the changes as you suggested. Should I revert the original change in PaymentMethodProcessor as it should no longer be necessary?

I think it's fine to keep. Happy to merge it . @pierre Any objections?

@pierre

pierre approved these changes Feb 11, 2019

Copy link
Member

pierre left a comment

Thanks!

@pierre pierre merged commit b667017 into killbill:master Feb 11, 2019

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: integration-tests Your tests passed on CircleCI!
Details
ci/circleci: test-h2 Your tests passed on CircleCI!
Details
ci/circleci: test-mysql Your tests passed on CircleCI!
Details
ci/circleci: test-postgresql Your tests passed on CircleCI!
Details

@beccagaspard beccagaspard deleted the beccagaspard:add_payment_method_with_control_null_pointer_fix branch Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment