-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
FeignClient Configuration Properties #1942
FeignClient Configuration Properties #1942
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is right, but my gut tells me that @Configuration
beans should override properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is precedent for config wins with ribbon
@@ -120,9 +121,63 @@ public void setApplicationContext(ApplicationContext context) throws BeansExcept | |||
builder.decode404(); | |||
} | |||
|
|||
// configure feign builder from properties if exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@ConfigurationProperties("feign.client") | ||
public class FeignClientProperties { | ||
|
||
private String defaultConfig = "default"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not seeing where this is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @ryanjbaxter , you can see in FeignClientFactoryBean
FeignClientProperties.FeignClientConfiguration defaultConfig = properties.getConfig().get(properties.getDefaultConfig());
if (defaultConfig != null) {
configureUsingProperties(defaultConfig, builder);
}
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is precedent for config wins with ribbon
return applicationContext.getBean(tClass); | ||
} catch (NoSuchBeanDefinitionException e) { | ||
try { | ||
return tClass.cast(tClass.newInstance()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using spring to create beans. You can see how ribbon does this.
@@ -0,0 +1,57 @@ | |||
/* | |||
* Copyright 2013-2015 the original author or authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
hi @ryanjbaxter @spencergibb, please review my changes |
// optional values | ||
configureFeign(context, builder); | ||
|
||
if (decode404) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exclude decode404 from properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me add decode404 to configuration properties
Targeting the Edgware release this summer, should be merged in the next week or two. |
@spencergibb I've already added decode404 to configuration properties |
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is bad and doesn't convey that setting this to false will result in @Configuration
winning. Maybe feign.client.default-to-properties
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll change properties name to default-to-properties
/** | ||
* @author Eko Kurniawan Khannedy | ||
*/ | ||
@Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove lombok, we're going to move away, it would be helpful to not have to convert this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I'll remove lombok
… to default-to-properties
@spencergibb please review the changes |
This pull request is improvement for #1931, so we can configure feign client using configuration properties