Skip to content
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

FIX: interceptors disapearing on BaseBuilder#clone #2804

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hosuaby
Copy link
Contributor

@hosuaby hosuaby commented Mar 4, 2025

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.java this test will fail.

Comment on lines 78 to 79
Feign.Builder enrichedBuilder = originalBuilder.enrich();
Feign.Builder enrichedBuilderWithInterceptor = enrichedBuilder.requestInterceptor((req) -> {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants