-
Notifications
You must be signed in to change notification settings - Fork 233
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
adds support for http middlewares #330
Changes from all commits
eb9b64f
80d73c0
de3151a
821b2d7
15223c9
d396e42
0edab1d
dcfdcac
406f6e8
5787885
eb70a50
500e03f
0561f88
1892de2
8254940
98d32fb
c367881
dbd5a7e
1d088a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
namespace Microsoft.Kiota.Abstractions { | ||
/// <summary> | ||
/// Represents a middleware option. | ||
/// </summary> | ||
public interface IMiddlewareOption { | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package com.microsoft.kiota; | ||
|
||
/** Represents a middleware option. */ | ||
public interface MiddlewareOption { | ||
|
||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
/** Represents a middleware option. */ | ||
export interface MiddlewareOption { | ||
/** Gets the option key for when adding it to a request. Must be unique. */ | ||
getKey(): string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
using System.Linq; | ||
using System.Collections.Generic; | ||
using System.Net.Http; | ||
using Microsoft.Kiota.Abstractions; | ||
|
||
namespace Microsoft.Kiota.Http.HttpClient { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the likelihood of this namespace being used in the same context as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @baywet, I think @nikithauc is referring to this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aaah we had a long naming debate with @darrelmiller a long time ago. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are transport implementations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well one of them is HTTP of course, another one could be gRPC, etc... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think another common thing between HTTP and gRPC is that they are both protocols. So we can also have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think transport is correct here. Protocol is more accurate. For example, if we introduced WebSockets, we'd have: They use the same transport. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The namespace looks meaningful.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, protocol works for me as well. As this is a discussion on existing naming pattern, and as I'd like Darrel's "blessing" before changing the naming, I'm going to create a separate issue for that. Thanks for all the great suggestions! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/// <summary> | ||
/// This class is used to build the HttpClient instance used by the core service. | ||
/// </summary> | ||
public static class HttpClientBuilder { | ||
/// <summary> | ||
/// Initializes the <see cref="HttpClient"/> with the default configuration and middlewares including a authentention middleware using the <see cref="IAuthenticationProvider"/> if provided. | ||
/// </summary> | ||
/// <param name="authenticationProvider">The <see cref="IAuthenticationProvider"/> to use for authentention.</param> | ||
/// <returns>The <see cref="HttpClient"/> with the default middlewares.</returns> | ||
public static System.Net.Http.HttpClient Create(IAuthenticationProvider authenticationProvider = default) { | ||
var defaultHandlers = CreateDefaultHandlers(authenticationProvider); | ||
var handler = ChainHandlersCollectionAndGetFirstLink(defaultHandlers.ToArray()); | ||
return handler != null ? new System.Net.Http.HttpClient(handler) : new System.Net.Http.HttpClient(); //TODO configure the default client options | ||
} | ||
/// <summary> | ||
/// Creates a default set of middleware to be used by the <see cref="HttpClient"/>. | ||
/// </summary> | ||
/// <param name="authenticationProvider">The <see cref="IAuthenticationProvider"/> to authenticate requests.</param> | ||
/// <returns>A list of the default handlers used by the client.</returns> | ||
public static IList<DelegatingHandler> CreateDefaultHandlers(IAuthenticationProvider authenticationProvider = default) | ||
{ | ||
return new List<DelegatingHandler>(); //TODO add the default middlewares when they are ready | ||
} | ||
/// <summary> | ||
/// Creates a <see cref="DelegatingHandler"/> to use for the <see cref="HttpClient" /> from the provided <see cref="DelegatingHandler"/> instances. Order matters. | ||
/// </summary> | ||
/// <param name="handlers">The <see cref="DelegatingHandler"/> instances to create the <see cref="DelegatingHandler"/> from.</param> | ||
/// <returns>The created <see cref="DelegatingHandler"/>.</returns> | ||
public static DelegatingHandler ChainHandlersCollectionAndGetFirstLink(params DelegatingHandler[] handlers) { | ||
if(handlers == null || !handlers.Any()) return default; | ||
var handlersCount = handlers.Count(); | ||
for(var i = 0; i < handlersCount; i++) { | ||
var handler = handlers[i]; | ||
var previousItemIndex = i - 1; | ||
if(previousItemIndex >= 0) { | ||
var previousHandler = handlers[previousItemIndex]; | ||
previousHandler.InnerHandler = handler; | ||
} | ||
} | ||
return handlers.First(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package com.microsoft.kiota.http; | ||
|
||
import com.microsoft.kiota.AuthenticationProvider; | ||
|
||
import okhttp3.OkHttpClient; | ||
|
||
import javax.annotation.Nullable; | ||
|
||
/** This class is used to build the HttpClient instance used by the core service. */ | ||
public class OkHttpClientBuilder { | ||
private OkHttpClientBuilder() { } | ||
/** | ||
* Creates an OkHttpClient Builder with the default configuration and middlewares including a authentention middleware using the {@link AuthenticationProvider} if provided. | ||
* @param authenticationProvider the authentication provider used to authenticate the requests. | ||
* @return an OkHttpClient Builder instance. | ||
*/ | ||
public static OkHttpClient.Builder Create(@Nullable final AuthenticationProvider authenticationProvider) { | ||
return new OkHttpClient.Builder(); //TODO configure the default client options. | ||
//TODO add the default middlewares when they are ready | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Please use something like [MiddlewareControl](https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/dev/src/middleware/MiddlewareControl.ts instead of using _middlewareOptions. Please move addMiddlewareOptions and getMiddlewareOptions options to the MiddlewareControl.
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.
Yeah I was looking at the dotnet sdk which uses options but have uses control too. I'm not sure why we have a difference between languages today. @darrelmiller any guidance on options or control?
https://www.github.com/microsoftgraph/msgraph-sdk-dotnet-core/tree/dev/src%2FMicrosoft.Graph.Core%2FRequests%2FMiddleware%2FOptions%2FIMiddlewareOption.cs
We're attaching the options to request info to avoid passing multiple parameters between the layers. Request info is already being passed between layers. And the options or controls are related to the request info, which is why it felt natural to add them there. I'm not sure how you'd do it differently without having to pass additional parameters to the http core service?
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.
Both JS and .NET use the term Options. It looks like JS has a container for for middlewareOptions. .NET just uses a dictionary. I'm not sure why JS has the extra layer of indirection. Part of the confusion might be because the original requirement document talked about middleware controls. That notion got renamed to middleware options during implementation.
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 context here. So I'm guessing we're sticking with options then.