FIX: interceptors disapearing on BaseBuilder#clone - #2804
Conversation
| Feign.Builder enrichedBuilder = originalBuilder.enrich(); | ||
| Feign.Builder enrichedBuilderWithInterceptor = enrichedBuilder.requestInterceptor((req) -> {}); |
There was a problem hiding this comment.
Well, by design enrich is meant to be the last thing you invoke.
After you set all your interceptors, encoders, decodes, you build, and build calls enrich witch will apply modifications to client, encoder and other components.
Doing this you are messing around with the Capability process.
If you wanna see my point, change you Capability on line 73 to do something with the requestInterceptor.
You will see that the capability will be ignored making very hard to anyone to understand why the capability is not working.
Now I don't think we can't have this change
There was a problem hiding this comment.
Yes, @velo
I know it. All interceptors added after enrich was invoked will be not processed by Capability. And this is exactly what my module oauth2 does: https://github.com/hosuaby/feign/blob/feature/oauth2/oauth2/src/main/java/feign/auth/oauth2/OAuth2Authentication.java#L80
This OAuth2Authentication is a Capability that adds interceptor, retryer and errorDecoded just before Feign is built. So they are ignored by any other capability.
Even if you approved my PR already, please follow the link, and check once again. Do you agree with this design?
034970d to
f1200b9
Compare
velo
left a comment
velo
left a comment
There was a problem hiding this comment.
Thanks! Clone/interceptor behavior fix is covered by tests, and I do not see compatibility or security concerns.
velo
left a comment
velo
left a comment
There was a problem hiding this comment.
Re-reviewed as part of an open-PR sweep. The fix is correct and the test is a good one — capturing thisB at construction time means it survives Object#clone() still pointing at the original builder, so every fluent call on the enriched clone mutates the original. Replacing it with (B) this at each call site is the right fix, and dropping the thisB filter from getFieldsToEnrich follows naturally.
checkCloneDontLooseInterceptors asserts exactly the right thing: that the original and the enriched builder diverge, in both directions.
The only blocker is that the branch now conflicts with master and CI has never run on it. Could you rebase onto master and push? Once checks are green I'll merge.
One heads-up: #2755 also touches BaseBuilder, so there will be a conflict between the two. I intend to land this one first.
2 participants
Hello @velo
I insist on this change that's why I am opening this PR again. This is not a mere style change, but a fix of a very ennoying bug that I have spotted during development of OAuth2 module. I added the test that reproduces the problem. If you revert
BaseBuilder.javathis test will fail.