From afb43c5883d4c7e486c5dd4e868afa368dc65007 Mon Sep 17 00:00:00 2001
From: Dave Syer <dsyer@pivotal.io>
Date: Thu, 21 Jul 2016 10:35:22 +0100
Subject: [PATCH] Actually fix the feign client validation issue

We need to still assert that the FeignClient is valid (no name and
value specified together for instance). Added missing tests.
---
 .../netflix/feign/FeignClientsRegistrar.java  |  9 ++--
 .../invalid/FeignClientValidationTests.java   | 47 ++++++++++++++++++-
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java
index cd111ed3..ff1a6728 100644
--- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java
+++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java
@@ -155,8 +155,6 @@ class FeignClientsRegistrar implements ImportBeanDefinitionRegistrar,
 					Map<String, Object> attributes = annotationMetadata
 							.getAnnotationAttributes(
 									FeignClient.class.getCanonicalName());
-					// Spring 4.2 didn't do this for us. With 4.3 it's idempotent.
-					attributes = AnnotationAttributes.fromMap(attributes);
 
 					String name = getClientName(attributes);
 					registerClientConfiguration(registry, name,
@@ -192,10 +190,9 @@ class FeignClientsRegistrar implements ImportBeanDefinitionRegistrar,
 	}
 
 	private void validate(Map<String, Object> attributes) {
-		if (StringUtils.hasText((String) attributes.get("value"))) {
-			Assert.isTrue(!StringUtils.hasText((String) attributes.get("serviceId")),
-					"Either name (serviceId) or value can be specified, but not both");
-		}
+		AnnotationAttributes annotation = AnnotationAttributes.fromMap(attributes);
+		// This blows up if an aliased property is overspecified
+		annotation.getAliasedString("name", FeignClient.class, null);
 	}
 
 	private String getName(Map<String, Object> attributes) {
diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java
index 4ec6bac3..178fc643 100644
--- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java
+++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java
@@ -39,6 +39,50 @@ public class FeignClientValidationTests {
 	@Rule
 	public ExpectedException expected = ExpectedException.none();
 
+	@Test
+	public void testNameAndValue() {
+		this.expected.expectMessage("only one is permitted");
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				NameAndValueConfiguration.class);
+		assertNotNull(context.getBean(NameAndValueConfiguration.Client.class));
+		context.close();
+	}
+
+	@Configuration
+	@Import(FeignAutoConfiguration.class)
+	@EnableFeignClients(clients = NameAndValueConfiguration.Client.class)
+	protected static class NameAndValueConfiguration {
+
+		@FeignClient(value = "foo", name = "bar")
+		interface Client {
+			@RequestMapping(method = RequestMethod.GET, value = "/")
+			String get();
+		}
+
+	}
+
+	@Test
+	public void testServiceIdAndValue() {
+		this.expected.expectMessage("only one is permitted");
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				NameAndValueConfiguration.class);
+		assertNotNull(context.getBean(NameAndServiceIdConfiguration.Client.class));
+		context.close();
+	}
+
+	@Configuration
+	@Import(FeignAutoConfiguration.class)
+	@EnableFeignClients(clients = NameAndServiceIdConfiguration.Client.class)
+	protected static class NameAndServiceIdConfiguration {
+
+		@FeignClient(serviceId = "foo", name = "bar")
+		interface Client {
+			@RequestMapping(method = RequestMethod.GET, value = "/")
+			String get();
+		}
+
+	}
+
 	@Test
 	public void testNotLegalHostname() {
 		this.expected.expectMessage("not legal hostname (foo_bar)");
@@ -115,7 +159,8 @@ public class FeignClientValidationTests {
 			return new Dummy();
 		}
 
-		class Dummy { }
+		class Dummy {
+		}
 
 	}
 }