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.

Sign up for GitHub

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

Jump to bottom

fix issue where user specified interceptors where being overridden b… #1758

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

fix issue where user specified interceptors where being overridden b… #1758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/com/microsoft/graph/core/requests/GraphClientFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ public static OkHttpClient.Builder create(@Nonnull BaseBearerTokenAuthentication
*/
@Nonnull
public static OkHttpClient.Builder create(@Nonnull GraphClientOption graphClientOption, @Nonnull Interceptor... interceptors) {
final OkHttpClient.Builder builder = create(graphClientOption);
final OkHttpClient.Builder builder = KiotaClientFactory.create(interceptors);
//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)){
Copy link
Contributor

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?

Copy link
Member

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

Copy link
Contributor

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?

Copy link
Author

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);
   }

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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)

Copy link
Contributor

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.

Copy link
Author

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?)

Copy link
Contributor

@Ndiritu Ndiritu Oct 2, 2024 •
edited
Loading

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

if(appliedInterceptors.contains(interceptor.getClass().toString())) {
continue;
}
Expand Down
40 changes: 33 additions & 7 deletions src/test/java/com/microsoft/graph/core/requests/middleware/GraphTelemetryHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@
import com.microsoft.kiota.http.middleware.RedirectHandler;
import com.microsoft.kiota.http.middleware.RetryHandler;

import com.microsoft.kiota.http.middleware.options.RetryHandlerOption;
import okhttp3.Interceptor;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

Expand All @@ -30,22 +34,35 @@ void telemetryHandlerDefaultTests() throws IOException {

assertNotNull(response);
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedCore));
assertTrue(!response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(CoreConstants.Headers.ANDROID_VERSION_PREFIX)); // Android version is not going to be present on unit tests running on java platform
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
assertTrue(!response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(
CoreConstants.Headers.ANDROID_VERSION_PREFIX)); // Android version is not going to be present on unit tests running on java platform
assertTrue(
response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
}

@Test
void arrayInterceptorsTest() throws IOException {
final String expectedCore = CoreConstants.Headers.GRAPH_VERSION_PREFIX + "/" + CoreConstants.Headers.VERSION;

final Interceptor[] interceptors = {new GraphTelemetryHandler(), new RetryHandler(), new RedirectHandler()};
final Interceptor[] interceptors = {new GraphTelemetryHandler(), getDisabledRetryHandler(),
new RedirectHandler()};
final OkHttpClient client = GraphClientFactory.create(interceptors).build();
final Request request = new Request.Builder().url("https://graph.microsoft.com/v1.0/users/").build();
final Response response = client.newCall(request).execute();

for (Interceptor clientInterceptor : client.interceptors()) {
if (clientInterceptor instanceof RetryHandler) {
RetryHandlerOption retryOptions = ((RetryHandler) clientInterceptor).getRetryOptions();
Assertions.assertEquals(0, retryOptions.maxRetries());
Assertions.assertEquals(0, retryOptions.delay());

}
}

assertNotNull(response);
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedCore));
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
assertTrue(
response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
}

@Test
Expand All @@ -59,7 +76,8 @@ void arrayInterceptorEmptyTest() throws IOException {

assertNotNull(response);
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedCore));
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
assertTrue(
response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(defaultSDKVersion));
}

@Test
Expand All @@ -76,7 +94,7 @@ void testClientOptions() throws IOException {
graphClientOption.setGraphServiceTargetVersion(serviceLibVer);

final String expectedCoreVer =
CoreConstants.Headers.GRAPH_VERSION_PREFIX + "/" +coreLibVer;
CoreConstants.Headers.GRAPH_VERSION_PREFIX + "/" + coreLibVer;
final String expectedClientEndpoint =
CoreConstants.Headers.JAVA_VERSION_PREFIX + "-" + serviceLibVer + "/" + clientLibVer;

Expand All @@ -85,9 +103,17 @@ void testClientOptions() throws IOException {
final Response response = client.newCall(request).execute();

assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedCoreVer));
assertTrue(response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedClientEndpoint));
assertTrue(
response.request().header(CoreConstants.Headers.SDK_VERSION_HEADER_NAME).contains(expectedClientEndpoint));
assertTrue(response.request().header(CoreConstants.Headers.CLIENT_REQUEST_ID).contains(requestId));
}

private static @NotNull RetryHandler getDisabledRetryHandler() {
RetryHandlerOption retryHandlerOption = new RetryHandlerOption(
(delay, executionCount, request, response) -> false, 0, 0);
RetryHandler retryHandler = new RetryHandler(retryHandlerOption);
return retryHandler;
}
}


Loading