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
Lazy openfeign bean registration #455
Conversation
without this change we're eagerly resolving placeholder properties in passed URLs / names. Since this happens at bean definition level it's extremely early e.g. Spring Cloud Contract has not yet registered any placeholders that Feign could try to consume. with this change we're changing the bean to become lazy initialized and we resolve the placeholder values at runtime - as late as possible. problems: - one of the tests uses the factory bean - there's no more factory bean since the instance supplier for the bean registration acts as one.
Codecov Report
@@ Coverage Diff @@
## master #455 +/- ##
============================================
- Coverage 77.16% 76.72% -0.44%
- Complexity 434 436 +2
============================================
Files 54 54
Lines 1607 1633 +26
Branches 234 242 +8
============================================
+ Hits 1240 1253 +13
- Misses 252 257 +5
- Partials 115 123 +8
|
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.
Looks good, however, I don't think it makes sense to ignore the test unless we have a plan in place to makes changes to it. We should either modify this one or create a new one or, if that's not possible, remove the existing one.
@@ -47,9 +48,10 @@ | |||
"feign.client.config.default.loggerLevel=full", | |||
"feign.client.config.default.requestInterceptors[0]=org.springframework.cloud.openfeign.FeignClientUsingPropertiesTests.FooRequestInterceptor", | |||
"feign.client.config.default.requestInterceptors[1]=org.springframework.cloud.openfeign.FeignClientUsingPropertiesTests.BarRequestInterceptor" }) | |||
@Ignore("I don't know how to test this since the factory bean is no longer created") |
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 is the factory bean no longer created?
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.
From the initial PR comment: there's no more factory bean since the instance supplier for the bean registration acts as one
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.
Then that class should be removed and the test removed.
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.
LGTM
I would like you consider #443
Probably reusing AbstractFactoryBean patterns will decrease chance Spring-related problems |
#443 still exists on 3.0.1-SNAPSHOT It looks very suspicious to me that AbstractFactoryBean#getObject returns "early singleton proxy" and FeignClientFactoryBean#getObject returns "concrete bean". There must be a reason that base Spring class returns proxy |
Fixes gh-441. |
without this change we're eagerly resolving placeholder properties in passed URLs / names. Since this happens at bean definition level it's extremely early e.g. Spring Cloud Contract has not yet registered any placeholders that Feign could try to consume. with this change we're changing the bean to become lazy initialized and we resolve the placeholder values at runtime - as late as possible. Fixes gh-441. # Conflicts: # spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java # spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java
without this change we're eagerly resolving placeholder properties in passed URLs / names. Since this happens at bean definition level it's extremely early e.g. Spring Cloud Contract has not yet registered any placeholders that Feign could try to consume.
with this change we're changing the bean to become lazy initialized and we resolve the placeholder values at runtime - as late as possible.
problems:
related issue spring-cloud/spring-cloud-contract#1303