-
Notifications
You must be signed in to change notification settings - Fork 29
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 issue where user specified interceptors where being overridden b… #1758
base: main
Are you sure you want to change the base?
Conversation
… default interceptors
@raghucha please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
//Skip adding interceptor if that class of interceptor already exist. | ||
final List<String> appliedInterceptors = new ArrayList<>(); | ||
for(Interceptor interceptor: builder.interceptors()) { | ||
appliedInterceptors.add(interceptor.getClass().toString()); | ||
} | ||
for (Interceptor interceptor:interceptors){ | ||
for (Interceptor interceptor:createDefaultGraphInterceptors(graphClientOption)){ |
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.
Thanks for the changes @raghucha. We might have a bigger problem here.
I think this could lead to conflicts since the order of handlers in the pipeline matters. e.g. by default all retries/redirects should include query parameter name decoding etc. A potential customization of query parameter decoding could mean it's added before the retry & redirect handlers..
I think we should avoid filling in missing middleware totally in case a developer chooses to pass their own interceptors. This seems to align with .NET and other languages as well.
Thoughts @baywet @andrueastman?
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.
correct, order matters, and this change might impact it. This is what was done in dotnet to address the same concern. We should probably align with that instead.
microsoft/kiota-http-dotnet#246
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.
Thanks for added context Vincent. Would you like to implement this instead @raghucha?
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 can implement it however i have few design question . Below is a sample code from KiotaClientFactory. If anyone uses below method to create a Client they will never be able to Override the default interceptor functionality .And Question such as if they use addInterceptor
Method will also never be able to override . Is this acceptable?
@Nonnull public static OkHttpClient.Builder create(
@Nonnull final BaseBearerTokenAuthenticationProvider authenticationProvider) {
ArrayList<Interceptor> interceptors = new ArrayList<>(createDefaultInterceptorsAsList());
interceptors.add(new AuthorizationHandler(authenticationProvider));
return create(interceptors);
}
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.
The createDefaultInterceptorsAsList was added two days ago. I'm not sure why we made it public @Ndiritu ?
If that was an oversight, I'd be ok with making it private again.
Regardless of whether we keep it public, we should be able to add overloads all the way down the stack that accept options. And simply point the current methods that do not accept the options to call those new overloads.
Let us know if you have any additional comments or questions.
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.
Apologies, I should have made this clearer in the PR description.
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.
are we using it outside of kiota java? (i.e. here in core)
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.
Added a similar create(List<Interceptor>)
overload in Graph core ref but not createDefaultInterceptorsAsList()
. The latter is only in Kiota Java.
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.
am i correct in assuming even with the fix in KiotaClientFactory as done in .net we would not be able to overload the default interceptor using . addInterceptor
method, we a need to pass it as constructor of the create method ( as there is no option to remove an interceptor once its added?)
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.
am i correct in assuming even with the fix in KiotaClientFactory as done in .net we would not be able to overload the default interceptor using
. addInterceptor
method, we a need to pass it as constructor of the create method ( as there is no option to remove an interceptor once its added?)
Yes, addInterceptor()
won't overload. It's a built-in OkHttp method to append interceptors
[Edit]
Link to OkHttp implementation
Use the user Provided Interceptor as higher preference in the Interceptor chain
Fixes #bug 1757
Changes proposed in this pull request
Other links