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

Add ResponseInterceptor support #1610

Closed
wants to merge 7 commits into from
Closed

Conversation

feiyanke
Copy link
Contributor

Proposal for #1126

Add ResponseInterceptor and call before and after decode method in order to do customized logic.

@DewaldDeJager
Copy link
Contributor

CC @velo

Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

Could you run mvn license:format and get the build to pass before I can take a look.

feiyanke and others added 3 commits May 2, 2022 00:01
Add the license header.

Co-authored-by: Dewald de Jager <DewaldDeJager@users.noreply.github.com>
* @param response
* @return
*/
void beforeDecode(Response response);
Copy link
Member

Choose a reason for hiding this comment

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

thinking if we could/should have a single method only.... may advantage is could be a functional interface. Also, if someone needs to share context between before and after it would be much easier.

We would have something similar to http filters or around aspects... one argument would have a proceed method that run feign internal code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If make a proceed argument like around aspect, the user need to call the proceed explicitly. It seems like a little confused for user.
May combine the before and after method to one method with two arguments (response and object) and invoke it only after decode, that could be easier to use

Copy link
Member

Choose a reason for hiding this comment

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

I think that is fine, since you are intercepting the call, you need to tell it when to run.

IMHO, that offers much better coding possibilities for anyone using it. Like how to share content from before and after methods....

Aspects work like that, filters work like that, I think we will be fine as long is documented.

@feiyanke
Copy link
Contributor Author

feiyanke commented May 5, 2022

I modify the interceptor to have a single method(aroundDecode), please check if it is what you want? @velo

@misselvexu
Copy link

I modify the interceptor to have a single method(aroundDecode), please check if it is what you want? @velo

Any updates on the progress?

@velo
Copy link
Member

velo commented Jun 21, 2022

Sorry, no, got distracted with something else, will try to get this done now

@velo
Copy link
Member

velo commented Jun 21, 2022

@feiyanke and @misselvexu , I made a few changes to this PR, but sadly I can't push to feiyanke:master

072711b

Take a look, and please lemme know if that would satisfy you usages

@misselvexu
Copy link

@feiyanke and @misselvexu , I made a few changes to this PR, but sadly I can't push to feiyanke:master

072711b

Take a look, and please lemme know if that would satisfy you usages

The requirement of Response extension has been met.

@velo velo closed this Jun 23, 2022
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.

None yet

4 participants