adds support for http middlewares #330

baywet · 2021-07-14T20:02:54Z

Summary

adds http client builder to allow for default middlewares and customization
renames chaining method
adds support for middleware options in dotnet abstractions and core

fixes #120

Generation Result

nikithauc · 2021-07-21T20:42:08Z

abstractions/typescript/src/requestInfo.ts

@@ -15,6 +16,22 @@ export class RequestInfo {
    public queryParameters: Map<string, object> = new Map<string, object>(); //TODO: case insensitive
    /** The Request Headers. */
    public headers: Map<string, string> = new Map<string, string>(); //TODO: case insensitive
+    private _middlewareOptions = new Map<string, MiddlewareOption>(); //TODO: case insensitive


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.

A separate class for managing the properties as an object allows extensibility. A user can extend the MiddlewareControl class and flexibly customize their get and add middleware options or any property of the requestinfo class.

We can observe Single Responsibility Principle if we neatly segregate some functionalities into specific classes.

Avoids bulking up the RequestInfo class with too many methods.

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?

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.

Thanks for the context here. So I'm guessing we're sticking with options then.

nikithauc · 2021-07-21T20:49:30Z

What is the reason to use predefined names such as fetch, okhttp as is and not a custom name like kiota-fetch or kiota-httpclient?

My concern is with

possible runtime issues incurred due to dependencies.
ambiguity in the code.

baywet · 2021-07-21T20:58:26Z

@nikithauc I'm not sure what you are referring to? The package name? A specific class?

zengin · 2021-08-03T16:42:21Z

http/dotnet/httpclient/src/HttpClientBuilder.cs

+using System.Net.Http;
+using Microsoft.Kiota.Abstractions;
+
+namespace Microsoft.Kiota.Http.HttpClient {


What is the likelihood of this namespace being used in the same context as System.Net.Http.HttpClient? If they show up in the same context, the naming collision can be confusing for the customers. Could we rename the last part or drop it altogether to bump the namespace one level up to Microsoft.Kiota.Http?

@baywet, I think @nikithauc is referring to this.

aaah we had a long naming debate with @darrelmiller a long time ago.
I wanted to name this Microsoft.Kiota.Transport.Http leaving room for other transport implementations potentially in the future. Thoughts on that name? And for typescript we'd have something like @microsoft/kiota-transport-http.

What are transport implementations?

well one of them is HTTP of course, another one could be gRPC, etc...

I think another common thing between HTTP and gRPC is that they are both protocols. So we can also have Microsoft.Kiota.Protocol.Http or Microsoft.Kiota.Protocols.Http as the namespace alternatives. I am OK with Microsoft.Kiota.Transport.Http too.

I don't think transport is correct here. Protocol is more accurate. For example, if we introduced WebSockets, we'd have:
Microsoft.Kiota.Protocol.Http
Microsoft.Kiota.Protocol.WebSocket

They use the same transport.

Microsoft.Kiota.Protocol.Http
Microsoft.Kiota.Protocol.WebSocket

The namespace looks meaningful.

Hypertext Transfer Protocol (HTTP) is an application-layer protocol for transmitting hypermedia documents, such as HTML. It was designed for communication between web browsers and web servers, but it can also be used for other purposes.

https://developer.mozilla.org/en-US/docs/Web/HTTP

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!

http/dotnet/httpclient/src/HttpClientBuilder.cs

…mization

- adds unit tests for options parameter

sonarqubecloud · 2021-08-04T13:58:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

93.4% Coverage
0.0% Duplication

baywet added this to the Alpha milestone Jul 14, 2021

baywet self-assigned this Jul 14, 2021

baywet mentioned this pull request Jul 14, 2021

add support for middleware pipeline for request execution #120

Closed

Base automatically changed from feature/client-configuration to main July 15, 2021 11:15

baywet force-pushed the feature/middlewares branch from fed27a4 to b16df2f Compare July 15, 2021 11:51

baywet changed the base branch from main to feature/request-builders-ctors July 15, 2021 12:47

baywet force-pushed the feature/middlewares branch from b16df2f to ba07130 Compare July 15, 2021 12:48

baywet marked this pull request as ready for review July 16, 2021 14:25

baywet requested review from abfarah, darrelmiller, ddyett, MIchaelMainer, nikithauc, peombwa and zengin as code owners July 16, 2021 14:25

baywet marked this pull request as draft July 16, 2021 14:29

baywet marked this pull request as ready for review July 16, 2021 18:30

baywet force-pushed the feature/request-builders-ctors branch from c7acaac to 276e900 Compare July 20, 2021 14:44

baywet force-pushed the feature/middlewares branch from f1e7b4f to f70d6c1 Compare July 21, 2021 13:34

baywet added the generator Issues or improvements relater to generation capabilities. label Jul 21, 2021

nikithauc reviewed Jul 21, 2021

View reviewed changes

Base automatically changed from feature/request-builders-ctors to main July 22, 2021 11:19

baywet requested a review from ramsessanchez as a code owner July 22, 2021 11:19

baywet force-pushed the feature/middlewares branch 2 times, most recently from 6c08c5f to 4f0bcb8 Compare July 28, 2021 16:39

zengin suggested changes Aug 3, 2021

View reviewed changes

nikithauc requested a review from andrueastman August 3, 2021 18:39

- adds http client builder to allow for default middlewares and custo…

eb9b64f

…mization

baywet added 13 commits August 4, 2021 09:43

- adds middleware option in java abstractions

0edab1d

- adds default client configuration for java http

dcfdcac

- adds options parameter for java generation

406f6e8

- fixes java overload generation with middleware options

5787885

- adds middleware options for typescript abstractions

eb70a50

- adds middleware options support to typescript generation

500e03f

- adds a middleware pipeline for http core typescript

0561f88

- bumps versions for abstractions and http libs

1892de2

- code linting

8254940

- fixes existing unit tests

98d32fb

- adds unit test for java overloads cross reference

c367881

- adds unit tests for options parameter

- adds missing middleware options parameters to middlewares

dbd5a7e

- performance fix for handlers chaining for dotnet

1d088a6

baywet force-pushed the feature/middlewares branch from 4f0bcb8 to 1d088a6 Compare August 4, 2021 13:56

baywet mentioned this pull request Aug 4, 2021

http cores should be named protocols to allow for multiple protocol implementations #444

Closed

baywet requested review from zengin and nikithauc August 4, 2021 14:08

zengin approved these changes Aug 4, 2021

View reviewed changes

baywet merged commit dac49f5 into main Aug 4, 2021

baywet deleted the feature/middlewares branch August 4, 2021 18:12

baywet mentioned this pull request Aug 4, 2021

Add support for request option generation and middleware configuration in ruby #446

Closed

5 tasks

baywet added a commit that referenced this pull request Aug 4, 2021

- fixes a bad API usage pattern after #330 in unit tests

3384f01

baywet added a commit that referenced this pull request Aug 5, 2021

- fixes a bad API usage pattern after #330 in unit tests

0a5a100

baywet added a commit that referenced this pull request Aug 10, 2021

- fixes a bad API usage pattern after #330 in unit tests

8c5aa60

baywet added a commit that referenced this pull request Aug 11, 2021

- fixes a bad API usage pattern after #330 in unit tests

567112a

baywet added a commit that referenced this pull request Aug 13, 2021

- fixes a bad API usage pattern after #330 in unit tests

6399afd

baywet added a commit that referenced this pull request Aug 16, 2021

- fixes a bad API usage pattern after #330 in unit tests

9155f0b

baywet added a commit that referenced this pull request Aug 17, 2021

- fixes a bad API usage pattern after #330 in unit tests

3ee09d0

baywet mentioned this pull request Aug 24, 2021

adds options support for native response handlers #525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for http middlewares #330

adds support for http middlewares #330

baywet commented Jul 14, 2021 •

edited

Loading

nikithauc Jul 21, 2021

baywet Jul 21, 2021

darrelmiller Jul 21, 2021

baywet Jul 21, 2021

nikithauc commented Jul 21, 2021

baywet commented Jul 21, 2021

zengin Aug 3, 2021

zengin Aug 3, 2021

baywet Aug 3, 2021

nikithauc Aug 3, 2021

baywet Aug 3, 2021

zengin Aug 3, 2021

MIchaelMainer Aug 3, 2021

nikithauc Aug 3, 2021

baywet Aug 4, 2021

baywet Aug 4, 2021

sonarqubecloud bot commented Aug 4, 2021

adds support for http middlewares #330

adds support for http middlewares #330

Conversation

baywet commented Jul 14, 2021 • edited Loading

Summary

Generation Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikithauc commented Jul 21, 2021

baywet commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 4, 2021

baywet commented Jul 14, 2021 •

edited

Loading