aligns naming conventions across packages

[baywet]

Description

• fixes http cores should be named protocols to allow for multiple protocol implementations #444
• fixes Rename IMiddlewareOption to IRequestOption #635

Generation diff

microsoft/kiota-samples#317

[andrueastman]

The refactoring changes look good to me.

The github action for the httpClient project for dotnet needs to be updated to reflect the updated project name then we can merge this in.

[baywet]

Thanks a lot! This is a very good catch of something I had looked over. Fixed.

[nikithauc]

Why are we keeping a marker interface for the request option?

My suggestion is the keep options as Map<"string", "unknown/object">.

There are multiple ways I can think of how the user can use the request option property, at least in JavaScript.

1. The request option can be any custom information specific to the user library with primitive data type values or a callback.
2. In JS/TS, the request option can be a request configuration such as an abort signal.
3. In my opinion we should maintain the name IMiddlewareOption in the core library and read information from the request option into the middleware options depending on the options key. The naming is clear in that case and the options can hold multiple types of information improving the scope of RequestInformation.

cc : @MIchaelMainer @zengin

[andrueastman]

My suggestion is the keep options as Map<"string", "unknown/object">.

Isn't that already the case? My understanding is that the requestOption is kept in RequestInformation as a dictionary in this form.

I believe the purpose of the PR is to rename IMiddlewareOption to RequestOption as in reality IMiddlewareOption is an option for the request that just happens to be used in middleware. Renaming captures that this name is limiting as the option can be used for other scenarios as you described.

[baywet]

1. RequestInformation is using a map for options of string, option and not type, option as type testing in TypeScript can be tidious. This reduces the search complexity from o(n) to o(1). We're using the RequestOption interface for type boxing in most languages and additional to "require" derived types to provide a unique key, so the corresponding middleware can search for that specific key, detect whether request specific options are present for the request and act accordingly.
2. why would you carry an abort signal and not just cancel the promise?
3. deferring to @darrelmiller for naming conventions and requirements but Andrew's explanation is pretty much the same Darrel came up with when he requested that change.

[nikithauc]

@andrueastman I agree with you on this.

I believe the purpose of the PR is to rename IMiddlewareOption to RequestOption as in reality IMiddlewareOption is an option for the request that just happens to be used in middleware. Renaming captures that this name is limiting as the option can be used for other scenarios as you described.

Here is my thought :

1. If I look at RequestInformation.options and find Map<"string", "unknown/object"> using intellisense I would know that this property takes in any custom information that I like.
2. I look at RequestInformation.options and find Map<"string", "RequestOption"> using intellisense I would need to know that is the RequestOption interface. Documenting this neatly is a Must.
3. The use of marker interface is debated and pros and cons can be language specific.

AVOID using marker interfaces (interfaces with no members).

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/interface?redirectedfrom=MSDN

@baywet why are we renaming HttpCore to RequestAdapter?

why would you carry an abort signal and not just cancel the promise?

Abort signal is just an example of a request configuration that is in Fetch Example:

import { Client,FetchOptions } from "@microsoft/microsoft-graph-client";
import { AbortController } from "abort-controller"; // <- import when using the abort-controller npm package.

const controller = new AbortController();

const timeout = setTimeout(() => {
	controller.abort();
}, 150);

const fetchOptions: FetchOptions = {
	signal: controller.signal;
}

const client = Client.initWithMiddleware({
  fetchOptions, // Pass the FetchOptions value where the AbortController.signal is set
  authProvider,
  ...
});

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

This configuration can be per request basis

won't this only work if you use 1 client per request. I would have assumed the timeout would need to wrap around an abort controller for each request, and each request would need to have it's own fetch options so that it resets the timer per request.

microsoftgraph/msgraph-sdk-javascript#296 (comment)

[baywet]

intellisense: you'd get a browsable reference and would be able to get right to the request option interface type declaration, which contains doc comments explaining you what is its purpose, if you'd like, we can beef up the doc comments but I feel like it solves you concern.

the guidance you linked is from 2008... it feels like its scope was for dotnet and it's outdated. I believe the main reason they didn't recommend the marker interface pattern at the time is because type testing used to be much more costly back then. Now with the pattern matching and advanced type boxing, it's a different story. On top of that, in the case of TypeScript, the interface defines methods.

abort controller: this should be implementation detail in the request adapter service, we can perfectly implement the cancel promise method to orchestrate the controller (I thought it already was doing that on most fetch implementations). Surfacing it to the fluent API surface level is breaking the abstractions contract (even if we hide that with an unknown type).

timeout scenario; assuming canceling the promise aborts the request (either natively or through our own implementation), we can add a timeout middleware with the current infrastructure we have. That middleware can have default options, and also read in options passed in from the fluent API surface. It then only becomes a simple matter of combining our promise + a set timeout promise + promise.race. more reading

renaming http core: http core didn't convey "what this does" when request adapter conveys the information "adapts request information into something else" (naming is hard).

I hope this answers your concerns, don't hesitate if you have follow ups.

[MIchaelMainer]

I appreciate the move from IMiddlewareOption to RequestOption. It provides me more clarity of purpose (even though the options also can apply to how to handle things in a response) as I don't need to understand "middleware" as a concept.

Is the scope of RequestInformation.options to only be used in middleware, or can we identify reasonable scenarios outside of middleware where it would be used? If it is only used in middleware, then <string, RequestOption> makes sense as we should have an expected interface for defining the information passed to middleware. If it would be used to alter behavior after the response information is passed beyond the middleware, then <string, any|object> makes sense to me.

I appreciate the HttpCore to RequestAdapter change (#444) so that it better addresses the potential for different protocols to be used with OpenAPI. I'd like for us to consider taking this concept a step further and learn from python request.adapters so that we can register (mount in Python) different RequestAdapters to target unique APIs with their own middleware. This will better support multi-host API scenarios for a single client.

[zengin]

I think the new names describes the intent better. Also thanks for the disambiguation by avoiding the naming collision with HttpClient.

I believe the purpose of the PR is to rename IMiddlewareOption to RequestOption as in reality IMiddlewareOption is an option for the request that just happens to be used in middleware. Renaming captures that this name is limiting as the option can be used for other scenarios as you described.

@andrueastman could you give an example to a use case of RequestOption outside middlewares? My understanding was that we were removing the extra level of indirection based on the comment here, (#330 (comment)), but it would be great to hear if there are additional benefits to the renaming with an example. I think that can also help in answering @MIchaelMainer's question on what type the map should be.

[andrueastman]

Thanks for the additional context @zengin. I wasn't aware of the extra level of indirection present in the javascript sdk.

I was also thinking in line with @MIchaelMainer thoughts where the requestOption could potentially be passed to the request at the time of sending so that potentially a response handler would then use the options described to determine/configure how the response is handled.

With regards to the type, I don't think the current RequestOption interface enforces any contract at the moment as the interface is blank (in dotnet, java). In a way, you could say that the dictionary holds just any object with some type checking (which is used as the key value for the Dictionary) in those languages.
TypeScript seems to only enforce the contract that the object should give a unique string to be used as key in the dictionary (which isn't really specifically related to the middleware).
I think it would be possible to keep the interface and still have the advantages of additional use cases as the interface doesn't really enforce any (middleware) specific contracts.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.4% Coverage
0.0% Duplication

[baywet]

Thanks everyone for all the great comments and discussions! merging.