From b27ba0434afe9abf4f1d4f17e40c6b44e0371b7c Mon Sep 17 00:00:00 2001 From: Arnaud Brunet Date: Fri, 13 Jan 2017 08:15:31 +1100 Subject: [PATCH] More robust instantiation in SpringClientFactory SpringClientFactory returns null if the class is not aware of IClientConfig. This change catches the exception when trying the constructor with IClientConfig so classes that aren't aware of IClientConfig can be created. Fixes gh-1608 --- .../netflix/ribbon/SpringClientFactory.java | 39 +++++++-------- .../ribbon/SpringClientFactoryTests.java | 50 +++++++++++++++++++ 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/SpringClientFactory.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/SpringClientFactory.java index 6dfdd1df..7dbccf5d 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/SpringClientFactory.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/SpringClientFactory.java @@ -16,6 +16,8 @@ package org.springframework.cloud.netflix.ribbon; +import java.lang.reflect.Constructor; + import org.springframework.beans.BeanUtils; import org.springframework.cloud.context.named.NamedContextFactory; import org.springframework.context.annotation.AnnotationConfigApplicationContext; @@ -80,33 +82,30 @@ public class SpringClientFactory extends NamedContextFactory C instantiateWithConfig(AnnotationConfigApplicationContext context, Class clazz, IClientConfig config) { C result = null; - if (IClientConfigAware.class.isAssignableFrom(clazz)) { - IClientConfigAware obj = (IClientConfigAware) BeanUtils.instantiate(clazz); - obj.initWithNiwsConfig(config); - @SuppressWarnings("unchecked") - C value = (C) obj; - result = value; + + try { + Constructor constructor = clazz.getConstructor(IClientConfig.class); + result = constructor.newInstance(config); + } catch (Throwable e) { + // Ignored } - else { - try { - if (clazz.getConstructor(IClientConfig.class) != null) { - result = clazz.getConstructor(IClientConfig.class) - .newInstance(config); - } - else { - result = BeanUtils.instantiate(clazz); - } + + if (result == null) { + result = BeanUtils.instantiate(clazz); + + if (result instanceof IClientConfigAware) { + ((IClientConfigAware) result).initWithNiwsConfig(config); } - catch (Throwable ex) { - // NOPMD + + if (context != null) { + context.getAutowireCapableBeanFactory().autowireBean(result); } } - if (context != null) { - context.getAutowireCapableBeanFactory().autowireBean(result); - } + return result; } + @Override public C getInstance(String name, Class type) { C instance = super.getInstance(name, type); if (instance != null) { diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java index bdcff563..2dd65e9c 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java @@ -23,9 +23,13 @@ import org.springframework.cloud.netflix.archaius.ArchaiusAutoConfiguration; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import com.netflix.client.DefaultLoadBalancerRetryHandler; +import com.netflix.client.IClientConfigAware; +import com.netflix.client.config.DefaultClientConfigImpl; +import com.netflix.client.config.IClientConfig; import com.netflix.niws.client.http.RestClient; import com.sun.jersey.client.apache4.ApacheHttpClient4; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.springframework.boot.test.util.EnvironmentTestUtils.addEnvironment; @@ -35,6 +39,31 @@ import static org.springframework.boot.test.util.EnvironmentTestUtils.addEnviron */ public class SpringClientFactoryTests { + public static class ClientConfigInjectedByConstructor { + + private IClientConfig clientConfig; + + public ClientConfigInjectedByConstructor(IClientConfig clientConfig) { + this.clientConfig = clientConfig; + } + } + + public static class ClientConfigInjectedByInitMethod implements IClientConfigAware { + + private IClientConfig clientConfig; + + @Override + public void initWithNiwsConfig(IClientConfig clientConfig) { + this.clientConfig = clientConfig; + } + } + + public static class NoClientConfigAware { + + public NoClientConfigAware() { + // no client config + } + } @Test public void testConfigureRetry() { @@ -65,4 +94,25 @@ public class SpringClientFactoryTests { .getHttpClient().getParams().getParameter(ClientPNames.COOKIE_POLICY)); factory.destroy(); } + + @Test + public void testInstantiateWithConfigInjectByConstructor() { + IClientConfig clientConfig = new DefaultClientConfigImpl(); + ClientConfigInjectedByConstructor instance = SpringClientFactory.instantiateWithConfig(ClientConfigInjectedByConstructor.class, clientConfig); + assertThat(instance.clientConfig).isSameAs(clientConfig); + } + + @Test + public void testInstantiateWithConfigInjectedByInitMethod() { + IClientConfig clientConfig = new DefaultClientConfigImpl(); + ClientConfigInjectedByInitMethod instance = SpringClientFactory.instantiateWithConfig(ClientConfigInjectedByInitMethod.class, clientConfig); + assertThat(instance.clientConfig).isSameAs(clientConfig); + } + + @Test + public void testInstantiateWithoutConfig() { + IClientConfig clientConfig = new DefaultClientConfigImpl(); + NoClientConfigAware instance = SpringClientFactory.instantiateWithConfig(NoClientConfigAware.class, clientConfig); + assertThat(instance).isNotNull(); + } }