FeignClient Configuration Properties #1942

khannedy · 2017-05-13T15:03:14Z

This pull request is improvement for #1931, so we can configure feign client using configuration properties

…nt using configuration properties

codecov-io · 2017-05-13T15:28:57Z

Codecov Report

Merging #1942 into master will decrease coverage by 0.13%.
The diff coverage is 67.03%.

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
- Coverage   76.22%   76.09%   -0.14%     
==========================================
  Files         209      210       +1     
  Lines        6377     6467      +90     
  Branches      794      817      +23     
==========================================
+ Hits         4861     4921      +60     
- Misses       1178     1203      +25     
- Partials      338      343       +5

Impacted Files	Coverage Δ
...rk/cloud/netflix/feign/FeignAutoConfiguration.java	`57.89% <ø> (ø)`	⬆️
...ork/cloud/netflix/feign/FeignClientProperties.java	`62.26% <62.26%> (ø)`
...rk/cloud/netflix/feign/FeignClientFactoryBean.java	`74.56% <73.68%> (-0.77%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34350cb...b142140. Read the comment docs.

ryanjbaxter · 2017-05-15T14:23:06Z

docs/src/main/asciidoc/spring-cloud-netflix.adoc

@@ -1020,8 +1020,41 @@ public class FooConfiguration {

 This replaces the `SpringMvcContract` with `feign.Contract.Default` and adds a `RequestInterceptor` to the collection of `RequestInterceptor`.

+`@FeignClient` also can be configured using configuration properties. If we create both `@Configuration` bean and configuration properties, configuration properties will win. It will override `@Configuration` values.


I am not sure what is right, but my gut tells me that @Configuration beans should override properties.

There is precedent for config wins with ribbon

ryanjbaxter · 2017-05-15T14:24:06Z

...tflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientFactoryBean.java

@@ -120,9 +121,63 @@ public void setApplicationContext(ApplicationContext context) throws BeansExcept
 			builder.decode404();
 		}

+		// configure feign builder from properties if exists


I think we could clean this code up a big. Maybe have a method that extracts the properties whether from configuration beans or properties and then just sets them in the builder in one spot?

ryanjbaxter · 2017-05-15T14:29:41Z

...etflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientProperties.java

+@ConfigurationProperties("feign.client")
+public class FeignClientProperties {
+
+	private String defaultConfig = "default";


im not seeing where this is used

hi @ryanjbaxter , you can see in FeignClientFactoryBean

FeignClientProperties.FeignClientConfiguration defaultConfig = properties.getConfig().get(properties.getDefaultConfig()); if (defaultConfig != null) { configureUsingProperties(defaultConfig, builder); }

spencergibb · 2017-05-15T14:35:34Z

docs/src/main/asciidoc/spring-cloud-netflix.adoc

@@ -1020,8 +1020,41 @@ public class FooConfiguration {

 This replaces the `SpringMvcContract` with `feign.Contract.Default` and adds a `RequestInterceptor` to the collection of `RequestInterceptor`.

+`@FeignClient` also can be configured using configuration properties. If we create both `@Configuration` bean and configuration properties, configuration properties will win. It will override `@Configuration` values.


There is precedent for config wins with ribbon

spencergibb · 2017-05-15T14:36:43Z

...tflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientFactoryBean.java

+			return applicationContext.getBean(tClass);
+		} catch (NoSuchBeanDefinitionException e) {
+			try {
+				return tClass.cast(tClass.newInstance());


We should be using spring to create beans. You can see how ribbon does this.

spencergibb · 2017-05-15T14:37:25Z

...etflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientProperties.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2013-2015 the original author or authors.


khannedy · 2017-05-16T05:28:41Z

hi @ryanjbaxter @spencergibb, please review my changes

spencergibb · 2017-05-20T00:39:30Z

...tflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientFactoryBean.java

-		// optional values
+		configureFeign(context, builder);
+
+		if (decode404) {


Why exclude decode404 from properties?

Let me add decode404 to configuration properties

spencergibb · 2017-05-20T01:08:17Z

Targeting the Edgware release this summer, should be merged in the next week or two.

khannedy · 2017-05-22T09:13:28Z

@spencergibb I've already added decode404 to configuration properties

spencergibb · 2017-06-07T19:20:27Z

docs/src/main/asciidoc/spring-cloud-netflix.adoc

+
+If we create both `@Configuration` bean and configuration properties, configuration properties will win.
+It will override `@Configuration` values. But if you want to change the priority to `@Configuration`,
+you can change `feign.client.primary` to `false`.


This name is bad and doesn't convey that setting this to false will result in @Configuration winning. Maybe feign.client.default-to-properties?

ok, I'll change properties name to default-to-properties

spencergibb · 2017-06-07T19:20:59Z

...etflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientProperties.java

+/**
+ * @author Eko Kurniawan Khannedy
+ */
+@Data


Can you remove lombok, we're going to move away, it would be helpful to not have to convert this later.

sure, I'll remove lombok

… to default-to-properties

khannedy · 2017-06-08T11:33:03Z

@spencergibb please review the changes

khannedy added 3 commits May 13, 2017 18:06

add feign client properties

5931f3c

configure feign builder from properties if exists

4b1a6fa

change string config to class config and add unit test for feign clie…

630ab37

…nt using configuration properties

ryanjbaxter requested changes May 15, 2017

View reviewed changes

spencergibb requested changes May 15, 2017

View reviewed changes

refactoring and add primary flag configuration

d0b06d4

spencergibb reviewed May 20, 2017

View reviewed changes

spencergibb mentioned this pull request May 20, 2017

Enables Feign.Builder customization #1858

Closed

khannedy added 2 commits May 22, 2017 15:46

Merge branch 'master' into features/feign_client_properties

b52389c

add decode404 in feign client configuration properties

76d6896

change unit test properties decode404 to true

89b5911

ryanjbaxter approved these changes May 24, 2017

View reviewed changes

spencergibb requested changes Jun 7, 2017

View reviewed changes

remove lombok from FeignClientProperties and change primary attribute…

b142140

… to default-to-properties

spencergibb approved these changes Jul 7, 2017

View reviewed changes

spencergibb merged commit c2c6347 into spring-cloud:master Jul 7, 2017

khannedy deleted the features/feign_client_properties branch December 4, 2018 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FeignClient Configuration Properties #1942

FeignClient Configuration Properties #1942

khannedy commented May 13, 2017

codecov-io commented May 13, 2017 •

edited

ryanjbaxter May 15, 2017

spencergibb May 15, 2017

ryanjbaxter May 15, 2017

ryanjbaxter May 15, 2017

khannedy May 16, 2017

spencergibb May 15, 2017

spencergibb May 15, 2017

spencergibb May 15, 2017

khannedy commented May 16, 2017

spencergibb May 20, 2017

khannedy May 22, 2017

spencergibb commented May 20, 2017

khannedy commented May 22, 2017

spencergibb Jun 7, 2017

khannedy Jun 8, 2017 •

edited

spencergibb Jun 7, 2017

khannedy Jun 8, 2017

khannedy commented Jun 8, 2017

		@@ -1020,8 +1020,41 @@ public class FooConfiguration {

		This replaces the `SpringMvcContract` with `feign.Contract.Default` and adds a `RequestInterceptor` to the collection of `RequestInterceptor`.

		`@FeignClient` also can be configured using configuration properties. If we create both `@Configuration` bean and configuration properties, configuration properties will win. It will override `@Configuration` values.

		@@ -0,0 +1,57 @@
		/*
		* Copyright 2013-2015 the original author or authors.

FeignClient Configuration Properties #1942

FeignClient Configuration Properties #1942

Conversation

khannedy commented May 13, 2017

codecov-io commented May 13, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khannedy commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencergibb commented May 20, 2017

khannedy commented May 22, 2017

Choose a reason for hiding this comment

khannedy Jun 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khannedy commented Jun 8, 2017

codecov-io commented May 13, 2017 •

edited

khannedy Jun 8, 2017 •

edited